On Tue, 28 Jul 2015, Vlastimil Babka wrote: > On 07/28/2015 03:49 PM, Eric B Munson wrote: > >On Tue, 28 Jul 2015, Michal Hocko wrote: > > > > [...] > > >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. > > >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. > > >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.
Attachment:
signature.asc
Description: Digital signature