linux-kernel.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
From: Ingo Molnar <mingo@kernel.org>
To: Linus Torvalds <torvalds@linux-foundation.org>
Cc: "Linux Kernel Mailing List" <linux-kernel@vger.kernel.org>,
	"Frédéric Weisbecker" <fweisbec@gmail.com>,
	"Thomas Gleixner" <tglx@linutronix.de>,
	"Peter Zijlstra" <a.p.zijlstra@chello.nl>,
	"Andrew Morton" <akpm@linux-foundation.org>
Subject: [PATCH v2] atomic: Fix bugs in 'fetch_or()' and rename it to 'xchg_or()'
Date: Tue, 15 Mar 2016 13:21:45 +0100	[thread overview]
Message-ID: <20160315122145.GA7225@gmail.com> (raw)
In-Reply-To: <20160315093245.GA7943@gmail.com>

Linus noticed a couple of problems with the fetch_or() implementation introduced
by 5fd7a09cfb8c ("atomic: Export fetch_or()"):

 - Sloppy macro implementation: 'mask' and 'ptr' is evaluated multiple times,
   which will break if arguments have side effects. Also, it uses 
   double-underscore temporary variables - which is dangerous as low level asm 
   ones are using those too and they may alias in unexpected ways.

 - Sloppy semantics: the naming and structure of the macro is ambigious, with
   no well-defined semantics. It's neither an atomic nor a cmpxchg() interface,
   but still it lives in include/linux/atomic.h.

Solve this by:

 - Extracting the arguments into helper variables once, and never
   using the original arguments from that point on. This makes it
   pretty clear that there can be no unwanted macro evaluation
   side effects.

 - Using single-underscore temporary variables.

 - Renaming fetch_or() to xchg_or(), recognizing that the semantics
   are xchg()-alike.

Also propagate the rename to the call sites.

Reported-by: Linus Torvalds <torvalds@linux-foundation.org>
Cc: Andrew Morton <akpm@linux-foundation.org>
Cc: Chris Metcalf <cmetcalf@ezchip.com>
Cc: Christoph Lameter <cl@linux.com>
Cc: Frederic Weisbecker <fweisbec@gmail.com>
Cc: Luiz Capitulino <lcapitulino@redhat.com>
Cc: Peter Zijlstra <a.p.zijlstra@chello.nl>
Cc: Peter Zijlstra <peterz@infradead.org>
Cc: Rik van Riel <riel@redhat.com>
Cc: Thomas Gleixner <tglx@linutronix.de>
Cc: Viresh Kumar <viresh.kumar@linaro.org>
Link: http://lkml.kernel.org/r/20160315093245.GA7943@gmail.com
Signed-off-by: Ingo Molnar <mingo@kernel.org>
---
 include/linux/atomic.h   | 26 ++++++++++++++++----------
 kernel/sched/core.c      |  2 +-
 kernel/time/tick-sched.c |  4 ++--
 3 files changed, 19 insertions(+), 13 deletions(-)

diff --git a/include/linux/atomic.h b/include/linux/atomic.h
index 6c502cb13c95..7bc5297bcca8 100644
--- a/include/linux/atomic.h
+++ b/include/linux/atomic.h
@@ -549,22 +549,28 @@ static inline int atomic_dec_if_positive(atomic_t *v)
 #endif
 
 /**
- * fetch_or - perform *ptr |= mask and return old value of *ptr
- * @ptr: pointer to value
+ * xchg_or - perform *ptr |= mask atomically and return old value of *ptr
+ * @ptr: pointer to value (cmpxchg() compatible integer pointer type)
  * @mask: mask to OR on the value
  *
- * cmpxchg based fetch_or, macro so it works for different integer types
+ * cmpxchg() based, it's a macro so it works for different integer types.
  */
-#ifndef fetch_or
-#define fetch_or(ptr, mask)						\
-({	typeof(*(ptr)) __old, __val = *(ptr);				\
+#ifndef xchg_or
+# define xchg_or(ptr, mask)						\
+({									\
+	typeof(ptr)  _ptr  = (ptr);					\
+	typeof(mask) _mask = (mask);					\
+									\
+	typeof(*_ptr) _old, _val = *_ptr;				\
+									\
 	for (;;) {							\
-		__old = cmpxchg((ptr), __val, __val | (mask));		\
-		if (__old == __val)					\
+		_old = cmpxchg(_ptr, _val, _val | _mask);		\
+		if (_old == _val)					\
 			break;						\
-		__val = __old;						\
+		_val = _old;						\
 	}								\
-	__old;								\
+									\
+	_old;								\
 })
 #endif
 
diff --git a/kernel/sched/core.c b/kernel/sched/core.c
index e5725b931bee..bbd347c04826 100644
--- a/kernel/sched/core.c
+++ b/kernel/sched/core.c
@@ -329,7 +329,7 @@ static inline void init_hrtick(void)
 static bool set_nr_and_not_polling(struct task_struct *p)
 {
 	struct thread_info *ti = task_thread_info(p);
-	return !(fetch_or(&ti->flags, _TIF_NEED_RESCHED) & _TIF_POLLING_NRFLAG);
+	return !(xchg_or(&ti->flags, _TIF_NEED_RESCHED) & _TIF_POLLING_NRFLAG);
 }
 
 /*
diff --git a/kernel/time/tick-sched.c b/kernel/time/tick-sched.c
index 969e6704c3c9..851631899352 100644
--- a/kernel/time/tick-sched.c
+++ b/kernel/time/tick-sched.c
@@ -264,7 +264,7 @@ static void tick_nohz_dep_set_all(unsigned long *dep,
 {
 	unsigned long prev;
 
-	prev = fetch_or(dep, BIT_MASK(bit));
+	prev = xchg_or(dep, BIT_MASK(bit));
 	if (!prev)
 		tick_nohz_full_kick_all();
 }
@@ -294,7 +294,7 @@ void tick_nohz_dep_set_cpu(int cpu, enum tick_dep_bits bit)
 
 	ts = per_cpu_ptr(&tick_cpu_sched, cpu);
 
-	prev = fetch_or(&ts->tick_dep_mask, BIT_MASK(bit));
+	prev = xchg_or(&ts->tick_dep_mask, BIT_MASK(bit));
 	if (!prev) {
 		preempt_disable();
 		/* Perf needs local kick that is NMI safe */

  parent reply	other threads:[~2016-03-15 12:22 UTC|newest]

Thread overview: 27+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2016-03-14 12:32 [GIT PULL] NOHZ updates for v4.6 Ingo Molnar
2016-03-15  2:44 ` Linus Torvalds
2016-03-15  8:42   ` Peter Zijlstra
2016-03-15  9:49     ` Ingo Molnar
2016-03-15  9:32   ` [PATCH] atomic: Fix bugs in 'fetch_or()' and rename it to 'xchg_or()' Ingo Molnar
2016-03-15 10:50     ` Peter Zijlstra
2016-03-15 12:08       ` Ingo Molnar
2016-03-15 12:42         ` Peter Zijlstra
2016-03-15 11:06     ` Peter Zijlstra
2016-03-15 11:59     ` Peter Zijlstra
2016-03-15 12:01     ` Ingo Molnar
2016-03-15 12:32       ` Ingo Molnar
2016-03-15 12:37         ` Ingo Molnar
2016-03-15 13:17         ` Peter Zijlstra
2016-03-15 12:21     ` Ingo Molnar [this message]
2016-03-15 13:26       ` [PATCH v2] " Peter Zijlstra
2016-03-16  8:04         ` Ingo Molnar
2016-03-16  8:29           ` Peter Zijlstra
2016-03-15 17:08       ` Frederic Weisbecker
2016-03-16  8:14         ` Ingo Molnar
2016-03-17  0:54           ` Frederic Weisbecker
2016-03-15 16:18     ` [PATCH] " Linus Torvalds
2016-03-15  9:53   ` [PATCH] nohz: Change tick_dep_mask from 'unsigned long' to 'unsigned int' Ingo Molnar
2016-03-15 12:15     ` Ingo Molnar
2016-03-15 16:30       ` Linus Torvalds
2016-03-15 17:28         ` Frederic Weisbecker
2016-03-15 17:36           ` Linus Torvalds

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=20160315122145.GA7225@gmail.com \
    --to=mingo@kernel.org \
    --cc=a.p.zijlstra@chello.nl \
    --cc=akpm@linux-foundation.org \
    --cc=fweisbec@gmail.com \
    --cc=linux-kernel@vger.kernel.org \
    --cc=tglx@linutronix.de \
    --cc=torvalds@linux-foundation.org \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).