linux-kernel - Re: [PATCH v2] context_tracking: remove local_irq_save from __acct_update

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] [day] [month] [year] [list]

Message-ID: <20150428135733.GB23123@twins.programming.kicks-ass.net>
Date:	Tue, 28 Apr 2015 15:57:33 +0200
From:	Peter Zijlstra <peterz@...radead.org>
To:	Rik van Riel <riel@...riel.com>
Cc:	Heiko Carstens <heiko.carstens@...ibm.com>,
	linux-kernel@...r.kernel.org,
	Andy Lutomirsky <amluto@...capital.com>,
	Frederic Weisbecker <fweisbec@...hat.com>, williams@...hat.com
Subject: Re: [PATCH v2] context_tracking: remove local_irq_save from
 __acct_update_integrals

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);
 }
 
 /**
--
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/