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

On Wednesday, 31 March 2021 9:43:19 AM AEDT John Hubbard
wrote:
> On 3/30/21 3:24 PM, Jason Gunthorpe wrote:
> ...
> >> As far as I can tell this has always been called try_to_munlock()
even
though
> >> it appears to do the opposite.
> > 
> > Maybe we should change it then?
> > 
> >>> /**
> >>>   * 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.
> >>>   */
> >>
> >> In other words it sets PG_mlocked if one or more vmas has it
mlocked. So
> >> try_to_mlock() might be a better name, except that seems to have
the
potential
> >> for confusion as well because it's only called from the
munlock code path
and
> >> never for mlock.
> > 
> > That explanation makes more sense.. This function looks like it is
> > 'set PG_mlocked of the page if any vm->flags has VM_LOCKED'
> > 
> > Maybe call it check_vm_locked or something then and reword the above
> > comment?
> > 
> > (and why is it OK to read vm->flags for this without any locking?)
> > 
> >>> Something needs attention here..
> >>
> >> I think the code is correct, but perhaps the naming could be
better.
Would be
> >> interested hearing any thoughts on renaming try_to_munlock() to 
try_to_mlock()
> >> as the current name appears based on the context it is called from
(munlock)
> >> rather than what it does (mlock).
> > 
> > The point of this patch is to make it clearer, after all, so I'd
> > change something and maybe slightly clarify the comment.
> > 
Yep, agree with that.
 
> I'd add that, after looking around the calling code, this is a really 
unhappy
> pre-existing situation. Anyone reading this has to remember at which point 
in the
> call stack the naming transitions from "do the opposite of what the
name
says",
> to "do what the name says".
>
> +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?

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

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:

/**
  * 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.

> 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. :)

>> 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,
-- 
John Hubbard
NVIDIA