Re: [PATCH RT 1/5] net: Properly annotate the try-lock for the seqlock
On 2020-11-15 05:52:33 [+0100], Mike Galbraith wrote: > On Sat, 2020-11-14 at 13:24 -0600, Tom Zanussi wrote: > > On Sat, 2020-11-14 at 20:00 +0100, Mike Galbraith wrote: > > > > > __raw_write_seqcount_end() is an integral part of write_sequnlock(), > > > but we do seem to be missing a seqcount_release() in 5.4-rt. > > > > > > > Yep, you're right, it's just the missing seqcount_release() - I'll > > resubmit with just that. > > Or just drop the backport, since it adds annotation, while the original > was fixing existing annotation. > > __raw_write_seqcount_begin() called in 5.4-rt try_write_seqlock() is > not annotated, while write_seqcount_begin() called by the 5.9-rt > version leads to the broken annotation that the original then fixed. That is correct. I was looking at the 5.4-RT series Steven posted and I was under the impression that this patch was correctly missing in previous RT since I even added the stable tag. As Mike said, the previous RT implementation did not use seqlock annotation, they used a spin-lock instead. So the "try_write_seqlock()" did the try-lock annotation. With the reworked seqcount implementation (v5.6-RT time frame) this was solved differently (closer to what upstream does) and the now the annotation was wrong and fixed. Sorry for that. > -Mike Sebastian
Re: [PATCH RT 1/5] net: Properly annotate the try-lock for the seqlock
On Sat, 2020-11-14 at 13:24 -0600, Tom Zanussi wrote: > On Sat, 2020-11-14 at 20:00 +0100, Mike Galbraith wrote: > > > __raw_write_seqcount_end() is an integral part of write_sequnlock(), > > but we do seem to be missing a seqcount_release() in 5.4-rt. > > > > Yep, you're right, it's just the missing seqcount_release() - I'll > resubmit with just that. Or just drop the backport, since it adds annotation, while the original was fixing existing annotation. __raw_write_seqcount_begin() called in 5.4-rt try_write_seqlock() is not annotated, while write_seqcount_begin() called by the 5.9-rt version leads to the broken annotation that the original then fixed. -Mike
Re: [PATCH RT 1/5] net: Properly annotate the try-lock for the seqlock
On Sat, 2020-11-14 at 20:00 +0100, Mike Galbraith wrote:
> On Fri, 2020-11-13 at 14:14 -0600, Tom Zanussi wrote:
> >
> > This patch seems to fix it for me:
>
> If there was any discussion about this patch, I missed it.
There wasn't, just this thread.
>
> > From 4855377d0cb34b1b67a5c6d84cc8609c9da0bc3e Mon Sep 17 00:00:00
> > 2001
> > Message-Id: <
> > 4855377d0cb34b1b67a5c6d84cc8609c9da0bc3e.1605297603.git.zanu...@kernel.org
> > >
> > From: Tom Zanussi
> > Date: Fri, 13 Nov 2020 13:04:15 -0600
> > Subject: [PATCH] net: Add missing __raw_write_seqcount_end() and
> > seqcount_release()
> >
> > The patch ('net: Properly annotate the try-lock for the seqlock")
> > adds
> > __raw_write_seqcount_begin() in qdisc_run_begin() but omits the
> > corresponding __raw_write_seqcount_end() and seqcount_release() in
> > qdisc_run_end().
> >
> > Add it unconditionally, since qdisc_run_end() is never called
> > unless
> > qdisc_run_begin() succeeds, and if it succeeds,
> > __raw_write_seqcount_begin() seqcount_acquire() will have been
> > called.
> >
> > Signed-off-by: Tom Zanussi
> > ---
> > include/net/sch_generic.h | 4
> > 1 file changed, 4 insertions(+)
> >
> > diff --git a/include/net/sch_generic.h b/include/net/sch_generic.h
> > index 112d2dca8b08..c5ccce4f8f62 100644
> > --- a/include/net/sch_generic.h
> > +++ b/include/net/sch_generic.h
> > @@ -192,7 +192,11 @@ static inline bool qdisc_run_begin(struct
> > Qdisc *qdisc)
> > static inline void qdisc_run_end(struct Qdisc *qdisc)
> > {
> > #ifdef CONFIG_PREEMPT_RT
> > + seqcount_t *s = >running.seqcount;
> > +
> > write_sequnlock(>running);
> > + __raw_write_seqcount_end(s);
> > + seqcount_release(>dep_map, 1, _RET_IP_);
> > #else
> > write_seqcount_end(>running);
> > #endif
>
> __raw_write_seqcount_end() is an integral part of write_sequnlock(),
> but we do seem to be missing a seqcount_release() in 5.4-rt.
>
Yep, you're right, it's just the missing seqcount_release() - I'll
resubmit with just that.
Thanks,
Tom
> -Mike
>
Re: [PATCH RT 1/5] net: Properly annotate the try-lock for the seqlock
On Fri, 2020-11-13 at 14:14 -0600, Tom Zanussi wrote:
>
> This patch seems to fix it for me:
If there was any discussion about this patch, I missed it.
> From 4855377d0cb34b1b67a5c6d84cc8609c9da0bc3e Mon Sep 17 00:00:00 2001
> Message-Id:
> <4855377d0cb34b1b67a5c6d84cc8609c9da0bc3e.1605297603.git.zanu...@kernel.org>
> From: Tom Zanussi
> Date: Fri, 13 Nov 2020 13:04:15 -0600
> Subject: [PATCH] net: Add missing __raw_write_seqcount_end() and
> seqcount_release()
>
> The patch ('net: Properly annotate the try-lock for the seqlock") adds
> __raw_write_seqcount_begin() in qdisc_run_begin() but omits the
> corresponding __raw_write_seqcount_end() and seqcount_release() in
> qdisc_run_end().
>
> Add it unconditionally, since qdisc_run_end() is never called unless
> qdisc_run_begin() succeeds, and if it succeeds,
> __raw_write_seqcount_begin() seqcount_acquire() will have been called.
>
> Signed-off-by: Tom Zanussi
> ---
> include/net/sch_generic.h | 4
> 1 file changed, 4 insertions(+)
>
> diff --git a/include/net/sch_generic.h b/include/net/sch_generic.h
> index 112d2dca8b08..c5ccce4f8f62 100644
> --- a/include/net/sch_generic.h
> +++ b/include/net/sch_generic.h
> @@ -192,7 +192,11 @@ static inline bool qdisc_run_begin(struct Qdisc *qdisc)
> static inline void qdisc_run_end(struct Qdisc *qdisc)
> {
> #ifdef CONFIG_PREEMPT_RT
> + seqcount_t *s = >running.seqcount;
> +
> write_sequnlock(>running);
> + __raw_write_seqcount_end(s);
> + seqcount_release(>dep_map, 1, _RET_IP_);
> #else
> write_seqcount_end(>running);
> #endif
__raw_write_seqcount_end() is an integral part of write_sequnlock(),
but we do seem to be missing a seqcount_release() in 5.4-rt.
-Mike
Re: [PATCH RT 1/5] net: Properly annotate the try-lock for the seqlock
Hi Steve,
On Fri, 2020-11-13 at 11:06 -0600, Tom Zanussi wrote:
> Hi Steve,
>
> On Tue, 2020-11-10 at 10:38 -0500, Steven Rostedt wrote:
> > 5.4.74-rt42-rc2 stable review patch.
> > If anyone has any objections, please let me know.
> >
> > --
> >
> > From: Sebastian Andrzej Siewior
> >
> > In patch
> >("net/Qdisc: use a seqlock instead seqcount")
> >
> > the seqcount has been replaced with a seqlock to allow to reader to
> > boost the preempted writer.
> > The try_write_seqlock() acquired the lock with a try-lock but the
> > seqcount annotation was "lock".
> >
> > Opencode write_seqcount_t_begin() and use the try-lock annotation
> > for
> > lockdep.
> >
> > Reported-by: Mike Galbraith
> > Cc: stable...@vger.kernel.org
> > Signed-off-by: Sebastian Andrzej Siewior
> > Signed-off-by: Steven Rostedt (VMware)
> > ---
> > include/linux/seqlock.h | 9 -
> > include/net/sch_generic.h | 10 +-
> > 2 files changed, 9 insertions(+), 10 deletions(-)
> >
> > diff --git a/include/linux/seqlock.h b/include/linux/seqlock.h
> > index e5207897c33e..f390293974ea 100644
> > --- a/include/linux/seqlock.h
> > +++ b/include/linux/seqlock.h
> > @@ -489,15 +489,6 @@ static inline void write_seqlock(seqlock_t
> > *sl)
> > __raw_write_seqcount_begin(>seqcount);
> > }
> >
> > -static inline int try_write_seqlock(seqlock_t *sl)
> > -{
> > - if (spin_trylock(>lock)) {
> > - __raw_write_seqcount_begin(>seqcount);
> > - return 1;
> > - }
> > - return 0;
> > -}
> > -
> > static inline void write_sequnlock(seqlock_t *sl)
> > {
> > __raw_write_seqcount_end(>seqcount);
> > diff --git a/include/net/sch_generic.h b/include/net/sch_generic.h
> > index e6afb4b9cede..112d2dca8b08 100644
> > --- a/include/net/sch_generic.h
> > +++ b/include/net/sch_generic.h
> > @@ -168,8 +168,16 @@ static inline bool qdisc_run_begin(struct
> > Qdisc
> > *qdisc)
> > return false;
> > }
> > #ifdef CONFIG_PREEMPT_RT
> > - if (try_write_seqlock(>running))
> > + if (spin_trylock(>running.lock)) {
> > + seqcount_t *s = >running.seqcount;
> > + /*
> > +* Variant of write_seqcount_t_begin() telling lockdep
> > that a
> > +* trylock was attempted.
> > +*/
> > + __raw_write_seqcount_begin(s);
> > + seqcount_acquire(>dep_map, 0, 1, _RET_IP_);
> > return true;
> > + }
> > return false;
> > #else
> > /* Variant of write_seqcount_begin() telling lockdep a trylock
>
> I applied this to 4.19 and saw some splat with my 'debug-full'
> configuration, so tried 5.4 and saw the same thing:
>
> [ 30.573698] BUG: workqueue leaked lock or atomic:
> kworker/1:4/0x/143
> last function: wireless_nlevent_process
> [ 30.573707] 1 lock held by kworker/1:4/143:
> [ 30.573708] #0: 8e981d80 (noop_qdisc.running#2){+.+.},
> at: __do_softirq+0xca/0x561
> [ 30.573715] CPU: 1 PID: 143 Comm: kworker/1:4 Not tainted 5.4.74-
> rt42-rt-test-full-debug #1
> [ 30.573716] Hardware name: LENOVO 4236L51/4236L51, BIOS 83ET59WW
> (1.29 ) 06/01/2011
> [ 30.573720] Workqueue: events wireless_nlevent_process
> [ 30.573721] Call Trace:
> [ 30.573724] dump_stack+0x71/0x9b
> [ 30.573728] process_one_work+0x533/0x760
> [ 30.573731] worker_thread+0x39/0x3f0
> [ 30.573733] ? process_one_work+0x760/0x760
> [ 30.573734] kthread+0x165/0x180
> [ 30.573736] ? __kthread_create_on_node+0x180/0x180
> [ 30.573737] ret_from_fork+0x3a/0x50
> [ 30.629329] wlp3s0: authenticate with 84:1b:5e:41:5e:4d
> [ 30.634864] wlp3s0: send auth to 84:1b:5e:41:5e:4d (try 1/3)
> [ 30.638433] wlp3s0: authenticated
> [ 30.642250] wlp3s0: associate with 84:1b:5e:41:5e:4d (try 1/3)
> [ 30.645704] wlp3s0: RX AssocResp from 84:1b:5e:41:5e:4d
> (capab=0x411 status=0 aid=6)
> [ 30.650803] wlp3s0: associated
>
> [ 30.656764]
> [ 30.656765] WARNING: lock held when returning to user space!
> [ 30.656766] 5.4.74-rt42-rt-test-full-debug #1 Not tainted
> [ 30.656767]
> [ 30.656768] wpa_supplicant/836 is leaving the kernel with locks
> still held!
> [ 30.656769] 1 lock held by wpa_supplicant/836:
> [ 30.656770] #0: 98f1c9622280 (>qdisc_running_key){+.+.},
> at: packet_sendmsg+0xec1/0x1ad0
>
> Let me know if you want me to send you my .config or the full output
> (a
> bunch more of the above).
>
> Thanks,
This patch seems to fix it for me:
>From 4855377d0cb34b1b67a5c6d84cc8609c9da0bc3e Mon Sep 17 00:00:00 2001
Message-Id:
<4855377d0cb34b1b67a5c6d84cc8609c9da0bc3e.1605297603.git.zanu...@kernel.org>
From: Tom Zanussi
Date: Fri, 13 Nov 2020 13:04:15 -0600
Subject: [PATCH] net: Add missing __raw_write_seqcount_end() and
seqcount_release()
The patch ('net: Properly annotate the try-lock for the seqlock") adds
__raw_write_seqcount_begin() in
Re: [PATCH RT 1/5] net: Properly annotate the try-lock for the seqlock
Hi Steve,
On Tue, 2020-11-10 at 10:38 -0500, Steven Rostedt wrote:
> 5.4.74-rt42-rc2 stable review patch.
> If anyone has any objections, please let me know.
>
> --
>
> From: Sebastian Andrzej Siewior
>
> In patch
>("net/Qdisc: use a seqlock instead seqcount")
>
> the seqcount has been replaced with a seqlock to allow to reader to
> boost the preempted writer.
> The try_write_seqlock() acquired the lock with a try-lock but the
> seqcount annotation was "lock".
>
> Opencode write_seqcount_t_begin() and use the try-lock annotation for
> lockdep.
>
> Reported-by: Mike Galbraith
> Cc: stable...@vger.kernel.org
> Signed-off-by: Sebastian Andrzej Siewior
> Signed-off-by: Steven Rostedt (VMware)
> ---
> include/linux/seqlock.h | 9 -
> include/net/sch_generic.h | 10 +-
> 2 files changed, 9 insertions(+), 10 deletions(-)
>
> diff --git a/include/linux/seqlock.h b/include/linux/seqlock.h
> index e5207897c33e..f390293974ea 100644
> --- a/include/linux/seqlock.h
> +++ b/include/linux/seqlock.h
> @@ -489,15 +489,6 @@ static inline void write_seqlock(seqlock_t *sl)
> __raw_write_seqcount_begin(>seqcount);
> }
>
> -static inline int try_write_seqlock(seqlock_t *sl)
> -{
> - if (spin_trylock(>lock)) {
> - __raw_write_seqcount_begin(>seqcount);
> - return 1;
> - }
> - return 0;
> -}
> -
> static inline void write_sequnlock(seqlock_t *sl)
> {
> __raw_write_seqcount_end(>seqcount);
> diff --git a/include/net/sch_generic.h b/include/net/sch_generic.h
> index e6afb4b9cede..112d2dca8b08 100644
> --- a/include/net/sch_generic.h
> +++ b/include/net/sch_generic.h
> @@ -168,8 +168,16 @@ static inline bool qdisc_run_begin(struct Qdisc
> *qdisc)
> return false;
> }
> #ifdef CONFIG_PREEMPT_RT
> - if (try_write_seqlock(>running))
> + if (spin_trylock(>running.lock)) {
> + seqcount_t *s = >running.seqcount;
> + /*
> + * Variant of write_seqcount_t_begin() telling lockdep
> that a
> + * trylock was attempted.
> + */
> + __raw_write_seqcount_begin(s);
> + seqcount_acquire(>dep_map, 0, 1, _RET_IP_);
> return true;
> + }
> return false;
> #else
> /* Variant of write_seqcount_begin() telling lockdep a trylock
I applied this to 4.19 and saw some splat with my 'debug-full'
configuration, so tried 5.4 and saw the same thing:
[ 30.573698] BUG: workqueue leaked lock or atomic: kworker/1:4/0x/143
last function: wireless_nlevent_process
[ 30.573707] 1 lock held by kworker/1:4/143:
[ 30.573708] #0: 8e981d80 (noop_qdisc.running#2){+.+.}, at:
__do_softirq+0xca/0x561
[ 30.573715] CPU: 1 PID: 143 Comm: kworker/1:4 Not tainted
5.4.74-rt42-rt-test-full-debug #1
[ 30.573716] Hardware name: LENOVO 4236L51/4236L51, BIOS 83ET59WW (1.29 )
06/01/2011
[ 30.573720] Workqueue: events wireless_nlevent_process
[ 30.573721] Call Trace:
[ 30.573724] dump_stack+0x71/0x9b
[ 30.573728] process_one_work+0x533/0x760
[ 30.573731] worker_thread+0x39/0x3f0
[ 30.573733] ? process_one_work+0x760/0x760
[ 30.573734] kthread+0x165/0x180
[ 30.573736] ? __kthread_create_on_node+0x180/0x180
[ 30.573737] ret_from_fork+0x3a/0x50
[ 30.629329] wlp3s0: authenticate with 84:1b:5e:41:5e:4d
[ 30.634864] wlp3s0: send auth to 84:1b:5e:41:5e:4d (try 1/3)
[ 30.638433] wlp3s0: authenticated
[ 30.642250] wlp3s0: associate with 84:1b:5e:41:5e:4d (try 1/3)
[ 30.645704] wlp3s0: RX AssocResp from 84:1b:5e:41:5e:4d (capab=0x411
status=0 aid=6)
[ 30.650803] wlp3s0: associated
[ 30.656764]
[ 30.656765] WARNING: lock held when returning to user space!
[ 30.656766] 5.4.74-rt42-rt-test-full-debug #1 Not tainted
[ 30.656767]
[ 30.656768] wpa_supplicant/836 is leaving the kernel with locks still held!
[ 30.656769] 1 lock held by wpa_supplicant/836:
[ 30.656770] #0: 98f1c9622280 (>qdisc_running_key){+.+.}, at:
packet_sendmsg+0xec1/0x1ad0
Let me know if you want me to send you my .config or the full output (a
bunch more of the above).
Thanks,
Tom
[PATCH RT 1/5] net: Properly annotate the try-lock for the seqlock
5.4.74-rt42-rc2 stable review patch.
If anyone has any objections, please let me know.
--
From: Sebastian Andrzej Siewior
In patch
("net/Qdisc: use a seqlock instead seqcount")
the seqcount has been replaced with a seqlock to allow to reader to
boost the preempted writer.
The try_write_seqlock() acquired the lock with a try-lock but the
seqcount annotation was "lock".
Opencode write_seqcount_t_begin() and use the try-lock annotation for
lockdep.
Reported-by: Mike Galbraith
Cc: stable...@vger.kernel.org
Signed-off-by: Sebastian Andrzej Siewior
Signed-off-by: Steven Rostedt (VMware)
---
include/linux/seqlock.h | 9 -
include/net/sch_generic.h | 10 +-
2 files changed, 9 insertions(+), 10 deletions(-)
diff --git a/include/linux/seqlock.h b/include/linux/seqlock.h
index e5207897c33e..f390293974ea 100644
--- a/include/linux/seqlock.h
+++ b/include/linux/seqlock.h
@@ -489,15 +489,6 @@ static inline void write_seqlock(seqlock_t *sl)
__raw_write_seqcount_begin(>seqcount);
}
-static inline int try_write_seqlock(seqlock_t *sl)
-{
- if (spin_trylock(>lock)) {
- __raw_write_seqcount_begin(>seqcount);
- return 1;
- }
- return 0;
-}
-
static inline void write_sequnlock(seqlock_t *sl)
{
__raw_write_seqcount_end(>seqcount);
diff --git a/include/net/sch_generic.h b/include/net/sch_generic.h
index e6afb4b9cede..112d2dca8b08 100644
--- a/include/net/sch_generic.h
+++ b/include/net/sch_generic.h
@@ -168,8 +168,16 @@ static inline bool qdisc_run_begin(struct Qdisc *qdisc)
return false;
}
#ifdef CONFIG_PREEMPT_RT
- if (try_write_seqlock(>running))
+ if (spin_trylock(>running.lock)) {
+ seqcount_t *s = >running.seqcount;
+ /*
+* Variant of write_seqcount_t_begin() telling lockdep that a
+* trylock was attempted.
+*/
+ __raw_write_seqcount_begin(s);
+ seqcount_acquire(>dep_map, 0, 1, _RET_IP_);
return true;
+ }
return false;
#else
/* Variant of write_seqcount_begin() telling lockdep a trylock
--
2.28.0
