Messages in this thread | | | Date | Sat, 29 Mar 2008 11:16:02 +0530 | From | Balbir Singh <> | Subject | Re: [-mm] Add an owner to the mm_struct (v2) |
| |
Paul Menage wrote: > Hi Balbir, > > Could you send out the latest version of your patch? I suspect it's > changed a bit based on on this review and it would be good to make > sure we're both on the same page. > > On Fri, Mar 28, 2008 at 11:10 AM, Balbir Singh > <balbir@linux.vnet.ibm.com> wrote: >> At fork time, we can have do_fork() run in parallel and we need to protect >> mm->owner, if several threads are created at the same time. We don't want to >> overwrite mm->owner for each thread that is created. > > Why would you want to overwrite mm->owner for any of the threads? If > they're sharing an existing mm, then that mm must already have an > owner, so no need to update it. >
Yes, that's why we have the lock and check.
>> > No, I think we need to call it later - after we've cleared current->mm >> > (from within task_lock(current)) - so we can't rely on p->mm in this >> > function, we have to pass it in. If we call it before while >> > current->mm == mm, then we risk a race where the (new or existing) >> > owner exits and passes it back to us *after* we've done a check to see >> > if we need to find a new owner. If we ensure that current->mm != mm >> > before we call mm_update_next_owner(), then we know we're not a >> > candidate for receiving the ownership if we don't have it already. >> > >> >> Yes and we could also check for flags & PF_EXITING >> > > A couple of issues with that: > > - I'm not sure how that handles the exec case > > - assume two users; the owner exits and wants to pass the ownership to > the other user. It finds it, but sees that it's PF_EXITING. What > should it do? If it waits for that other user to exit, it could take a > long time (e.g. core dumps can take many seconds). If it exits > immediately, then it will leave mm->owner pointing to an invalid task. > If it passes ownership to the other task, it might pass it after the > other task had done its mm_update_next_owner() check, which would be > too late. > > - assume three users; the owner exits and wants to pass the ownership > to one of the other two users. it searches through the candidates and > finds one of the other users, which is in PF_EXITING, so it skips it. > Just after this it sees that the user count has fallen to two users. > How does it know whether the user that dropped the count was the > PF_EXITING process that it saw previously (in which case it should > keep searching) or the third user that it's not encountered yet (in > which case it's not going to find the other user anywhere in its > search). > >> >> But there is no way to guarantee that, what is the new_owner exec's after we've >> >> done the check and assigned. Won't we end up breaking the invariant? How about >> >> we have mm_update_new_owner() call in exec_mmap() as well? That way, we can >> >> still use owner_lock and keep the invariant. >> >> >> > >> > Oops, I thought that exit_mm() already got called in the execve() >> > path, but you're right, it doesn't. >> > >> > Yes, exit_mmap() should call mm_update_next_owner() after the call to >> > task_unlock(), i.e. after it's set its new mm. >> > >> > So I need to express the invariant more carefully. >> > >> > What we need to preserve is that, for every mm at all times, mm->owner >> > points to a valid task. So either: >> > >> > 1) mm->owner->mm == mm AND mm->owner will check to see whether it >> > needs to pass ownership before it exits or execs. >> > >> > OR >> > >> > 2) mm->owner is the last user of mm and is about to free mm. >> > >> > OR >> > >> > 3) mm->owner is currently searching for another user of mm to pass the >> > ownership to. >> > >> > In order to get from state 3 to state 1 safely we have to hold >> > task_lock(new_owner). Otherwise we can race with an exit or exec in >> > new_owner, resulting in a process that has already passed the point of >> > checking current->mm->owner. >> > >> >> No.. like you said if we do it after current->mm has changed and is different >> from mm, then it's safe to find a new owner. I still don't see why we need >> task_lock(new_owner). > > How about the following sequence: A is old owner, B is new owner > > A gets to the task_unlock() in exit_mm(): A->mm is now NULL, mm->owner == A > B starts to execve() > A calls mm_update_next_owner() > B gets to the "active_mm = tsk->active_mm" in exec_mmap() > A finds that B->mm == mm > B continues through the critical section, gets past the point where it > needs to check for ownership > A sets mm->owner = B > B finishes its exec, and carries on with its new mmap >
OK, a task changing the mm from underneath us can be a problem. Let me move over to using task_lock(). I wish there was a simpler solution for implementing mm->owner, but the fact that CLONE_THREAD and CLONE_VM can be called independently is a huge bottleneck.
> >> Even if we have task_lock(new_owner), it can still exit or >> exec later. > > Yes, but once we've set mm->owner to the other task and released its > task_lock, the new owner is responsible for handing off the mm to yet > another owner if necessary. > >> Why mix task_lock() to protect mm->owner? > > We're not protecting mm->owner - we're protecting new_owner->mm >
Yes
> Paul
-- Warm Regards, Balbir Singh Linux Technology Center IBM, ISTL
| |