diff mbox series

[RT,1/5] net: Properly annotate the try-lock for the seqlock

Message ID 20201110154024.958923729@goodmis.org
State New
Headers show
Series Linux 5.4.74-rt42-rc2 | expand

Commit Message

Steven Rostedt Nov. 10, 2020, 3:38 p.m. UTC 
5.4.74-rt42-rc2 stable review patch.
If anyone has any objections, please let me know.

------------------

From: Sebastian Andrzej Siewior <bigeasy@linutronix.de>

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@gmx.de>
Cc: stable-rt@vger.kernel.org
Signed-off-by: Sebastian Andrzej Siewior <bigeasy@linutronix.de>
Signed-off-by: Steven Rostedt (VMware) <rostedt@goodmis.org>
---
 include/linux/seqlock.h   |  9 ---------
 include/net/sch_generic.h | 10 +++++++++-
 2 files changed, 9 insertions(+), 10 deletions(-)

Comments

Tom Zanussi Nov. 13, 2020, 5:06 p.m. UTC | #1 
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@linutronix.de>

> 

> 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@gmx.de>

> Cc: stable-rt@vger.kernel.org

> Signed-off-by: Sebastian Andrzej Siewior <bigeasy@linutronix.de>

> Signed-off-by: Steven Rostedt (VMware) <rostedt@goodmis.org>

> ---

>  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,

Tom
Tom Zanussi Nov. 13, 2020, 8:14 p.m. UTC | #2 
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@linutronix.de>

> > 

> > 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@gmx.de>

> > Cc: stable-rt@vger.kernel.org

> > Signed-off-by: Sebastian Andrzej Siewior <bigeasy@linutronix.de>

> > Signed-off-by: Steven Rostedt (VMware) <rostedt@goodmis.org>

> > ---

> >  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@kernel.org>
From: Tom Zanussi <zanussi@kernel.org>

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@kernel.org>

---
 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
Mike Galbraith Nov. 14, 2020, 7 p.m. UTC | #3 
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.zanussi@kernel.org>

> From: Tom Zanussi <zanussi@kernel.org>

> 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@kernel.org>

> ---

>  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


__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
Tom Zanussi Nov. 14, 2020, 7:24 p.m. UTC | #4 
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.zanussi@kernel.org

> > >

> > From: Tom Zanussi <zanussi@kernel.org>

> > 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@kernel.org>

> > ---

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

> 

> __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

>
Mike Galbraith Nov. 15, 2020, 4:52 a.m. UTC | #5 
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
Sebastian Andrzej Siewior Nov. 16, 2020, 5:19 p.m. UTC | #6 
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
diff mbox series

Patch

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