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 <bigeasy@xxxxxxxxxxxxx> > > > > 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 <efault@xxxxxx> > > Cc: stable-rt@xxxxxxxxxxxxxxx > > Signed-off-by: Sebastian Andrzej Siewior <bigeasy@xxxxxxxxxxxxx> > > Signed-off-by: Steven Rostedt (VMware) <rostedt@xxxxxxxxxxx> > > --- > > 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(&sl->seqcount); > > } > > > > -static inline int try_write_seqlock(seqlock_t *sl) > > -{ > > - if (spin_trylock(&sl->lock)) { > > - __raw_write_seqcount_begin(&sl->seqcount); > > - return 1; > > - } > > - return 0; > > -} > > - > > static inline void write_sequnlock(seqlock_t *sl) > > { > > __raw_write_seqcount_end(&sl->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(&qdisc->running)) > > + if (spin_trylock(&qdisc->running.lock)) { > > + seqcount_t *s = &qdisc->running.seqcount; > > + /* > > + * Variant of write_seqcount_t_begin() telling lockdep > > that a > > + * trylock was attempted. > > + */ > > + __raw_write_seqcount_begin(s); > > + seqcount_acquire(&s->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/0x00000000/143 > last function: wireless_nlevent_process > [ 30.573707] 1 lock held by kworker/1:4/143: > [ 30.573708] #0: ffffffff8e981d80 (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: ffff98f1c9622280 (&dev->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.zanussi@xxxxxxxxxx> From: Tom Zanussi <zanussi@xxxxxxxxxx> 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 <zanussi@xxxxxxxxxx> --- 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 = &qdisc->running.seqcount; + write_sequnlock(&qdisc->running); + __raw_write_seqcount_end(s); + seqcount_release(&s->dep_map, 1, _RET_IP_); #else write_seqcount_end(&qdisc->running); #endif -- 2.17.1