Re: [-mm] Add an owner to the mm_struct (v2)

[Balbir Singh <balbir@linux.vnet.ibm.com>]

Paul Menage wrote:
> On Fri, Mar 28, 2008 at 5:36 AM, Balbir Singh <balbir@linux.vnet.ibm.com> wrote:
>>  > - you hold task_lock() for the new owner (which is necessary anyway to
>>  > ensure that the new owner's mm doesn't change while you're updating
>>  > mm->owner)
>>  >
>>
>>  tsk->mm should not change unless the task is exiting or when a kernel thread
>>  does use_mm() (PF_BORROWED_MM).
>>
> 
> Or the task calls execve().
> 
>>  I see mm->owner changing when
>>
>>  1. The mm->owner exits
>>  2. At fork time for clone calls with CLONE_VM
> 

This was supposed to be deleted, ignore it

> Why would a clone() call with CLONE_VM change mm->owner? We're sharing
> with the existing owner, who is still using the mm, so we should leave
> it as it is.
> 
> I don't see what invariant you're trying to protect with
> mm->owner_lock. Can you explain it?
> 

mm->owner_lock is there to protect mm->owner field from changing simultaneously
as tasks fork/exit.

>>  >>  @@ -1357,6 +1379,10 @@ static struct task_struct *copy_process(
>>  >>         write_unlock_irq(&tasklist_lock);
>>  >>         proc_fork_connector(p);
>>  >>         cgroup_post_fork(p);
>>  >>  +
>>  >>  +       if (!(clone_flags & CLONE_VM) && (p != p->group_leader))
>>  >>  +               mm_fork_init_owner(p);
>>  >>  +
>>  >
>>  > I'm not sure I understand what this is doing.
>>  >
>>  > I read it as "if p has its own mm and p is a child thread, set
>>  > p->mm->owner to p->group_leader". But by definition if p has its own
>>  > mm, then p->group_leader->mm will be different to p->mm, therefore
>>  > we'd end up with mm->owner->mm != mm, which seems very bad.
>>  >
>>  > What's the intention of this bit of code?
>>  >
>>
>>  The intention is to handle the case when clone is called without CLONE_VM and
>>  with CLONE_THREAD. This means that p can have it's own mm and a shared group_leader.
>>
> 
> In which case p should be the owner of its new mm, not
> p->group_leader. mm_fork_init_owner() does:
> 

Oh! yes.. my bad again. The check should have been p == p->thread_group, but
that is not required either. The check should now ideally be

if (!(clone_flags & CLONE_VM))

>>  +       if (mm->owner != p)
>>  +               rcu_assign_pointer(mm->owner, p->group_leader);
> 
> Since we just created a fresh mm for p in copy_mm(), and set mm->owner
> = p, I don't see how this test can succeed. And if it does, then we
> end up setting p->mm->owner to p->group_leader, which has to be bad
> since we know that:
> 
> - p is the only user of its mm
> - p != p->group_leader
> 
> therefore we're setting p->mm->owner to point to a process that
> doesn't use p->mm. What am I missing?
> 
>>  >>  +/*
>>  >>  + * Task p is exiting and it owned p, so lets find a new owner for it
>>  >>  + */
>>  >>  +static inline int
>>  >>  +mm_need_new_owner(struct mm_struct *mm, struct task_struct *p)
>>  >>  +{
>>  >>  +       int ret;
>>  >>  +
>>  >>  +       rcu_read_lock();
>>  >>  +       ret = (mm && (rcu_dereference(mm->owner) == p) &&
>>  >>  +               (atomic_read(&mm->mm_users) > 1));
>>  >>  +       rcu_read_unlock();
>>  >>  +       return ret;
>>  >
>>  > The only way that rcu_read_lock() helps here is if mm freeing is
>>  > protected by RCU, which I don't think is the case.
>>  >
>>
>>  rcu_read_lock() also ensures that preemption does not cause us to see incorrect
>>  values.
>>
> 
> The only thing that RCU is protecting us against here is that if
> mm->owner is non-NULL, the task it points to won't be freed until the
> end of our RCU read section. But since we never dereference mm->owner
> in the RCU read section, that buys us nothing and is just confusing.
> 
>>  > But as long as p==current, there's no race, since no other process
>>  > will re-point mm->owner at themselves, so mm can't go away anyway
>>  > since we have a reference to it that we're going to be dropping soon.
>>  >
>>
>>  mm cannot go away, but mm->owner can be different from current and could be
>>  going away.
> 
> But that's a problem for mm->owner, not for current.
> 
> By this point we've already cleared current->mm, so we're no longer a
> candidate for becoming mm->owner if we're not already the owner.
> Therefore the truth or falsity of (mm->owner == current) can't be
> changed by races - either we are already the owner and no-one but us
> can change mm->owner, or we're not the owner and no one can point
> mm->owner at us.
> 
>>  >>  +
>>  >>  +       if (!mm_need_new_owner(mm, p))
>>  >>  +               return;
>>  >>  +
>>  >>  +       /*
>>  >>  +        * Search in the children
>>  >>  +        */
>>  >>  +       list_for_each_entry(c, &p->children, sibling) {
>>  >>  +               if (c->mm == p->mm)
> 
> Oh, and at the point in exit_mm() when you call
> mm_update_next_owner(), p->mm is NULL, so this will only match against
> tasks that have no mm.
> 

Yes.. I think we need to call it earlier.

>>  >>  +                       goto assign_new_owner;
>>  >>  +       }
>>  >
>>  > We need to keep checking mm_need_new_owner() since it can become false
>>  > if the only other user of the mm exits at the same time that we do.
>>  > (In which case there's nothing to do).
>>  >
>>
>>  I would rather deal with the case where mm->owner is NULL, rather than keep
>>  checking
> 
> I mean that it's possible that there's one other task using mm at the
> point when we enter mm_update_next_owner(), but while we're trying to
> find it, it does an exit() or execve() and stops being a user of mm.
> if that happens, we should bail from the search since we no longer
> need to find another user to pass it to (and there is no other user to
> find).
> 
>> (since even with constant checking we cannot guarantee that mm->owner
>>  will not become NULL)
>>
>>
>>  >>  +        * Search through everything else. We should not get
>>  >>  +        * here often
>>  >>  +        */
>>  >>  +       for_each_process(c) {
>>  >>  +               g = c;
>>  >>  +               do {
>>  >>  +                       if (c->mm && (c->mm == p->mm))
>>  >>  +                                       goto assign_new_owner;
>>  >>  +               } while ((c = next_thread(c)) != g);
>>  >>  +       }
>>  >
>>  > Is there a reason to not code this as for_each_thread?
>>  >
>>
>>  Is there a for_each_thread()?
>>
> 
> Sorry, I meant do_each_thread()/while_each_thread(). Isn't that
> basically the same thing as what you've explicitly coded here?
> 

Yes, it is.

>>  >>  +assign_new_owner:
>>  >>  +       spin_lock(&mm->owner_lock);
>>  >>  +       rcu_assign_pointer(mm->owner, c);
>>  >>  +       spin_unlock(&mm->owner_lock);
>>  >>  +}
>>  >
>>  > This can break if c is also exiting and has passed the call to
>>  > mm_update_next_owner() by the time we assign mm->owner. That's why my
>>  > original suggested version had a function like:
>>  >
>>
>>  Won't it better to check for c->flags & PF_EXITING?
>>
> 
> Or if c is execing and gives up the mm that way.
> 



>>  > static inline void try_give_mm_ownership(struct task_struct *task,
>>  > struct mm_struct *mm) {
>>  >   if (task->mm != mm) return;
>>  >   task_lock(task);
>>  >   if (task->mm == mm) {
>>  >     mm->owner = task;
>>  >   }
>>  >   task_unlock(task);
>>  > }
>>  >
>>  > i.e. determining that a task is a valid candidate and updating the
>>  > owner pointer has to be done in the same critical section.
>>  >
>>
>>  Let me try and revamp the locking rules and see what that leads to. But, I don't
>>  like protecting an mm_struct's member with a task_struct's lock.
> 
> We're not protecting mm->owner with task_lock(new_owner). As I said,
> the locking rule for mm->owner should that it can only be changed if
> (current==mm->owner). What we're protecting is new_owner->mm - i.e.
> we're ensuring that we don't break the invariant that (mm->owner->mm
> == mm)
> 

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.


>>  >>   config CGROUP_MEM_RES_CTLR
>>  >>         bool "Memory Resource Controller for Control Groups"
>>  >>  -       depends on CGROUPS && RESOURCE_COUNTERS
>>  >>  +       depends on CGROUPS && RESOURCE_COUNTERS && MM_OWNER
>>  >
>>  > Maybe this should select MM_OWNER rather than depending on it?
>>  >
>>
>>  I thought of it, but wondered if the user should make an informed choice about
>>  MM_OWNER and the overhead it brings along.
> 
> I would vote the other way on that - make it clear in the memory
> controller config help that the memory controller can introduce
> overhead, and have it implicitly enable MM_OWNER.
> 

That's a possibility

Thanks for your patience and review.

-- 
	Warm Regards,
	Balbir Singh
	Linux Technology Center
	IBM, ISTL