Nouveau - Apr 2021 - [PATCH v7 3/8] mm/rmap: Split try_to_munlock from try_to_unmap

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,
>

On Wed, Mar 31, 2021 at 03:15:47PM +1100, Alistair Popple
wrote:
> 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.
It is really searching all VMA's to see if the VMA flag is set and if
any are found then it mlocks the page.

But presenting this rountine in its simplified form raises lots of
questions:

 - What locking is being used to read the VMA flag?
 - Why do we need to manipulate global struct page flags under the
   page table locks of a single VMA?
 - Why do we need to check for huge pages inside the VMA loop, not
   before going to the rmap? PageTransCompoundHead() is not sensitive to
   the PTEs. (and what happens if the huge page breaks up concurrently?)
 - Why do we clear the mlock bit then run around to try and set it?
   Feels racey.

Jason

On Wednesday, 31 March 2021 10:57:46 PM AEDT Jason Gunthorpe
wrote:
> On Wed, Mar 31, 2021 at 03:15:47PM +1100, Alistair Popple wrote:
> > 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.
> 
> It is really searching all VMA's to see if the VMA flag is set and if
> any are found then it mlocks the page.
> 
> But presenting this rountine in its simplified form raises lots of
> questions:
> 
>  - What locking is being used to read the VMA flag?
>  - Why do we need to manipulate global struct page flags under the
>    page table locks of a single VMA?
I was wondering that and questioned it in an earlier version of this series. I 
have done some digging and the commit log for b87537d9e2fe ("mm: rmap use
pte
lock not mmap_sem to set PageMlocked") provides the original justification.

It's fairly long so I won't quote it here but the summary seems to be
that
among other things the combination of page lock and ptl makes this safe. I 
have yet to verify if everything there still holds and is sensible, but the 
last paragraph certainly is :-)

"Stopped short of separating try_to_munlock_one() from try_to_munmap_one()
on this occasion, but that's probably the sensible next step - with a
rename, given that try_to_munlock()'s business is to try to set
Mlocked."
>  - Why do we need to check for huge pages inside the VMA loop, not
>    before going to the rmap? PageTransCompoundHead() is not sensitive to
>    the PTEs. (and what happens if the huge page breaks up concurrently?)
>  - Why do we clear the mlock bit then run around to try and set it?
I don't have an answer for that as I'm not (yet) across all the mlock
code
paths, but I'm hoping this patch at least won't change anything.
>    Feels racey.
>
> Jason
>