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

Powered by blists - more mailing lists