On Wednesday, 31 March 2021 2:56:38 PM AEDT John Hubbard wrote:

On 3/30/21 3:56 PM, Alistair Popple wrote:
...

quoted

quoted

+1 for renaming "munlock*" items to "mlock*", where applicable. good 

grief.

quoted

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

quoted

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.

quoted

This is actually inspired from a suggestion in Documentation/vm/

unevictable-

quoted

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

quoted

    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.

quoted

quoted

Although, it seems reasonable to tack such renaming patches onto the tail

end

quoted

of this series. But whatever works.

Unless anyone objects strongly I will roll the rename into this patch as 

there

quoted

is only one caller of try_to_munlock.

  - Alistair

No objections here. :)

thanks,