Messages in this thread | | | Subject | Re: [PATCH v7 3/8] mm/rmap: Split try_to_munlock from try_to_unmap | From | John Hubbard <> | Date | Tue, 30 Mar 2021 20:56:38 -0700 |
| |
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
| |