Re: [PATCH V5 0/7] Allow user to request memory to be locked on page fault
On 07/29/2015 12:45 PM, Michal Hocko wrote: 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. Vlastimil has already pointed that out. MCL_FUTURE doesn't clear MCL_CURRENT. I was quite surprised in the beginning but it makes a perfect sense. mlockall call shouldn't lead into munlocking, that would be just weird. Clearing MCL_FUTURE on MCL_CURRENT makes sense on the other hand because the request is explicit about _current_ memory and it doesn't lead to any munlocking. Yeah after more thinking it does make some sense despite the perceived inconsistency, but it's definitely worth documenting properly. It also already covers the usecase for munlockall2(MCL_FUTURE) which IIRC you had in the earlier revisions... ___ Linuxppc-dev mailing list Linuxppc-dev@lists.ozlabs.org https://lists.ozlabs.org/listinfo/linuxppc-dev
Re: [PATCH V5 0/7] Allow user to request memory to be locked on page fault
On Tue 28-07-15 09:49:42, Eric B Munson wrote:
On Tue, 28 Jul 2015, Michal Hocko wrote:
[I am sorry but I didn't get to this sooner.]
On Mon 27-07-15 10:54:09, Eric B Munson wrote:
Now that VM_LOCKONFAULT is a modifier to VM_LOCKED and
cannot be specified independentally, it might make more sense to mirror
that relationship to userspace. Which would lead to soemthing like the
following:
A modifier makes more sense.
To lock and populate a region:
mlock2(start, len, 0);
To lock on fault a region:
mlock2(start, len, MLOCK_ONFAULT);
If LOCKONFAULT is seen as a modifier to mlock, then having the flags
argument as 0 mean do mlock classic makes more sense to me.
To mlock current on fault only:
mlockall(MCL_CURRENT | MCL_ONFAULT);
To mlock future on fault only:
mlockall(MCL_FUTURE | MCL_ONFAULT);
To lock everything on fault:
mlockall(MCL_CURRENT | MCL_FUTURE | MCL_ONFAULT);
Makes sense to me. The only remaining and still tricky part would be
the munlock{all}(flags) behavior. What should munlock(MLOCK_ONFAULT)
do? Keep locked and poppulate the range or simply ignore the flag an
just unlock?
I can see some sense to allow munlockall(MCL_FUTURE[|MLOCK_ONFAULT]),
munlockall(MCL_CURRENT) resp. munlockall(MCL_CURRENT|MCL_FUTURE) but
other combinations sound weird to me.
Anyway munlock with flags opens new doors of trickiness.
In the current revision there are no new munlock[all] system calls
introduced. munlockall() unconditionally cleared both MCL_CURRENT and
MCL_FUTURE before the set and now unconditionally clears all three.
munlock() does the same for VM_LOCK and VM_LOCKONFAULT.
OK if new munlock{all}(flags) is not introduced then this is much saner
IMO.
If the user
wants to adjust mlockall flags today, they need to call mlockall a
second time with the new flags, this remains true for mlockall after
this set and the same behavior is mirrored in mlock2.
OK, this makes sense to me.
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.
I think this is OK (worth documenting though) considering that ONFAULT
is just modifier for the current mlock* operation. The memory is locked
the same way for both - aka once the memory is present you do not know
whether it was done during mlock call or later during the fault.
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).
Yes this is true but unlike mmap it seems fixable I guess. We do not have
to unmap and we can downgrade mmap_sem to read and the fault so nobody
can race with a concurent mlock.
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.
Vlastimil has already pointed that out. MCL_FUTURE doesn't clear
MCL_CURRENT. I was quite surprised in the beginning but it makes a
perfect sense. mlockall call shouldn't lead into munlocking, that would
be just weird. Clearing MCL_FUTURE on MCL_CURRENT makes sense on the
other hand because the request is explicit about _current_ memory and it
doesn't lead to any munlocking.
--
Michal Hocko
SUSE Labs
___
Linuxppc-dev mailing list
Linuxppc-dev@lists.ozlabs.org
https://lists.ozlabs.org/listinfo/linuxppc-dev
Re: [PATCH V5 0/7] Allow user to request memory to be locked on page fault
On Tue, 28 Jul 2015, Michal Hocko wrote:
[I am sorry but I didn't get to this sooner.]
On Mon 27-07-15 10:54:09, Eric B Munson wrote:
Now that VM_LOCKONFAULT is a modifier to VM_LOCKED and
cannot be specified independentally, it might make more sense to mirror
that relationship to userspace. Which would lead to soemthing like the
following:
A modifier makes more sense.
To lock and populate a region:
mlock2(start, len, 0);
To lock on fault a region:
mlock2(start, len, MLOCK_ONFAULT);
If LOCKONFAULT is seen as a modifier to mlock, then having the flags
argument as 0 mean do mlock classic makes more sense to me.
To mlock current on fault only:
mlockall(MCL_CURRENT | MCL_ONFAULT);
To mlock future on fault only:
mlockall(MCL_FUTURE | MCL_ONFAULT);
To lock everything on fault:
mlockall(MCL_CURRENT | MCL_FUTURE | MCL_ONFAULT);
Makes sense to me. The only remaining and still tricky part would be
the munlock{all}(flags) behavior. What should munlock(MLOCK_ONFAULT)
do? Keep locked and poppulate the range or simply ignore the flag an
just unlock?
I can see some sense to allow munlockall(MCL_FUTURE[|MLOCK_ONFAULT]),
munlockall(MCL_CURRENT) resp. munlockall(MCL_CURRENT|MCL_FUTURE) but
other combinations sound weird to me.
Anyway munlock with flags opens new doors of trickiness.
In the current revision there are no new munlock[all] system calls
introduced. munlockall() unconditionally cleared both MCL_CURRENT and
MCL_FUTURE before the set and now unconditionally clears all three.
munlock() does the same for VM_LOCK and VM_LOCKONFAULT. If the user
wants to adjust mlockall flags today, they need to call mlockall a
second time with the new flags, this remains true for mlockall after
this set and the same behavior is mirrored in mlock2. 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. 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). 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.
signature.asc
Description: Digital signature
___
Linuxppc-dev mailing list
Linuxppc-dev@lists.ozlabs.org
https://lists.ozlabs.org/listinfo/linuxppc-dev
Re: [PATCH V5 0/7] Allow user to request memory to be locked on page fault
On 07/28/2015 01:17 PM, Michal Hocko wrote:
[I am sorry but I didn't get to this sooner.]
On Mon 27-07-15 10:54:09, Eric B Munson wrote:
Now that VM_LOCKONFAULT is a modifier to VM_LOCKED and
cannot be specified independentally, it might make more sense to mirror
that relationship to userspace. Which would lead to soemthing like the
following:
A modifier makes more sense.
To lock and populate a region:
mlock2(start, len, 0);
To lock on fault a region:
mlock2(start, len, MLOCK_ONFAULT);
If LOCKONFAULT is seen as a modifier to mlock, then having the flags
argument as 0 mean do mlock classic makes more sense to me.
To mlock current on fault only:
mlockall(MCL_CURRENT | MCL_ONFAULT);
To mlock future on fault only:
mlockall(MCL_FUTURE | MCL_ONFAULT);
To lock everything on fault:
mlockall(MCL_CURRENT | MCL_FUTURE | MCL_ONFAULT);
Makes sense to me. The only remaining and still tricky part would be
the munlock{all}(flags) behavior. What should munlock(MLOCK_ONFAULT)
do? Keep locked and poppulate the range or simply ignore the flag an
just unlock?
munlock(all) already lost both MLOCK_LOCKED and MLOCK_ONFAULT flags in
this revision, so I suppose in the next revision it will also not accept
MLOCK_ONFAULT, and will just munlock whatever was mlocked in either mode.
I can see some sense to allow munlockall(MCL_FUTURE[|MLOCK_ONFAULT]),
munlockall(MCL_CURRENT) resp. munlockall(MCL_CURRENT|MCL_FUTURE) but
other combinations sound weird to me.
The effect of munlockall(MCL_FUTURE|MLOCK_ONFAULT), which you probably
intended for converting the onfault to full prepopulation for future
mappings, can be achieved by calling mlockall(MCL_FUTURE) (without
MLOCK_ONFAULT).
Anyway munlock with flags opens new doors of trickiness.
___
Linuxppc-dev mailing list
Linuxppc-dev@lists.ozlabs.org
https://lists.ozlabs.org/listinfo/linuxppc-dev
Re: [PATCH V5 0/7] Allow user to request memory to be locked on page fault
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. signature.asc Description: Digital signature ___ Linuxppc-dev mailing list Linuxppc-dev@lists.ozlabs.org https://lists.ozlabs.org/listinfo/linuxppc-dev
Re: [PATCH V5 0/7] Allow user to request memory to be locked on page fault
[I am sorry but I didn't get to this sooner.]
On Mon 27-07-15 10:54:09, Eric B Munson wrote:
Now that VM_LOCKONFAULT is a modifier to VM_LOCKED and
cannot be specified independentally, it might make more sense to mirror
that relationship to userspace. Which would lead to soemthing like the
following:
A modifier makes more sense.
To lock and populate a region:
mlock2(start, len, 0);
To lock on fault a region:
mlock2(start, len, MLOCK_ONFAULT);
If LOCKONFAULT is seen as a modifier to mlock, then having the flags
argument as 0 mean do mlock classic makes more sense to me.
To mlock current on fault only:
mlockall(MCL_CURRENT | MCL_ONFAULT);
To mlock future on fault only:
mlockall(MCL_FUTURE | MCL_ONFAULT);
To lock everything on fault:
mlockall(MCL_CURRENT | MCL_FUTURE | MCL_ONFAULT);
Makes sense to me. The only remaining and still tricky part would be
the munlock{all}(flags) behavior. What should munlock(MLOCK_ONFAULT)
do? Keep locked and poppulate the range or simply ignore the flag an
just unlock?
I can see some sense to allow munlockall(MCL_FUTURE[|MLOCK_ONFAULT]),
munlockall(MCL_CURRENT) resp. munlockall(MCL_CURRENT|MCL_FUTURE) but
other combinations sound weird to me.
Anyway munlock with flags opens new doors of trickiness.
--
Michal Hocko
SUSE Labs
___
Linuxppc-dev mailing list
Linuxppc-dev@lists.ozlabs.org
https://lists.ozlabs.org/listinfo/linuxppc-dev
Re: [PATCH V5 0/7] Allow user to request memory to be locked on page fault
On Mon, 27 Jul 2015, Vlastimil Babka wrote: On 07/24/2015 11:28 PM, Eric B Munson wrote: ... 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. signature.asc Description: Digital signature ___ Linuxppc-dev mailing list Linuxppc-dev@lists.ozlabs.org https://lists.ozlabs.org/listinfo/linuxppc-dev
Re: [PATCH V5 0/7] Allow user to request memory to be locked on page fault
On Mon, 27 Jul 2015, Vlastimil Babka wrote: On 07/27/2015 03:35 PM, Eric B Munson wrote: On Mon, 27 Jul 2015, Vlastimil Babka wrote: On 07/24/2015 11:28 PM, Eric B Munson wrote: ... 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. I think it's already explicit enough that the user calls mlock2(), no? He obviously wants the range mlocked. An optional flag says that there should be no pre-fault. The caller of mlock2() will be required to fill in the flags argument regardless. I guess users not caring about MLOCK_ONFAULT will continue using plain mlock() without flags anyway. 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. Yeah that would remove the weird 4-states-of-which-2-are-invalid problem I mentioned, but at the cost of glibc wrapper behaving differently than the kernel syscall itself. For little gain. 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)? Hm it's similar but not exactly the same, because MCL_FUTURE is not the same as MLOCK_ONFAULT :) So MLOCK_CURRENT would be even more confusing. Especially if mlockall(MCL_CURRENT | MCL_FUTURE) is OK, but mlock2(MLOCK_LOCKED | MLOCK_ONFAULT) is invalid. MLOCK_ONFAULT isn't meant to be the same as MCL_FUTURE, rather it is meant to be the same as MCL_ONFAULT. MCL_FUTURE only controls if the locking policy will be applied to any new mappings made by this process, not the locking policy itself. The better comparison is MCL_CURRENT to MLOCK_LOCK and MCL_ONFAULT to MLOCK_ONFAULT. MCL_CURRENT and MLOCK_LOCK do the same thing, only one requires a specific range of addresses while the other works process wide. This is why I suggested changing MLOCK_LOCK to MLOCK_CURRENT. It is an error to call mlock2(MLOCK_LOCK | MLOCK_ONFAULT) just like it is an error to call mlockall(MCL_CURRENT | MCL_ONFAULT). The combinations do no make sense. This was all decided when VM_LOCKONFAULT was a separate state from VM_LOCKED. Now that VM_LOCKONFAULT is a modifier to VM_LOCKED and cannot be specified independentally, it might make more sense to mirror that relationship to userspace. Which would lead to soemthing like the following: To lock and populate a region: mlock2(start, len, 0); To lock on fault a region: mlock2(start, len, MLOCK_ONFAULT); If LOCKONFAULT is seen as a modifier to mlock, then having the flags argument as 0 mean do mlock classic makes more sense to me. To mlock current on fault only: mlockall(MCL_CURRENT | MCL_ONFAULT); To mlock future on fault only: mlockall(MCL_FUTURE | MCL_ONFAULT); To lock everything on fault: mlockall(MCL_CURRENT | MCL_FUTURE | MCL_ONFAULT); I think I have talked myself into rewriting the set again :/ 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
Re: [PATCH V5 0/7] Allow user to request memory to be locked on page fault
On 07/27/2015 04:54 PM, Eric B Munson wrote: On Mon, 27 Jul 2015, Vlastimil Babka wrote: 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)? Hm it's similar but not exactly the same, because MCL_FUTURE is not the same as MLOCK_ONFAULT :) So MLOCK_CURRENT would be even more confusing. Especially if mlockall(MCL_CURRENT | MCL_FUTURE) is OK, but mlock2(MLOCK_LOCKED | MLOCK_ONFAULT) is invalid. MLOCK_ONFAULT isn't meant to be the same as MCL_FUTURE, rather it is meant to be the same as MCL_ONFAULT. MCL_FUTURE only controls if the locking policy will be applied to any new mappings made by this process, not the locking policy itself. The better comparison is MCL_CURRENT to MLOCK_LOCK and MCL_ONFAULT to MLOCK_ONFAULT. MCL_CURRENT and MLOCK_LOCK do the same thing, only one requires a specific range of addresses while the other works process wide. This is why I suggested changing MLOCK_LOCK to MLOCK_CURRENT. It is an error to call mlock2(MLOCK_LOCK | MLOCK_ONFAULT) just like it is an error to call mlockall(MCL_CURRENT | MCL_ONFAULT). The combinations do no make sense. How is it an error to call mlockall(MCL_CURRENT | MCL_ONFAULT)? How else would you apply mlock2(MCL_ONFAULT) to all current mappings? Later below you use the same example and I don't think it's different by removing MLOCK_LOCKED flag. This was all decided when VM_LOCKONFAULT was a separate state from VM_LOCKED. Now that VM_LOCKONFAULT is a modifier to VM_LOCKED and cannot be specified independentally, it might make more sense to mirror that relationship to userspace. Which would lead to soemthing like the following: To lock and populate a region: mlock2(start, len, 0); To lock on fault a region: mlock2(start, len, MLOCK_ONFAULT); If LOCKONFAULT is seen as a modifier to mlock, then having the flags argument as 0 mean do mlock classic makes more sense to me. Yup that's what I was trying to suggest. To mlock current on fault only: mlockall(MCL_CURRENT | MCL_ONFAULT); To mlock future on fault only: mlockall(MCL_FUTURE | MCL_ONFAULT); To lock everything on fault: mlockall(MCL_CURRENT | MCL_FUTURE | MCL_ONFAULT); I think I have talked myself into rewriting the set again :/ Sorry :) You could also wait a bit for more input than just from me... 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. I don't want to NAK that patch if you think it's useful. ___ Linuxppc-dev mailing list Linuxppc-dev@lists.ozlabs.org https://lists.ozlabs.org/listinfo/linuxppc-dev
Re: [PATCH V5 0/7] Allow user to request memory to be locked on page fault
On 07/27/2015 03:35 PM, Eric B Munson wrote: On Mon, 27 Jul 2015, Vlastimil Babka wrote: On 07/24/2015 11:28 PM, Eric B Munson wrote: ... 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. I think it's already explicit enough that the user calls mlock2(), no? He obviously wants the range mlocked. An optional flag says that there should be no pre-fault. The caller of mlock2() will be required to fill in the flags argument regardless. I guess users not caring about MLOCK_ONFAULT will continue using plain mlock() without flags anyway. 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. Yeah that would remove the weird 4-states-of-which-2-are-invalid problem I mentioned, but at the cost of glibc wrapper behaving differently than the kernel syscall itself. For little gain. 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)? Hm it's similar but not exactly the same, because MCL_FUTURE is not the same as MLOCK_ONFAULT :) So MLOCK_CURRENT would be even more confusing. Especially if mlockall(MCL_CURRENT | MCL_FUTURE) is OK, but mlock2(MLOCK_LOCKED | MLOCK_ONFAULT) is invalid. 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. I don't want to NAK that patch if you think it's useful. ___ Linuxppc-dev mailing list Linuxppc-dev@lists.ozlabs.org https://lists.ozlabs.org/listinfo/linuxppc-dev
Re: [PATCH V5 0/7] Allow user to request memory to be locked on page fault
On 07/24/2015 11:28 PM, Eric B Munson wrote: ... 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! ___ Linuxppc-dev mailing list Linuxppc-dev@lists.ozlabs.org https://lists.ozlabs.org/listinfo/linuxppc-dev
