linux-kernel - Re: [PATCH v2 3/5] sched, timer: Use atomics in thread_group

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]

Date:	Wed, 29 Apr 2015 13:14:28 -0700
From:	Jason Low <jason.low2@...com>
To:	Waiman Long <waiman.long@...com>
Cc:	Peter Zijlstra <peterz@...radead.org>,
	Ingo Molnar <mingo@...nel.org>,
	Thomas Gleixner <tglx@...utronix.de>,
	linux-kernel@...r.kernel.org,
	"Paul E. McKenney" <paulmck@...ux.vnet.ibm.com>,
	Andrew Morton <akpm@...ux-foundation.org>,
	Oleg Nesterov <oleg@...hat.com>,
	Frederic Weisbecker <fweisbec@...il.com>,
	Mel Gorman <mgorman@...e.de>, Rik van Riel <riel@...hat.com>,
	Steven Rostedt <rostedt@...dmis.org>,
	Preeti U Murthy <preeti@...ux.vnet.ibm.com>,
	Mike Galbraith <umgwanakikbuti@...il.com>,
	Davidlohr Bueso <dave@...olabs.net>,
	Aswin Chandramouleeswaran <aswin@...com>,
	Scott J Norton <scott.norton@...com>, jason.low2@...com
Subject: Re: [PATCH v2 3/5] sched, timer: Use atomics in
 thread_group_cputimer to improve scalability

On Wed, 2015-04-29 at 14:43 -0400, Waiman Long wrote:
> On 04/28/2015 04:00 PM, Jason Low wrote:
> >   void thread_group_cputimer(struct task_struct *tsk, struct task_cputime *times)
> >   {
> >   	struct thread_group_cputimer *cputimer =&tsk->signal->cputimer;
> >   	struct task_cputime sum;
> > -	unsigned long flags;
> >
> > -	if (!cputimer->running) {
> > +	/* Check if cputimer isn't running. This is accessed without locking. */
> > +	if (!READ_ONCE(cputimer->running)) {
> >   		/*
> >   		 * The POSIX timer interface allows for absolute time expiry
> >   		 * values through the TIMER_ABSTIME flag, therefore we have
> > -		 * to synchronize the timer to the clock every time we start
> > -		 * it.
> > +		 * to synchronize the timer to the clock every time we start it.
> >   		 */
> >   		thread_group_cputime(tsk,&sum);
> > -		raw_spin_lock_irqsave(&cputimer->lock, flags);
> > -		cputimer->running = 1;
> > -		update_gt_cputime(&cputimer->cputime,&sum);
> > -	} else
> > -		raw_spin_lock_irqsave(&cputimer->lock, flags);
> > -	*times = cputimer->cputime;
> > -	raw_spin_unlock_irqrestore(&cputimer->lock, flags);
> > +		update_gt_cputime(cputimer,&sum);
> > +
> > +		/*
> > +		 * We're setting cputimer->running without a lock. Ensure
> > +		 * this only gets written to in one operation. We set
> > +		 * running after update_gt_cputime() as a small optimization,
> > +		 * but barriers are not required because update_gt_cputime()
> > +		 * can handle concurrent updates.
> > +		 */
> > +		WRITE_ONCE(cputimer->running, 1);
> > +	}
> > +	sample_group_cputimer(times, cputimer);
> >   }
> 
> If there is a possibility that more than one thread will be running this 
> code concurrently, I think it will be safer to  use cmpxchg to set the 
> running flag:
> 
>      if (!READ_ONCE(cputimer->running) && !cmpxchg(&cputimer->running, 
> 0, 1)) {
>          ...
> 
> This will ensure that only one thread will update it.

Using cmpxchg to update the running field would be fine too, though
there isn't really much of a problem with multiple threads running this
code concurrently. The update_gt_cputime() already handles concurrent
update, and this code path gets rarely executed because we only enter it
when enabling the timer.

In that case, it might be better to to keep it the way it currently is
since I think it is a bit more readable.

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