linux-kernel - Re: [PATCH 3/3] timer: Reduce unnecessary sighand lock contention

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: <1440707390.32300.100.camel@j-VirtualBox>
Date:	Thu, 27 Aug 2015 13:29:50 -0700
From:	Jason Low <jason.low2@...com>
To:	Frederic Weisbecker <fweisbec@...il.com>
Cc:	Peter Zijlstra <peterz@...radead.org>,
	Ingo Molnar <mingo@...nel.org>,
	Thomas Gleixner <tglx@...utronix.de>,
	Oleg Nesterov <oleg@...hat.com>,
	"Paul E. McKenney" <paulmck@...ux.vnet.ibm.com>,
	linux-kernel@...r.kernel.org,
	Linus Torvalds <torvalds@...ux-foundation.org>,
	Davidlohr Bueso <dave@...olabs.net>,
	Steven Rostedt <rostedt@...dmis.org>,
	Andrew Morton <akpm@...ux-foundation.org>,
	Terry Rudd <terry.rudd@...com>, Rik van Riel <riel@...hat.com>,
	Scott J Norton <scott.norton@...com>, jason.low2@...com
Subject: Re: [PATCH 3/3] timer: Reduce unnecessary sighand lock contention

On Thu, 2015-08-27 at 14:53 +0200, Frederic Weisbecker wrote:
> On Wed, Aug 26, 2015 at 04:32:34PM -0700, Jason Low wrote:
> > On Thu, 2015-08-27 at 00:56 +0200, Frederic Weisbecker wrote:
> > > On Tue, Aug 25, 2015 at 08:17:48PM -0700, Jason Low wrote:
> > > > It was found while running a database workload on large systems that
> > > > significant time was spent trying to acquire the sighand lock.
> > > > 
> > > > The issue was that whenever an itimer expired, many threads ended up
> > > > simultaneously trying to send the signal. Most of the time, nothing
> > > > happened after acquiring the sighand lock because another thread
> > > > had already sent the signal and updated the "next expire" time. The
> > > > fastpath_timer_check() didn't help much since the "next expire" time
> > > > was updated later.
> > > >  
> > > > This patch addresses this by having the thread_group_cputimer structure
> > > > maintain a boolean to signify when a thread in the group is already
> > > > checking for process wide timers, and adds extra logic in the fastpath
> > > > to check the boolean.
> > > > 
> > > > Signed-off-by: Jason Low <jason.low2@...com>
> > > > ---
> > > >  include/linux/init_task.h      |    1 +
> > > >  include/linux/sched.h          |    3 +++
> > > >  kernel/time/posix-cpu-timers.c |   19 +++++++++++++++++--
> > > >  3 files changed, 21 insertions(+), 2 deletions(-)
> > > > 
> > > > diff --git a/include/linux/init_task.h b/include/linux/init_task.h
> > > > index d0b380e..3350c77 100644
> > > > --- a/include/linux/init_task.h
> > > > +++ b/include/linux/init_task.h
> > > > @@ -53,6 +53,7 @@ extern struct fs_struct init_fs;
> > > >  	.cputimer	= { 						\
> > > >  		.cputime_atomic	= INIT_CPUTIME_ATOMIC,			\
> > > >  		.running	= 0,					\
> > > > +		.checking_timer	= 0,					\
> > > >  	},								\
> > > >  	INIT_PREV_CPUTIME(sig)						\
> > > >  	.cred_guard_mutex =						\
> > > > diff --git a/include/linux/sched.h b/include/linux/sched.h
> > > > index 119823d..a6c8334 100644
> > > > --- a/include/linux/sched.h
> > > > +++ b/include/linux/sched.h
> > > > @@ -619,6 +619,8 @@ struct task_cputime_atomic {
> > > >   * @cputime_atomic:	atomic thread group interval timers.
> > > >   * @running:		non-zero when there are timers running and
> > > >   * 			@cputime receives updates.
> > > > + * @checking_timer:	non-zero when a thread is in the process of
> > > > + *			checking for thread group timers.
> > > >   *
> > > >   * This structure contains the version of task_cputime, above, that is
> > > >   * used for thread group CPU timer calculations.
> > > > @@ -626,6 +628,7 @@ struct task_cputime_atomic {
> > > >  struct thread_group_cputimer {
> > > >  	struct task_cputime_atomic cputime_atomic;
> > > >  	int running;
> > > > +	int checking_timer;
> > > 
> > > How about a flag in the "running" field instead?
> > > 
> > > 1) Space in signal_struct is not as important as in task_strut but it
> > >    still matters.
> > 
> > George Spelvin suggested that we convert them to booleans which would
> > make them take up 2 bytes.
> > 
> > > 2) We already read the "running" field locklessly. Adding a new field like
> > >    checking_timer gets even more complicated. Ideally there should be at
> > >    least a paired memory barrier between both. Let's just simplify that
> > >    with a single field.
> > 
> > hmmm, so having 1 "flag" where we access bits for the "running" and
> > "checking_timer"?
> 
> Sure, like:
> 
> #define CPUTIMER_RUNNING 0x1
> #define CPUTIMER_CHECKING 0x2
> 
> struct thread_group_cputimer {
>     struct task_cputime_atomic cputime_atomic;
>     int status;
> }
> 
> So from cputimer_running() you just need to check:
> 
>      if (cputimer->status & CPUTIMER_RUNNING)
> 
> And from run_posix_cpu_timer() fast-path:
> 
>      if (cputimer->status == CPUTIMER_RUNNING)
> 
> so that ignores CPUTIMER_CHECKING case.

Right, having just 1 "status" field can simply things a bit. The
(cputimer->status == CPUTIMER_RUNNING) check does appear misleading
though, since we're technically not only checking for if the "cputimer
is running".

Maybe something like:

int status = cputimer->status;
if ((status & CPUTIMER_RUNNING) && !(status & CPUTIMER_CHECKING))

makes it more obvious what's going on here.

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