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