linux-kernel - Re: [-mm] Add an owner to the mm

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-Id: <1207249164.21922.71.camel@nimitz.home.sr71.net>
Date:	Thu, 03 Apr 2008 11:59:24 -0700
From:	Dave Hansen <dave@...ux.vnet.ibm.com>
To:	balbir@...ux.vnet.ibm.com
Cc:	Paul Menage <menage@...gle.com>,
	Pavel Emelianov <xemul@...nvz.org>,
	Hugh Dickins <hugh@...itas.com>,
	Sudhir Kumar <skumar@...ux.vnet.ibm.com>,
	YAMAMOTO Takashi <yamamoto@...inux.co.jp>, lizf@...fujitsu.com,
	linux-kernel@...r.kernel.org, taka@...inux.co.jp,
	linux-mm@...ck.org, David Rientjes <rientjes@...gle.com>,
	Andrew Morton <akpm@...ux-foundation.org>,
	KAMEZAWA Hiroyuki <kamezawa.hiroyu@...fujitsu.com>
Subject: Re: [-mm] Add an owner to the mm_struct (v7)

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

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/