On Mon, 27 Jul 2015, Vlastimil Babka wrote:

On 07/24/2015 11:28 PM, Eric B Munson wrote:

...

quoted

Changes from V4:
Drop all architectures for new sys call entries except x86[_64] and MIPS
Drop munlock2 and munlockall2
Make VM_LOCKONFAULT a modifier to VM_LOCKED only to simplify book keeping
Adjust tests to match

Hi, thanks for considering my suggestions. Well, I do hope there
were correct as API's are hard and I'm no API expert. But since
API's are also impossible to change after merging, I'm sorry but
I'll keep pestering for one last thing. Thanks again for persisting,
I do believe it's for the good thing!

The thing is that I still don't like that one has to call
mlock2(MLOCK_LOCKED) to get the equivalent of the old mlock(). Why
is that flag needed? We have two modes of locking now, and v5 no
longer treats them separately in vma flags. But having two flags
gives us four possible combinations, so two of them would serve
nothing but to confuse the programmer IMHO. What will mlock2()
without flags do? What will mlock2(MLOCK_LOCKED | MLOCK_ONFAULT) do?
(Note I haven't studied the code yet, as having agreed on the API
should come first. But I did suggest documenting these things more
thoroughly too...)
OK I checked now and both cases above seem to return EINVAL.

So about the only point I see in MLOCK_LOCKED flag is parity with
MAP_LOCKED for mmap(). But as Kirill said (and me before as well)
MAP_LOCKED is broken anyway so we shouldn't twist the rest just of
the API to keep the poor thing happier in its misery.

Also note that AFAICS you don't have MCL_LOCKED for mlockall() so
there's no full parity anyway. But please don't fix that by adding
MCL_LOCKED :)

Thanks!

I have an MLOCK_LOCKED flag because I prefer an interface to be
explicit.  The caller of mlock2() will be required to fill in the flags
argument regardless.  I can drop the MLOCK_LOCKED flag with 0 being the
value for LOCKED, but I thought it easier to make clear what was going
on at any call to mlock2().  If user space defines a MLOCK_LOCKED that
happens to be 0, I suppose that would be okay.

We do actually have an MCL_LOCKED, we just call it MCL_CURRENT.  Would
you prefer that I match the name in mlock2() (add MLOCK_CURRENT
instead)?

Finally, on the question of MAP_LOCKONFAULT, do you just dislike
MAP_LOCKED and do not want to see it extended, or is this a NAK on the
set if that patch is included.  I ask because I have to spin a V6 to get
the MLOCK flag declarations right, but I would prefer not to do a V7+.
If this is a NAK with, I can drop that patch and rework the tests to
cover without the mmap flag.  Otherwise I want to keep it, I have an
internal user that would like to see it added.