[PATCH] seqlock,lockdep: Only check for preemption_disabled in non-rt

[PATCH] seqlock,lockdep: Only check for preemption_disabled in non-rt

[Davidlohr Bueso]

This silences the writer hitting this nonsensical warning on PREEMPT_RT.

Reported-by: Shung-Hsi Yu <shung-hsi.yu@suse.com>
Signed-off-by: Davidlohr Bueso <dbueso@suse.de>
---
 include/linux/seqlock.h | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/include/linux/seqlock.h b/include/linux/seqlock.h
index f61e34fbaaea..c8f9253f1a2f 100644
--- a/include/linux/seqlock.h
+++ b/include/linux/seqlock.h
@@ -268,7 +268,9 @@ static inline bool __seqprop_preemptible(const seqcount_t *s)
 
 static inline void __seqprop_assert(const seqcount_t *s)
 {
+#ifndef CONFIG_PREEMPT_RT
 	lockdep_assert_preemption_disabled();
+#endif
 }
 
 #define __SEQ_RT	IS_ENABLED(CONFIG_PREEMPT_RT)
-- 
2.26.2

[PATCH v2] seqlock,lockdep: Only check for preemption_disabled in non-rt

[Davidlohr Bueso]

This silences the writer hitting this nonsensical warning on PREEMPT_RT.

Reported-by: Shung-Hsi Yu <shung-hsi.yu@suse.com>
Signed-off-by: Davidlohr Bueso <dbueso@suse.de>
---

v2: Resending because I had left out some small comments I had meant to add.

 include/linux/seqlock.h | 4 ++++
 1 file changed, 4 insertions(+)

diff --git a/include/linux/seqlock.h b/include/linux/seqlock.h
index f61e34fbaaea..2ce3e1efc9a9 100644
--- a/include/linux/seqlock.h
+++ b/include/linux/seqlock.h
@@ -268,7 +268,9 @@ static inline bool __seqprop_preemptible(const seqcount_t *s)
 
 static inline void __seqprop_assert(const seqcount_t *s)
 {
+#ifndef CONFIG_PREEMPT_RT
 	lockdep_assert_preemption_disabled();
+#endif
 }
 
 #define __SEQ_RT	IS_ENABLED(CONFIG_PREEMPT_RT)
@@ -529,6 +531,8 @@ static inline void do_write_seqcount_begin_nested(seqcount_t *s, int subclass)
  * only if the seqcount write serialization lock is associated, and
  * preemptible.  If readers can be invoked from hardirq or softirq
  * context, interrupts or bottom halves must be respectively disabled.
+ * The PREEMPT_RT case relies on the reader serializing via LOCK+UNLOCK,
+ * so the context is preemptible.
  */
 #define write_seqcount_begin(s)						\
 do {									\
-- 
2.26.2

Re: [PATCH v2] seqlock,lockdep: Only check for preemption_disabled in non-rt

[Peter Zijlstra]

On Fri, May 07, 2021 at 04:47:13PM -0700, Davidlohr Bueso wrote:
> This silences the writer hitting this nonsensical warning on PREEMPT_RT.
> 
> Reported-by: Shung-Hsi Yu <shung-hsi.yu@suse.com>
> Signed-off-by: Davidlohr Bueso <dbueso@suse.de>
> ---
> 
> v2: Resending because I had left out some small comments I had meant to add.
> 
>  include/linux/seqlock.h | 4 ++++
>  1 file changed, 4 insertions(+)
> 
> diff --git a/include/linux/seqlock.h b/include/linux/seqlock.h
> index f61e34fbaaea..2ce3e1efc9a9 100644
> --- a/include/linux/seqlock.h
> +++ b/include/linux/seqlock.h
> @@ -268,7 +268,9 @@ static inline bool __seqprop_preemptible(const seqcount_t *s)
>  
>  static inline void __seqprop_assert(const seqcount_t *s)
>  {
> +#ifndef CONFIG_PREEMPT_RT
>  	lockdep_assert_preemption_disabled();
> +#endif
>  }
>  
>  #define __SEQ_RT	IS_ENABLED(CONFIG_PREEMPT_RT)
> @@ -529,6 +531,8 @@ static inline void do_write_seqcount_begin_nested(seqcount_t *s, int subclass)
>   * only if the seqcount write serialization lock is associated, and
>   * preemptible.  If readers can be invoked from hardirq or softirq
>   * context, interrupts or bottom halves must be respectively disabled.
> + * The PREEMPT_RT case relies on the reader serializing via LOCK+UNLOCK,
> + * so the context is preemptible.
>   */

I'm confused, and the Changelog is useless. The code you actually
changed is for seqcount_t, which doesn't have an associated LOCK. If
there is a lock, the code should be changed to use the appropriate
seqcount_LOCKNAME_t and the assertion will change into the one found in
__seqprop_##lockname##_assert(), namely:

  lockdep_assert_held(lockmember)


But as is, seqcount_t usage relies on being non-preemptible, even for
PREEMPT_RT, and this is a good thing. Please describe the site that goes
boom and explain things..

Re: [PATCH v2] seqlock,lockdep: Only check for preemption_disabled in non-rt

[Davidlohr Bueso]

On Wed, 12 May 2021, Peter Zijlstra wrote:

>I'm confused, and the Changelog is useless. The code you actually
>changed is for seqcount_t, which doesn't have an associated LOCK. If

Hmm it was never my intention to touch seqcount_t, I now see the error of
my ways.

>there is a lock, the code should be changed to use the appropriate
>seqcount_LOCKNAME_t and the assertion will change into the one found in
>__seqprop_##lockname##_assert(), namely:
>
>  lockdep_assert_held(lockmember)
>
>
>But as is, seqcount_t usage relies on being non-preemptible, even for
>PREEMPT_RT, and this is a good thing. Please describe the site that goes
>boom and explain things..

So the splat is:
     WARNING: CPU: 0 PID: 15 at kernel/locking/lockdep.c:5363 lockdep_assert_preemption_disabled+0x7a/0xa0
     CPU: 0 PID: 15 Comm: kworker/0:1 Tainted: G            E      5.3.18-rt_syzkaller #1
     Workqueue: events xfrm_hash_resize
     RIP: 0010:lockdep_assert_preemption_disabled+0x7a/0xa0
     Code: 09 00 00 48 b8 00 00 00 00 00 fc ff df 48 89 fa 48 c1 ea 03 0f b6 04 02 84 c0 74 04 3c 03 7e 1c 8b 83 c8 09 00 00 85 c0 74 02 <0f> 0b 5b c3 48 c7 c7 54 39 ce 83 e8 c6 0d 43 00 eb 9f e8 bf 0d 43
     RSP: 0018:ffff888118497ca0 EFLAGS: 00010202
     RAX: 0000000000000001 RBX: ffff88811847ce40 RCX: 1ffffffff079c72a
     RDX: 1ffff1102308fb01 RSI: 0000000000000022 RDI: ffff88811847d808
     RBP: ffffffff83b9ebb0 R08: 0000000000000001 R09: ffff888118497bd8
     R10: ffff888118497c47 R11: 0000000000000001 R12: ffff88811b232200
     R13: ffff888118497dc0 R14: 0000000000000010 R15: ffff88811847ce40
      xfrm_hash_resize+0xd7/0x1490
      process_one_work+0x78e/0x16e0
      ? pwq_dec_nr_in_flight+0x2e0/0x2e0
      ? do_raw_spin_lock+0x11a/0x250
      ? _raw_spin_lock_irq+0xa/0x40
      worker_thread+0x5f5/0x1080
      ? process_one_work+0x16e0/0x16e0
      kthread+0x401/0x4f0
      ? __kthread_parkme+0x290/0x290
      ret_from_fork+0x24/0x30

I was initially chasing (and hence why the preemption check wasn't making sense):

	seqcount_mutex_init(&xfrm_policy_hash_generation, &hash_resize_mutex);

But there are actually two xfrm_hash_resize() calls (*sigh*). And the other
one, the right one, is/was indeed seqcount_t xfrm_state_hash_generation:

xfrm_hash_resize() // kworker callback, task context
     spin_lock_bh(&net->xfrm.xfrm_policy_lock); // disables softirq, preemption still enabled
     write_seqcount_begin(&xfrm_state_hash_generation);
	__seqprop_assert() <-- boom

And therefore converting it to an associated spinlock would avoid the preemption
check, which is exactly what Ahmed has already done:

bc8e0adff34 (net: xfrm: Use sequence counter with associated spinlock)
e88add19f68 (net: xfrm: Localize sequence counter per network namespace)

Sorry for the noise.

Thanks,
Davidlohr

Re: [PATCH v2] seqlock,lockdep: Only check for preemption_disabled in non-rt

[Ahmed S. Darwish]

On Thu, May 13, 2021, Davidlohr Bueso wrote:
>
> And therefore converting it to an associated spinlock would avoid the
> preemption check, which is exactly what Ahmed has already done:
>
> bc8e0adff34 (net: xfrm: Use sequence counter with associated spinlock)
> e88add19f68 (net: xfrm: Localize sequence counter per network namespace)
>
> Sorry for the noise.
>

Exactly, so it seems everything is good on your side :)

(The pending patch queue I mentioned is much larger and gets rid of the
 main packet scheduling sequence counter Qdisc::running, but I'm
 brushing it up, then sending it for an internal review round, first.
 There are already some workarounds in the RT tree for that one until
 the correct fix is merged mainline.)

Kind regards,

--
Ahmed S. Darwish
Linutronix GmbH

Re: [PATCH] seqlock,lockdep: Only check for preemption_disabled in non-rt

[Ahmed S. Darwish]

On Fri, May 07, 2021, Davidlohr Bueso wrote:
> This silences the writer hitting this nonsensical warning on PREEMPT_RT.
>
> Reported-by: Shung-Hsi Yu <shung-hsi.yu@suse.com>
> Signed-off-by: Davidlohr Bueso <dbueso@suse.de>
> ---
>  include/linux/seqlock.h | 2 ++
>  1 file changed, 2 insertions(+)
>
> diff --git a/include/linux/seqlock.h b/include/linux/seqlock.h
> index f61e34fbaaea..c8f9253f1a2f 100644
> --- a/include/linux/seqlock.h
> +++ b/include/linux/seqlock.h
> @@ -268,7 +268,9 @@ static inline bool __seqprop_preemptible(const seqcount_t *s)
>
>  static inline void __seqprop_assert(const seqcount_t *s)
>  {
> +#ifndef CONFIG_PREEMPT_RT
>  	lockdep_assert_preemption_disabled();
> +#endif
>  }
>

Nope, it is more complicated than that.

In general, for RT, seqcount_LOCKNAME_t variants should be used instead
of plain seqcount_t, as they can be safely used while preemption is
enabled on the write side.

For plain seqcount_t (which __seqprop_assert() is about), preemption
must be disabled, even for PREEMPT_RT. So the patch above is invalid.

Now, there are still some call sites in the kernel which needs
conversion obviously. I have a large patch series in queue which convert
a number of remaining networking call sites (the changes are locking
algorithm changes, not just direct substitution).

Good luck,

--
Ahmed S. Darwish
Linutronix GmbH