On Tue, 23 Apr 2019, Bueso wrote:
On Wed, 03 Apr 2019, Daniel Jordan wrote:
On Wed, Apr 03, 2019 at 06:58:45AM +0200, Christophe Leroy wrote:
Le 02/04/2019 à 22:41, Daniel Jordan a écrit :
With locked_vm now an atomic, there is no need to take mmap_sem as
writer. Delete and refactor accordingly.
Could you please detail the change ?
Ok, I'll be more specific in the next version, using some of your language in
fact. :)
It looks like this is not the only
change. I'm wondering what the consequences are.
Before we did:
- lock
- calculate future value
- check the future value is acceptable
- update value if future value acceptable
- return error if future value non acceptable
- unlock
Now we do:
- atomic update with future (possibly too high) value
- check the new value is acceptable
- atomic update back with older value if new value not acceptable and return
error
So if a concurrent action wants to increase locked_vm with an acceptable
step while another one has temporarily set it too high, it will now fail.
I think we should keep the previous approach and do a cmpxchg after
validating the new value.
Wouldn't the cmpxchg alternative also be exposed the locked_vm changing between
validating the new value and the cmpxchg() and we'd bogusly fail even when there
is still just because the value changed (I'm assuming we don't hold any locks,
otherwise all this is pointless).
current_locked = atomic_read(&mm->locked_vm);
new_locked = current_locked + npages;
if (new_locked < lock_limit)
if (cmpxchg(&mm->locked_vm, current_locked, new_locked) == current_locked)
Err, this being != of course.
