All of lore.kernel.org
 help / color / mirror / Atom feed
* [PATCH v4] locking/lock_events: Use this_cpu_add() when necessary
@ 2019-05-24 19:42 Waiman Long
  2019-05-24 21:24 ` Linus Torvalds
  2019-05-27  8:23 ` Peter Zijlstra
  0 siblings, 2 replies; 6+ messages in thread
From: Waiman Long @ 2019-05-24 19:42 UTC (permalink / raw)
  To: Peter Zijlstra, Ingo Molnar, Will Deacon, Thomas Gleixner,
	Borislav Petkov, H. Peter Anvin
  Cc: linux-kernel, x86, Davidlohr Bueso, Linus Torvalds, Tim Chen,
	huang ying, Waiman Long

The kernel test robot has reported that the use of __this_cpu_add()
causes bug messages like:

  BUG: using __this_cpu_add() in preemptible [00000000] code: ...

Given the imprecise nature of the count and the possibility of resetting
the count and doing the measurement again, this is not really a big
problem to use the unprotected __this_cpu_*() functions.

To make the preemption checking code happy, the this_cpu_*() functions
will be used if CONFIG_DEBUG_PREEMPT is defined.

The imprecise nature of the locking counts are also documented with
the suggestion that we should run the measurement a few times with the
counts reset in between to get a better picture of what is going on
under the hood.

Fixes: a8654596f0371 ("locking/rwsem: Enable lock event counting")
Suggested-by: Linus Torvalds <torvalds@linux-foundation.org>
Signed-off-by: Waiman Long <longman@redhat.com>
---
 kernel/locking/lock_events.h | 42 ++++++++++++++++++++++++++++++++++--
 1 file changed, 40 insertions(+), 2 deletions(-)

diff --git a/kernel/locking/lock_events.h b/kernel/locking/lock_events.h
index feb1acc54611..46b71af8eef2 100644
--- a/kernel/locking/lock_events.h
+++ b/kernel/locking/lock_events.h
@@ -30,13 +30,51 @@ enum lock_events {
  */
 DECLARE_PER_CPU(unsigned long, lockevents[lockevent_num]);
 
+/*
+ * The purpose of the lock event counting subsystem is to provide a low
+ * overhead way to record the number of specific locking events by using
+ * percpu counters. It is the percpu sum that matters, not specifically
+ * how many of them happens in each cpu.
+ *
+ * It is possible that the same percpu counter may be modified in both
+ * the process and interrupt contexts. For architectures that perform
+ * percpu operation with multiple instructions, it is possible to lose
+ * count if a process context percpu update is interrupted in the middle
+ * and the same counter is updated in the interrupt context. Therefore,
+ * the generated percpu sum may not be precise. The error, if any, should
+ * be small and insignificant.
+ *
+ * For those architectures that do multi-instruction percpu operation,
+ * preemption in the middle and moving the task to another cpu may cause
+ * a larger error in the count. Again, this will be few and far between.
+ * Given the imprecise nature of the count and the possibility of resetting
+ * the count and doing the measurement again, this is not really a big
+ * problem.
+ *
+ * To get a better picture of what is happening under the hood, it is
+ * suggested that a few measurements should be taken with the counts
+ * reset in between to stamp out outliner because of these possible
+ * error conditions.
+ *
+ * To minimize overhead, we use __this_cpu_*() in all cases except when
+ * CONFIG_DEBUG_PREEMPT is defined. In this particular case, this_cpu_*()
+ * will be used to avoid the appearance of unwanted BUG messages.
+ */
+#ifdef CONFIG_DEBUG_PREEMPT
+#define lockevent_percpu_inc(x)		this_cpu_inc(x)
+#define lockevent_percpu_add(x, v)	this_cpu_add(x, v)
+#else
+#define lockevent_percpu_inc(x)		__this_cpu_inc(x)
+#define lockevent_percpu_add(x, v)	__this_cpu_add(x, v)
+#endif
+
 /*
  * Increment the PV qspinlock statistical counters
  */
 static inline void __lockevent_inc(enum lock_events event, bool cond)
 {
 	if (cond)
-		__this_cpu_inc(lockevents[event]);
+		lockevent_percpu_inc(lockevents[event]);
 }
 
 #define lockevent_inc(ev)	  __lockevent_inc(LOCKEVENT_ ##ev, true)
@@ -44,7 +82,7 @@ static inline void __lockevent_inc(enum lock_events event, bool cond)
 
 static inline void __lockevent_add(enum lock_events event, int inc)
 {
-	__this_cpu_add(lockevents[event], inc);
+	lockevent_percpu_add(lockevents[event], inc);
 }
 
 #define lockevent_add(ev, c)	__lockevent_add(LOCKEVENT_ ##ev, c)
-- 
2.18.1


^ permalink raw reply	[flat|nested] 6+ messages in thread

* Re: [PATCH v4] locking/lock_events: Use this_cpu_add() when necessary
  2019-05-24 19:42 [PATCH v4] locking/lock_events: Use this_cpu_add() when necessary Waiman Long
@ 2019-05-24 21:24 ` Linus Torvalds
  2019-05-27  8:23 ` Peter Zijlstra
  1 sibling, 0 replies; 6+ messages in thread
From: Linus Torvalds @ 2019-05-24 21:24 UTC (permalink / raw)
  To: Waiman Long
  Cc: Peter Zijlstra, Ingo Molnar, Will Deacon, Thomas Gleixner,
	Borislav Petkov, H. Peter Anvin, Linux List Kernel Mailing,
	the arch/x86 maintainers, Davidlohr Bueso, Tim Chen, huang ying

On Fri, May 24, 2019 at 12:42 PM Waiman Long <longman@redhat.com> wrote:
>
> Fixes: a8654596f0371 ("locking/rwsem: Enable lock event counting")

Applied directly,

               Linus

^ permalink raw reply	[flat|nested] 6+ messages in thread

* Re: [PATCH v4] locking/lock_events: Use this_cpu_add() when necessary
  2019-05-24 19:42 [PATCH v4] locking/lock_events: Use this_cpu_add() when necessary Waiman Long
  2019-05-24 21:24 ` Linus Torvalds
@ 2019-05-27  8:23 ` Peter Zijlstra
  2019-05-27 19:33   ` Linus Torvalds
  2019-06-03 13:33   ` [tip:locking/core] locking/lock_events: Use raw_cpu_{add,inc}() for stats tip-bot for Peter Zijlstra
  1 sibling, 2 replies; 6+ messages in thread
From: Peter Zijlstra @ 2019-05-27  8:23 UTC (permalink / raw)
  To: Waiman Long
  Cc: Ingo Molnar, Will Deacon, Thomas Gleixner, Borislav Petkov,
	H. Peter Anvin, linux-kernel, x86, Davidlohr Bueso,
	Linus Torvalds, Tim Chen, huang ying

On Fri, May 24, 2019 at 03:42:22PM -0400, Waiman Long wrote:
> +#ifdef CONFIG_DEBUG_PREEMPT
> +#define lockevent_percpu_inc(x)		this_cpu_inc(x)
> +#define lockevent_percpu_add(x, v)	this_cpu_add(x, v)
> +#else
> +#define lockevent_percpu_inc(x)		__this_cpu_inc(x)
> +#define lockevent_percpu_add(x, v)	__this_cpu_add(x, v)
> +#endif

That's disguisting... I see Linus already applied it, but yuck. That's
what we have raw_cpu_*() for.

Something like the below perhaps.

---

diff --git a/kernel/locking/lock_events.h b/kernel/locking/lock_events.h
index 46b71af8eef2..8c7e7d25f09c 100644
--- a/kernel/locking/lock_events.h
+++ b/kernel/locking/lock_events.h
@@ -31,50 +31,13 @@ enum lock_events {
 DECLARE_PER_CPU(unsigned long, lockevents[lockevent_num]);
 
 /*
- * The purpose of the lock event counting subsystem is to provide a low
- * overhead way to record the number of specific locking events by using
- * percpu counters. It is the percpu sum that matters, not specifically
- * how many of them happens in each cpu.
- *
- * It is possible that the same percpu counter may be modified in both
- * the process and interrupt contexts. For architectures that perform
- * percpu operation with multiple instructions, it is possible to lose
- * count if a process context percpu update is interrupted in the middle
- * and the same counter is updated in the interrupt context. Therefore,
- * the generated percpu sum may not be precise. The error, if any, should
- * be small and insignificant.
- *
- * For those architectures that do multi-instruction percpu operation,
- * preemption in the middle and moving the task to another cpu may cause
- * a larger error in the count. Again, this will be few and far between.
- * Given the imprecise nature of the count and the possibility of resetting
- * the count and doing the measurement again, this is not really a big
- * problem.
- *
- * To get a better picture of what is happening under the hood, it is
- * suggested that a few measurements should be taken with the counts
- * reset in between to stamp out outliner because of these possible
- * error conditions.
- *
- * To minimize overhead, we use __this_cpu_*() in all cases except when
- * CONFIG_DEBUG_PREEMPT is defined. In this particular case, this_cpu_*()
- * will be used to avoid the appearance of unwanted BUG messages.
- */
-#ifdef CONFIG_DEBUG_PREEMPT
-#define lockevent_percpu_inc(x)		this_cpu_inc(x)
-#define lockevent_percpu_add(x, v)	this_cpu_add(x, v)
-#else
-#define lockevent_percpu_inc(x)		__this_cpu_inc(x)
-#define lockevent_percpu_add(x, v)	__this_cpu_add(x, v)
-#endif
-
-/*
- * Increment the PV qspinlock statistical counters
+ * Increment the statistical counters. use raw_cpu_inc() because of lower
+ * overhead and we don't care if we loose the occasional update.
  */
 static inline void __lockevent_inc(enum lock_events event, bool cond)
 {
 	if (cond)
-		lockevent_percpu_inc(lockevents[event]);
+		raw_cpu_inc(lockevents[event]);
 }
 
 #define lockevent_inc(ev)	  __lockevent_inc(LOCKEVENT_ ##ev, true)
@@ -82,7 +45,7 @@ static inline void __lockevent_inc(enum lock_events event, bool cond)
 
 static inline void __lockevent_add(enum lock_events event, int inc)
 {
-	lockevent_percpu_add(lockevents[event], inc);
+	raw_cpu_add(lockevents[event], inc);
 }
 
 #define lockevent_add(ev, c)	__lockevent_add(LOCKEVENT_ ##ev, c)


^ permalink raw reply	[flat|nested] 6+ messages in thread

* Re: [PATCH v4] locking/lock_events: Use this_cpu_add() when necessary
  2019-05-27  8:23 ` Peter Zijlstra
@ 2019-05-27 19:33   ` Linus Torvalds
  2019-05-28  8:22     ` Peter Zijlstra
  2019-06-03 13:33   ` [tip:locking/core] locking/lock_events: Use raw_cpu_{add,inc}() for stats tip-bot for Peter Zijlstra
  1 sibling, 1 reply; 6+ messages in thread
From: Linus Torvalds @ 2019-05-27 19:33 UTC (permalink / raw)
  To: Peter Zijlstra
  Cc: Waiman Long, Ingo Molnar, Will Deacon, Thomas Gleixner,
	Borislav Petkov, H. Peter Anvin, Linux List Kernel Mailing,
	the arch/x86 maintainers, Davidlohr Bueso, Tim Chen, huang ying

On Mon, May 27, 2019 at 1:23 AM Peter Zijlstra <peterz@infradead.org> wrote:
>
> That's disguisting... I see Linus already applied it, but yuck. That's
> what we have raw_cpu_*() for.

Ahh, I tried to look for that, but there was enough indirection and
confusion that I wasn't sure they were generically available.

And the "raw_cpu_*()" functions are rare enough that I'd never
encountered them enough to really be aware of them. In fact, we seem
to have exactly _one_ user of "raw_cpu_add()" in the whole kernel, and
a handful of "raw_cpu_inc()".

But ack on your patch, and a heartfelt "yeah, that's the right thing". Thanks,

                Linus

^ permalink raw reply	[flat|nested] 6+ messages in thread

* Re: [PATCH v4] locking/lock_events: Use this_cpu_add() when necessary
  2019-05-27 19:33   ` Linus Torvalds
@ 2019-05-28  8:22     ` Peter Zijlstra
  0 siblings, 0 replies; 6+ messages in thread
From: Peter Zijlstra @ 2019-05-28  8:22 UTC (permalink / raw)
  To: Linus Torvalds
  Cc: Waiman Long, Ingo Molnar, Will Deacon, Thomas Gleixner,
	Borislav Petkov, H. Peter Anvin, Linux List Kernel Mailing,
	the arch/x86 maintainers, Davidlohr Bueso, Tim Chen, huang ying

On Mon, May 27, 2019 at 12:33:56PM -0700, Linus Torvalds wrote:
> On Mon, May 27, 2019 at 1:23 AM Peter Zijlstra <peterz@infradead.org> wrote:
> >
> > That's disguisting... I see Linus already applied it, but yuck. That's
> > what we have raw_cpu_*() for.
> 
> Ahh, I tried to look for that, but there was enough indirection and
> confusion that I wasn't sure they were generically available.
> 
> And the "raw_cpu_*()" functions are rare enough that I'd never
> encountered them enough to really be aware of them. In fact, we seem
> to have exactly _one_ user of "raw_cpu_add()" in the whole kernel, and
> a handful of "raw_cpu_inc()".

Yeah, not having many is good. From a correctness PoV they're basically
always the wrong thing to use, except for this one usecase where we
prefer speed over correctness.

> But ack on your patch, and a heartfelt "yeah, that's the right thing". Thanks,

Thanks, I'll go write me a Changelog then ;-)

^ permalink raw reply	[flat|nested] 6+ messages in thread

* [tip:locking/core] locking/lock_events: Use raw_cpu_{add,inc}() for stats
  2019-05-27  8:23 ` Peter Zijlstra
  2019-05-27 19:33   ` Linus Torvalds
@ 2019-06-03 13:33   ` tip-bot for Peter Zijlstra
  1 sibling, 0 replies; 6+ messages in thread
From: tip-bot for Peter Zijlstra @ 2019-06-03 13:33 UTC (permalink / raw)
  To: linux-tip-commits
  Cc: huang.ying.caritas, hpa, tglx, mingo, longman, torvalds, bp,
	tim.c.chen, will.deacon, linux-kernel, peterz, dave

Commit-ID:  24811637dbfd07c69da7e9db586d35d17e6afca3
Gitweb:     https://git.kernel.org/tip/24811637dbfd07c69da7e9db586d35d17e6afca3
Author:     Peter Zijlstra <peterz@infradead.org>
AuthorDate: Mon, 27 May 2019 10:23:26 +0200
Committer:  Ingo Molnar <mingo@kernel.org>
CommitDate: Mon, 3 Jun 2019 12:32:56 +0200

locking/lock_events: Use raw_cpu_{add,inc}() for stats

Instead of playing silly games with CONFIG_DEBUG_PREEMPT toggling
between this_cpu_*() and __this_cpu_*() use raw_cpu_*(), which is
exactly what we want here.

Signed-off-by: Peter Zijlstra (Intel) <peterz@infradead.org>
Acked-by: Linus Torvalds <torvalds@linux-foundation.org>
Cc: Borislav Petkov <bp@alien8.de>
Cc: Davidlohr Bueso <dave@stgolabs.net>
Cc: H. Peter Anvin <hpa@zytor.com>
Cc: Peter Zijlstra <peterz@infradead.org>
Cc: Thomas Gleixner <tglx@linutronix.de>
Cc: Tim Chen <tim.c.chen@linux.intel.com>
Cc: Waiman Long <longman@redhat.com>
Cc: Will Deacon <will.deacon@arm.com>
Cc: huang ying <huang.ying.caritas@gmail.com>
Link: https://lkml.kernel.org/r/20190527082326.GP2623@hirez.programming.kicks-ass.net
Signed-off-by: Ingo Molnar <mingo@kernel.org>
---
 kernel/locking/lock_events.h | 45 ++++----------------------------------------
 1 file changed, 4 insertions(+), 41 deletions(-)

diff --git a/kernel/locking/lock_events.h b/kernel/locking/lock_events.h
index 46b71af8eef2..8c7e7d25f09c 100644
--- a/kernel/locking/lock_events.h
+++ b/kernel/locking/lock_events.h
@@ -31,50 +31,13 @@ enum lock_events {
 DECLARE_PER_CPU(unsigned long, lockevents[lockevent_num]);
 
 /*
- * The purpose of the lock event counting subsystem is to provide a low
- * overhead way to record the number of specific locking events by using
- * percpu counters. It is the percpu sum that matters, not specifically
- * how many of them happens in each cpu.
- *
- * It is possible that the same percpu counter may be modified in both
- * the process and interrupt contexts. For architectures that perform
- * percpu operation with multiple instructions, it is possible to lose
- * count if a process context percpu update is interrupted in the middle
- * and the same counter is updated in the interrupt context. Therefore,
- * the generated percpu sum may not be precise. The error, if any, should
- * be small and insignificant.
- *
- * For those architectures that do multi-instruction percpu operation,
- * preemption in the middle and moving the task to another cpu may cause
- * a larger error in the count. Again, this will be few and far between.
- * Given the imprecise nature of the count and the possibility of resetting
- * the count and doing the measurement again, this is not really a big
- * problem.
- *
- * To get a better picture of what is happening under the hood, it is
- * suggested that a few measurements should be taken with the counts
- * reset in between to stamp out outliner because of these possible
- * error conditions.
- *
- * To minimize overhead, we use __this_cpu_*() in all cases except when
- * CONFIG_DEBUG_PREEMPT is defined. In this particular case, this_cpu_*()
- * will be used to avoid the appearance of unwanted BUG messages.
- */
-#ifdef CONFIG_DEBUG_PREEMPT
-#define lockevent_percpu_inc(x)		this_cpu_inc(x)
-#define lockevent_percpu_add(x, v)	this_cpu_add(x, v)
-#else
-#define lockevent_percpu_inc(x)		__this_cpu_inc(x)
-#define lockevent_percpu_add(x, v)	__this_cpu_add(x, v)
-#endif
-
-/*
- * Increment the PV qspinlock statistical counters
+ * Increment the statistical counters. use raw_cpu_inc() because of lower
+ * overhead and we don't care if we loose the occasional update.
  */
 static inline void __lockevent_inc(enum lock_events event, bool cond)
 {
 	if (cond)
-		lockevent_percpu_inc(lockevents[event]);
+		raw_cpu_inc(lockevents[event]);
 }
 
 #define lockevent_inc(ev)	  __lockevent_inc(LOCKEVENT_ ##ev, true)
@@ -82,7 +45,7 @@ static inline void __lockevent_inc(enum lock_events event, bool cond)
 
 static inline void __lockevent_add(enum lock_events event, int inc)
 {
-	lockevent_percpu_add(lockevents[event], inc);
+	raw_cpu_add(lockevents[event], inc);
 }
 
 #define lockevent_add(ev, c)	__lockevent_add(LOCKEVENT_ ##ev, c)

^ permalink raw reply	[flat|nested] 6+ messages in thread

end of thread, other threads:[~2019-06-03 13:33 UTC | newest]

Thread overview: 6+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2019-05-24 19:42 [PATCH v4] locking/lock_events: Use this_cpu_add() when necessary Waiman Long
2019-05-24 21:24 ` Linus Torvalds
2019-05-27  8:23 ` Peter Zijlstra
2019-05-27 19:33   ` Linus Torvalds
2019-05-28  8:22     ` Peter Zijlstra
2019-06-03 13:33   ` [tip:locking/core] locking/lock_events: Use raw_cpu_{add,inc}() for stats tip-bot for Peter Zijlstra

This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.