linux-kernel - Re: [PATCH 2.6.27-rc5] Fix itimer/many thread hang.

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] [day] [month] [year] [list]

Date:	Wed, 10 Sep 2008 10:50:58 -0700
From:	Frank Mayhar <fmayhar@...gle.com>
To:	Oleg Nesterov <oleg@...sign.ru>
Cc:	Ingo Molnar <mingo@...e.hu>, linux-kernel@...r.kernel.org,
	Roland McGrath <roland@...hat.com>,
	Thomas Gleixner <tglx@...utronix.de>,
	Alexey Dobriyan <adobriyan@...il.com>,
	Andrew Morton <akpm@...ux-foundation.org>
Subject: Re: [PATCH 2.6.27-rc5] Fix itimer/many thread hang.

On Wed, 2008-09-10 at 16:12 +0400, Oleg Nesterov wrote:
> On 09/09, Frank Mayhar wrote:
> >
> > On Tue, 2008-09-09 at 20:01 +0400, Oleg Nesterov wrote:
> >
> > > As for this particular function, it seems to me that ->signal == NULL
> > > is not possible, no?
> >
> > That's not completely clear to me.  I'm allowing for the possibility
> > that it might be called during, say, process teardown.  It's used in so
> > many places that I'm uncomfortable leaving the == NULL check out.
> 
> Please see my reply to Roland.

I saw it.  I dunno, I'm more of the "belt-and-suspenders" mindset.  I'll
add a comment, thought, that this is probably a "can't happen" but we
check it anyway.

> > > Btw, this function has a lot of callers, perhaps it is better to
> > > uninline it.
> >
> > If that's the consensus I'll do so.  I assumed that speed was more
> > important than space in this case.  Am I mistaken?
> 
> Are you sure inline will be faster? It has a lot of calllers, think
> about i-cache. And the function call is not that expensive.

Hmm.  It just seems to me that making it inline enables the optimizer to
do smarter things with the flow of control.  The routine isn't all that
long, disassembling the posix_cpu_timers_exit_group() routine gives a
good view of it and it appears to be around 120 bytes.  Certainly less
than 200 bytes (all of posix_cpu_timers_exit_group() is only 201 bytes).
That doesn't seem like a cache buster but I defer to those who are more
familiar with this stuff.

> > > So, the first CLONE_THREAD creates ->cputime.totals. After that
> > > thread_group_cputime_account_xxx() start to use it even if the task
> > > doesn't have the attached cpu timers.
> > >
> > > Stupid question: can't we allocate .totals in posix_cpu_timer_create() /
> > > set_process_cpu_timer() ?
> >
> > That was the original plan but we (that is, Roland and I) decided to
> > eliminate the separate storage for the dead-threads totals.  It's now
> > all kept in the totals field, for the whole thread group.
> 
> I see, thanks.

You're welcome.  I actually like the new method of keeping track of this
stuff; it seems cleaner and certainly removes the requirement of walking
the thread group to add it up.  Should help some microbenchmarks, at
least. :-)
-- 
Frank Mayhar <fmayhar@...gle.com>
Google, Inc.

--
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/