On Tue, 28 Jul 2015, Vlastimil Babka wrote:

On 07/28/2015 03:49 PM, Eric B Munson wrote:

quoted

On Tue, 28 Jul 2015, Michal Hocko wrote:

[...]

quoted

The only
remaining question I have is should we have 2 new mlockall flags so that
the caller can explicitly set VM_LOCKONFAULT in the mm->def_flags vs
locking all current VMAs on fault.  I ask because if the user wants to
lock all current VMAs the old way, but all future VMAs on fault they
have to call mlockall() twice:

mlockall(MCL_CURRENT);
mlockall(MCL_CURRENT | MCL_FUTURE | MCL_ONFAULT);

This has the side effect of converting all the current VMAs to
VM_LOCKONFAULT, but because they were all made present and locked in the
first call, this should not matter in most cases.

Shouldn't the user be able to do this?

mlockall(MCL_CURRENT)
mlockall(MCL_FUTURE | MCL_ONFAULT);

Note that the second call shouldn't change (i.e. munlock) existing
vma's just because MCL_CURRENT is not present. The current
implementation doesn't do that thanks to the following in
do_mlockall():

        if (flags == MCL_FUTURE)
                goto out;

before current vma's are processed and MCL_CURRENT is checked. This
is probably so that do_mlockall() can also handle the munlockall()
syscall.
So we should be careful not to break this, but otherwise there are
no limitations by not having two MCL_ONFAULT flags. Having to do
invoke syscalls instead of one is not an issue as this shouldn't be
frequent syscall.

Good catch, my current implementation did break this and is now fixed.

quoted

The catch is that,
like mmap(MAP_LOCKED), mlockall() does not communicate if mm_populate()
fails.  This has been true of mlockall() from the beginning so I don't
know if it needs more than an entry in the man page to clarify (which I
will add when I add documentation for MCL_ONFAULT).

Good point.

quoted

In a much less
likely corner case, it is not possible in the current setup to request
all current VMAs be VM_LOCKONFAULT and all future be VM_LOCKED.

So again this should work:

mlockall(MCL_CURRENT | MCL_ONFAULT)
mlockall(MCL_FUTURE);

But the order matters here, as current implementation of
do_mlockall() will clear VM_LOCKED from def_flags if MCL_FUTURE is
not passed. So *it's different* from how it handles MCL_CURRENT (as
explained above). And not documented in manpage. Oh crap, this API
is a closet full of skeletons. Maybe it was an unnoticed regression
and we can restore some sanity?

I will add a note about the ordering problem to the manpage as well.
Unfortunately, the basic idea of clearing VM_LOCKED from mm->def_flags
if MCL_FUTURE is not specified but not doing the same for MCL_CURRENT
predates the move to git, so I am not sure if it was ever different.