From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1030937AbbD1N5u (ORCPT ); Tue, 28 Apr 2015 09:57:50 -0400 Received: from bombadil.infradead.org ([198.137.202.9]:35754 "EHLO bombadil.infradead.org" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1030292AbbD1N5s (ORCPT ); Tue, 28 Apr 2015 09:57:48 -0400 Date: Tue, 28 Apr 2015 15:57:33 +0200 From: Peter Zijlstra To: Rik van Riel Cc: Heiko Carstens , linux-kernel@vger.kernel.org, Andy Lutomirsky , Frederic Weisbecker , williams@redhat.com Subject: Re: [PATCH v2] context_tracking: remove local_irq_save from __acct_update_integrals Message-ID: <20150428135733.GB23123@twins.programming.kicks-ass.net> References: <20150424111653.2a87a103@annuminas.surriel.com> <20150425094346.GA5897@osiris> <553B8DA9.3060600@surriel.com> <20150427111842.GA4491@osiris> <553F82BE.7050808@surriel.com> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: <553F82BE.7050808@surriel.com> User-Agent: Mutt/1.5.21 (2012-12-30) Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org On Tue, Apr 28, 2015 at 08:53:18AM -0400, Rik van Riel wrote: > So what can I do to move forward with this patch? > > It speeds up syscall entry / exit by 7% when nohz_full > is enabled on a CPU... > > Should I have the irq block compiled in only when > sizeof(cputime_t) > sizeof(long) ? So I'm not sure how the IRQ disable even works today (as you write in the Changelog), it appears racy. At which point you already have the problem of load/store tearing. So either you (the pural) need to go fix that, or we should collectively say sod it and just do your patch. That said, your patch would probably want a WRITE_ONCE() around the assignment to acct_timexpd; ensuring the read happens in a single load doesn't really help if then the store is done in two :-) Also, we should probably reduce indent in that function, something like the below. --- kernel/tsacct.c | 34 +++++++++++++++++----------------- 1 file changed, 17 insertions(+), 17 deletions(-) diff --git a/kernel/tsacct.c b/kernel/tsacct.c index 975cb49e32bf..9e225425bc3a 100644 --- a/kernel/tsacct.c +++ b/kernel/tsacct.c @@ -123,27 +123,27 @@ void xacct_add_tsk(struct taskstats *stats, struct task_struct *p) static void __acct_update_integrals(struct task_struct *tsk, cputime_t utime, cputime_t stime) { - if (likely(tsk->mm)) { - cputime_t time, dtime; - struct timeval value; - unsigned long flags; - u64 delta; - - local_irq_save(flags); - time = stime + utime; - dtime = time - tsk->acct_timexpd; - jiffies_to_timeval(cputime_to_jiffies(dtime), &value); - delta = value.tv_sec; - delta = delta * USEC_PER_SEC + value.tv_usec; - - if (delta == 0) - goto out; + cputime_t time, dtime; + struct timeval value; + unsigned long flags; + u64 delta; + + if (unlikely(!tsk->mm)) + return; + + local_irq_save(flags); + time = stime + utime; + dtime = time - tsk->acct_timexpd; + jiffies_to_timeval(cputime_to_jiffies(dtime), &value); + delta = value.tv_sec; + delta = delta * USEC_PER_SEC + value.tv_usec; + + if (delta) { tsk->acct_timexpd = time; tsk->acct_rss_mem1 += delta * get_mm_rss(tsk->mm); tsk->acct_vm_mem1 += delta * tsk->mm->total_vm; - out: - local_irq_restore(flags); } + local_irq_restore(flags); } /**