From: Alistair Popple <apopple@nvidia.com>
To: John Hubbard <jhubbard@nvidia.com>
Cc: rcampbell@nvidia.com, willy@infradead.org,
linux-doc@vger.kernel.org, nouveau@lists.freedesktop.org,
dri-devel@lists.freedesktop.org, linux-kernel@vger.kernel.org,
kvm-ppc@vger.kernel.org, hch@infradead.org, linux-mm@kvack.org,
jglisse@redhat.com, bskeggs@redhat.com,
Jason Gunthorpe <jgg@nvidia.com>,
akpm@linux-foundation.org, Christoph Hellwig <hch@lst.de>
Subject: Re: [PATCH v7 3/8] mm/rmap: Split try_to_munlock from try_to_unmap
Date: Wed, 31 Mar 2021 15:15:47 +1100 [thread overview]
Message-ID: <23784464.epyy5R1Yul@nvdebian> (raw)
In-Reply-To: <b4b11c59-975d-26c7-043a-6acddff78dfd@nvidia.com>
On Wednesday, 31 March 2021 2:56:38 PM AEDT John Hubbard wrote:
> On 3/30/21 3:56 PM, Alistair Popple wrote:
> ...
> >> +1 for renaming "munlock*" items to "mlock*", where applicable. good
grief.
> >
> > At least the situation was weird enough to prompt further investigation :)
> >
> > Renaming to mlock* doesn't feel like the right solution to me either
though. I
> > am not sure if you saw me responding to myself earlier but I am thinking
> > renaming try_to_munlock() -> page_mlocked() and try_to_munlock_one() ->
> > page_mlock_one() might be better. Thoughts?
> >
>
> Quite confused by this naming idea. Because: try_to_munlock() returns
> void, so a boolean-style name such as "page_mlocked()" is already not a
> good fit.
>
> Even more important, though, is that try_to_munlock() is mlock-ing the
> page, right? Is there some subtle point I'm missing? It really is doing
> an mlock to the best of my knowledge here. Although the kerneldoc
> comment for try_to_munlock() seems questionable too:
It's mlocking the page if it turns out it still needs to be locked after
unlocking it. But I don't think you're missing anything.
> /**
> * try_to_munlock - try to munlock a page
> * @page: the page to be munlocked
> *
> * Called from munlock code. Checks all of the VMAs mapping the page
> * to make sure nobody else has this page mlocked. The page will be
> * returned with PG_mlocked cleared if no other vmas have it mlocked.
> */
>
> ...because I don't see where, in *this* routine, it clears PG_mlocked!
>
> Obviously we agree that a routine should be named based on what it does,
> rather than on who calls it. So I think that still leads to:
>
> try_to_munlock() --> try_to_mlock()
> try_to_munlock_one() --> try_to_mlock_one()
>
> Sorry if I'm missing something really obvious.
Nope, I confused things somewhat by blindly quoting the documentation whilst
forgetting that try_to_munlock() returns void rather than a bool.
> > This is actually inspired from a suggestion in Documentation/vm/
unevictable-
> > lru.rst which warns about this problem:
> >
> > try_to_munlock() Reverse Map Scan
> > ---------------------------------
> >
> > .. warning::
> > [!] TODO/FIXME: a better name might be page_mlocked() - analogous to
the
> > page_referenced() reverse map walker.
> >
>
> This is actually rather bad advice! page_referenced() returns an
> int-that-is-really-a-boolean, whereas try_to_munlock(), at least as it
> stands now, returns void. Usually when I'm writing a TODO item, I'm in a
> hurry, and I think that's what probably happened here, too. :)
So I think we're in agreement. The naming is bad and the advice in the
documentation is also questionable :-)
Thanks for the thoughts, I will re-send this with naming and documentation
updates.
> >> Although, it seems reasonable to tack such renaming patches onto the tail
> > end
> >> of this series. But whatever works.
> >
> > Unless anyone objects strongly I will roll the rename into this patch as
there
> > is only one caller of try_to_munlock.
> >
> > - Alistair
> >
>
> No objections here. :)
>
> thanks,
>
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel
next prev parent reply other threads:[~2021-03-31 4:16 UTC|newest]
Thread overview: 31+ messages / expand[flat|nested] mbox.gz Atom feed top
2021-03-26 0:07 [PATCH v7 0/8] Add support for SVM atomics in Nouveau Alistair Popple
2021-03-26 0:07 ` [PATCH v7 1/8] mm: Remove special swap entry functions Alistair Popple
2021-03-30 18:38 ` Jason Gunthorpe
2021-03-26 0:07 ` [PATCH v7 2/8] mm/swapops: Rework swap entry manipulation code Alistair Popple
2021-03-26 0:08 ` [PATCH v7 3/8] mm/rmap: Split try_to_munlock from try_to_unmap Alistair Popple
2021-03-30 18:49 ` Jason Gunthorpe
2021-03-30 22:09 ` Alistair Popple
2021-03-30 22:16 ` Alistair Popple
2021-03-30 22:24 ` Jason Gunthorpe
2021-03-30 22:43 ` John Hubbard
2021-03-30 22:56 ` Alistair Popple
2021-03-31 3:56 ` John Hubbard
2021-03-31 4:09 ` John Hubbard
2021-03-31 4:15 ` Alistair Popple [this message]
2021-03-31 11:57 ` Jason Gunthorpe
2021-04-01 4:36 ` Alistair Popple
2021-04-01 19:21 ` Shakeel Butt
2021-03-26 0:08 ` [PATCH v7 4/8] mm/rmap: Split migration into its own function Alistair Popple
2021-03-26 0:08 ` [PATCH v7 5/8] mm: Device exclusive memory access Alistair Popple
2021-03-30 19:32 ` Jason Gunthorpe
2021-03-31 12:59 ` Alistair Popple
2021-03-31 13:18 ` Jason Gunthorpe
2021-03-31 13:27 ` Alistair Popple
2021-03-31 13:46 ` Jason Gunthorpe
2021-04-01 0:45 ` Alistair Popple
2021-04-01 0:48 ` Jason Gunthorpe
2021-04-01 2:20 ` Alistair Popple
2021-04-01 11:55 ` Jason Gunthorpe
2021-03-26 0:08 ` [PATCH v7 6/8] mm: Selftests for exclusive device memory Alistair Popple
2021-03-26 0:08 ` [PATCH v7 7/8] nouveau/svm: Refactor nouveau_range_fault Alistair Popple
2021-03-26 0:08 ` [PATCH v7 8/8] nouveau/svm: Implement atomic SVM access Alistair Popple
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=23784464.epyy5R1Yul@nvdebian \
--to=apopple@nvidia.com \
--cc=akpm@linux-foundation.org \
--cc=bskeggs@redhat.com \
--cc=dri-devel@lists.freedesktop.org \
--cc=hch@infradead.org \
--cc=hch@lst.de \
--cc=jgg@nvidia.com \
--cc=jglisse@redhat.com \
--cc=jhubbard@nvidia.com \
--cc=kvm-ppc@vger.kernel.org \
--cc=linux-doc@vger.kernel.org \
--cc=linux-kernel@vger.kernel.org \
--cc=linux-mm@kvack.org \
--cc=nouveau@lists.freedesktop.org \
--cc=rcampbell@nvidia.com \
--cc=willy@infradead.org \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).