Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1751246AbbD2UOe (ORCPT ); Wed, 29 Apr 2015 16:14:34 -0400 Received: from g4t3425.houston.hp.com ([15.201.208.53]:57004 "EHLO g4t3425.houston.hp.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1751158AbbD2UOd (ORCPT ); Wed, 29 Apr 2015 16:14:33 -0400 Message-ID: <1430338468.2475.8.camel@j-VirtualBox> Subject: Re: [PATCH v2 3/5] sched, timer: Use atomics in thread_group_cputimer to improve scalability From: Jason Low To: Waiman Long Cc: Peter Zijlstra , Ingo Molnar , Thomas Gleixner , linux-kernel@vger.kernel.org, "Paul E. McKenney" , Andrew Morton , Oleg Nesterov , Frederic Weisbecker , Mel Gorman , Rik van Riel , Steven Rostedt , Preeti U Murthy , Mike Galbraith , Davidlohr Bueso , Aswin Chandramouleeswaran , Scott J Norton , jason.low2@hp.com Date: Wed, 29 Apr 2015 13:14:28 -0700 In-Reply-To: <5541265E.1020005@hp.com> References: <1430251224-5764-1-git-send-email-jason.low2@hp.com> <1430251224-5764-4-git-send-email-jason.low2@hp.com> <5541265E.1020005@hp.com> Content-Type: text/plain; charset="UTF-8" X-Mailer: Evolution 3.2.3-0ubuntu6 Content-Transfer-Encoding: 7bit Mime-Version: 1.0 Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org Content-Length: 2601 Lines: 64 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@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/