Re: [PATCH v2 3/5] sched, timer: Use atomics in thread_group_cputimer to improve scalability

[Jason Low]

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@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/