LKML Archive on lore.kernel.org
help / color / mirror / Atom feed
* [PATCH v3] locking/lock_events: Use this_cpu_add() when necessary
@ 2019-05-24 18:53 Waiman Long
  2019-05-24 19:00 ` Linus Torvalds
  0 siblings, 1 reply; 3+ messages in thread
From: Waiman Long @ 2019-05-24 18:53 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: ...

This is only an issue on preempt kernel where preemption can happen in
the middle of a percpu operation. We are still using __this_cpu_*() for
!preempt kernel to avoid additional overhead in case CONFIG_PREEMPT_COUNT
is set.

 v2: Simplify the condition to just preempt or !preempt.
 v3: Document the imprecise nature of the percpu count.

Fixes: a8654596f0371 ("locking/rwsem: Enable lock event counting")
Signed-off-by: Waiman Long <longman@redhat.com>
---
 kernel/locking/lock_events.h | 32 ++++++++++++++++++++++++++++++--
 1 file changed, 30 insertions(+), 2 deletions(-)

diff --git a/kernel/locking/lock_events.h b/kernel/locking/lock_events.h
index feb1acc54611..6bcb96b7e1d3 100644
--- a/kernel/locking/lock_events.h
+++ b/kernel/locking/lock_events.h
@@ -30,13 +30,41 @@ 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.
+ *
+ * In !preempt kernel, we can just use __this_cpu_*() as preemption
+ * won't happen in the middle of the percpu operation. In preempt kernel,
+ * preemption happens in the middle of the percpu operation may produce
+ * incorrect result that can be signficant if the task is moved to a
+ * different cpu.
+ */
+#ifdef CONFIG_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 +72,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] 3+ messages in thread

* Re: [PATCH v3] locking/lock_events: Use this_cpu_add() when necessary
  2019-05-24 18:53 [PATCH v3] locking/lock_events: Use this_cpu_add() when necessary Waiman Long
@ 2019-05-24 19:00 ` Linus Torvalds
  2019-05-24 19:05   ` Waiman Long
  0 siblings, 1 reply; 3+ messages in thread
From: Linus Torvalds @ 2019-05-24 19:00 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 11:54 AM Waiman Long <longman@redhat.com> wrote:
>
>  v2: Simplify the condition to just preempt or !preempt.
>  v3: Document the imprecise nature of the percpu count.

My point was that if they are imprecise., then you shouldn't use CONFIG_PREEMPT.

Because CONFIG_PREEMPT doesn't matter, and the count is imprecise with
it or without it.

So if they are imprecise, then what matters isn't whether the
operation is atomic or not, and the real issue is avout whether it
causes that "BUG: using __this_cpu_add() in preemptible" message.

IOW, you should use the config option that matters and is relevant,
namely CONFIG_DEBUG_PREEMPT.

                 Linus

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

* Re: [PATCH v3] locking/lock_events: Use this_cpu_add() when necessary
  2019-05-24 19:00 ` Linus Torvalds
@ 2019-05-24 19:05   ` Waiman Long
  0 siblings, 0 replies; 3+ messages in thread
From: Waiman Long @ 2019-05-24 19:05 UTC (permalink / raw)
  To: Linus Torvalds
  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 5/24/19 3:00 PM, Linus Torvalds wrote:
> On Fri, May 24, 2019 at 11:54 AM Waiman Long <longman@redhat.com> wrote:
>>  v2: Simplify the condition to just preempt or !preempt.
>>  v3: Document the imprecise nature of the percpu count.
> My point was that if they are imprecise., then you shouldn't use CONFIG_PREEMPT.
>
> Because CONFIG_PREEMPT doesn't matter, and the count is imprecise with
> it or without it.
>
> So if they are imprecise, then what matters isn't whether the
> operation is atomic or not, and the real issue is avout whether it
> causes that "BUG: using __this_cpu_add() in preemptible" message.
>
> IOW, you should use the config option that matters and is relevant,
> namely CONFIG_DEBUG_PREEMPT.

Yes, that makes sense. I will update the patch again.

Thanks,
Longman


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

end of thread, other threads:[~2019-05-24 19:05 UTC | newest]

Thread overview: 3+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2019-05-24 18:53 [PATCH v3] locking/lock_events: Use this_cpu_add() when necessary Waiman Long
2019-05-24 19:00 ` Linus Torvalds
2019-05-24 19:05   ` Waiman Long

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).