[PATCH v3] locking/lock_events: Use this_cpu_add() when necessary

[PATCH v3] locking/lock_events: Use this_cpu_add() when necessary

[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

Re: [PATCH v3] locking/lock_events: Use this_cpu_add() when necessary

[Linus Torvalds]

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

Re: [PATCH v3] locking/lock_events: Use this_cpu_add() when necessary

[Waiman Long]

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