On Wed, Mar 31, 2021 at 03:15:47PM +1100, Alistair Popple wrote:

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

quoted

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

quoted

quoted

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

grief.

quoted

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

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.

It is really searching all VMA's to see if the VMA flag is set and if
any are found then it mlocks the page.

But presenting this rountine in its simplified form raises lots of
questions:

 - What locking is being used to read the VMA flag?
 - Why do we need to manipulate global struct page flags under the
   page table locks of a single VMA?
 - Why do we need to check for huge pages inside the VMA loop, not
   before going to the rmap? PageTransCompoundHead() is not sensitive to
   the PTEs. (and what happens if the huge page breaks up concurrently?)
 - Why do we clear the mlock bit then run around to try and set it?
   Feels racey.

Jason