lkml.org 
[lkml]   [2008]   [Apr]   [3]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
/
SubjectRe: [-mm] Add an owner to the mm_struct (v7)
From
Date
On Fri, 2008-04-04 at 00:21 +0530, Balbir Singh wrote:
> >> +static inline int
> >> +mm_need_new_owner(struct mm_struct *mm, struct task_struct *p)
> >> +{
> >> + int ret;
> >> +
> >> + /*
> >> + * If there are other users of the mm and the owner (us) is exiting
> >> + * we need to find a new owner to take on the responsibility.
> >> + * When we use thread groups (CLONE_THREAD), the thread group
> >> + * leader is kept around in zombie state, even after it exits.
> >> + * delay_group_leader() ensures that if the group leader is around
> >> + * we need not select a new owner.
> >> + */
> >> + ret = (mm && (atomic_read(&mm->mm_users) > 1) && (mm->owner == p) &&
> >> + !delay_group_leader(p));
> >> + return ret;
> >> +}
> >
> > Ugh. Could you please spell this out a bit more. I find that stuff
> > above really hard to read. Something like:
> >
> > if (!mm)
> > return 0;
> > if (atomic_read(&mm->mm_users) <= 1)
> > return 0;
> > if (mm->owner != p)
> > return 0;
> > if (delay_group_leader(p))
> > return 0;
> > return 1;
> >
>
> The problem with code above is 4 branch instructions and the code I have just 4
> AND operations.

Please give the compiler a little credit. Give it a try. Compile both
versions and see how different they look in the end. What you see on
your screen in C has very little to do with whether the compiler uses
branch or AND instructions.

> I don't think &&'s are so hard to read. If there is a mixture of
> operations (&&, ||) then it can get a little harder

Yup, it's just a suggestion. I think the extra parenthesis were the
hardest part for my weak little brain to parse. It's not awful or
anything, I'm just suggesting what I think is a slightly better form.

> >> +retry:
> >> + if (!mm_need_new_owner(mm, p))
> >> + return;
> >> +
> >> + rcu_read_lock();
> >> + /*
> >> + * Search in the children
> >> + */
> >> + list_for_each_entry(c, &p->children, sibling) {
> >> + if (c->mm == mm)
> >> + goto assign_new_owner;
> >> + }
> >> +
> >> + /*
> >> + * Search in the siblings
> >> + */
> >> + list_for_each_entry(c, &p->parent->children, sibling) {
> >> + if (c->mm == mm)
> >> + goto assign_new_owner;
> >> + }
> >> +
> >> + /*
> >> + * Search through everything else. We should not get
> >> + * here often
> >> + */
> >> + do_each_thread(g, c) {
> >> + if (c->mm == mm)
> >> + goto assign_new_owner;
> >> + } while_each_thread(g, c);
> >
> > What is the case in which we get here? Threading that's two deep where
> > none of the immeidate siblings or children is still alive?
> >
>
> This usually happens for cases where threads were created without CLONE_THREAD.
> We need to scan for shared mm's between processes (siblings and children scans
> have not been successful).
>
> > Have you happened to instrument this and see if it happens in practice
> > much?
> >
>
> Yes, I have. I removed the !delay_group_leader() and registered the cgroup
> mm_owner_changed callback and saw the mm->owner change.

I'm just wondering how *common* it is. It's a slow operation so perhaps
we should optimize it if it's happening all the time.

-- Dave



\
 
 \ /
  Last update: 2008-04-03 21:01    [W:0.061 / U:0.108 seconds]
©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site