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 21:09:56 -0700 |
| |
On 3/30/21 8:56 PM, John Hubbard wrote: > 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.
Actually, re-reading your and Jason's earlier points in the thread, I see that I'm *not* missing anything, and we are actually in agreement about how the code operates. OK, good!
Also, as you point out above, maybe the "try_" prefix is not really accurate either, given how this works. So maybe we have arrived at something like:
try_to_munlock() --> page_mlock() // or mlock_page()... try_to_munlock_one() --> page_mlock_one()
thanks, -- John Hubbard NVIDIA
| |