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

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

Although, it seems reasonable to tack such renaming patches onto the tail end
of this series. But whatever works.

thanks,
-- 
John Hubbard
NVIDIA

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