[PATCH RT 0/5] Linux 5.4.74-rt42-rc2

[PATCH RT 0/5] Linux 5.4.74-rt42-rc2

[Steven Rostedt]

Dear RT Folks,

This is the RT stable review cycle of patch 5.4.74-rt42-rc2.

Please scream at me if I messed something up. Please test the patches too.

The -rc release will be uploaded to kernel.org and will be deleted when
the final release is out. This is just a review release (or release candidate).

The pre-releases will not be pushed to the git repository, only the
final release is.

If all goes well, this patch will be converted to the next main release
on 11/11/2020.

Changes since -rc1: removed patch:

    tcp: Remove superfluous BH-disable around listening_hash

Enjoy,

-- Steve


To build 5.4.74-rt42-rc2 directly, the following patches should be applied:

  http://www.kernel.org/pub/linux/kernel/v5.x/linux-5.4.tar.xz

  http://www.kernel.org/pub/linux/kernel/v5.x/patch-5.4.74.xz

  http://www.kernel.org/pub/linux/kernel/projects/rt/5.4/patch-5.4.74-rt42-rc2.patch.xz

You can also build from 5.4.74-rt41 by applying the incremental patch:

http://www.kernel.org/pub/linux/kernel/projects/rt/5.4/incr/patch-5.4.74-rt41-rt42-rc2.patch.xz


Changes from 5.4.74-rt41:

---


Oleg Nesterov (1):
      ptrace: fix ptrace_unfreeze_traced() race with rt-lock

Sebastian Andrzej Siewior (3):
      net: Properly annotate the try-lock for the seqlock
      mm/memcontrol: Disable preemption in __mod_memcg_lruvec_state()
      timers: Don't block on ->expiry_lock for TIMER_IRQSAFE

Steven Rostedt (VMware) (1):
      Linux 5.4.74-rt42-rc2

----
 include/linux/seqlock.h   |  9 ---------
 include/net/sch_generic.h | 10 +++++++++-
 kernel/ptrace.c           | 23 +++++++++++++++--------
 kernel/time/timer.c       |  9 ++++++++-
 localversion-rt           |  2 +-
 mm/memcontrol.c           |  2 ++
 6 files changed, 35 insertions(+), 20 deletions(-)

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

[Steven Rostedt]

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

[PATCH RT 2/5] mm/memcontrol: Disable preemption in __mod_memcg_lruvec_state()

[Steven Rostedt]

5.4.74-rt42-rc2 stable review patch.
If anyone has any objections, please let me know.

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

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

The callers expect disabled preemption/interrupts while invoking
__mod_memcg_lruvec_state(). This works mainline because a lock of
somekind is acquired.

Use preempt_disable_rt() where per-CPU variables are accessed and a
stable pointer is expected. This is also done in __mod_zone_page_state()
for the same reason.

Cc: stable-rt@vger.kernel.org
Signed-off-by: Sebastian Andrzej Siewior <bigeasy@linutronix.de>
Signed-off-by: Steven Rostedt (VMware) <rostedt@goodmis.org>
---
 mm/memcontrol.c | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/mm/memcontrol.c b/mm/memcontrol.c
index 9bdb75ef6d62..c9d02e2272e1 100644
--- a/mm/memcontrol.c
+++ b/mm/memcontrol.c
@@ -752,6 +752,7 @@ void __mod_lruvec_state(struct lruvec *lruvec, enum node_stat_item idx,
 	pn = container_of(lruvec, struct mem_cgroup_per_node, lruvec);
 	memcg = pn->memcg;
 
+	preempt_disable_rt();
 	/* Update memcg */
 	__mod_memcg_state(memcg, idx, val);
 
@@ -767,6 +768,7 @@ void __mod_lruvec_state(struct lruvec *lruvec, enum node_stat_item idx,
 		x = 0;
 	}
 	__this_cpu_write(pn->lruvec_stat_cpu->count[idx], x);
+	preempt_enable_rt();
 }
 
 void __mod_lruvec_slab_state(void *p, enum node_stat_item idx, int val)
-- 
2.28.0

[PATCH RT 3/5] ptrace: fix ptrace_unfreeze_traced() race with rt-lock

[Steven Rostedt]

5.4.74-rt42-rc2 stable review patch.
If anyone has any objections, please let me know.

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

From: Oleg Nesterov <oleg@redhat.com>

The patch "ptrace: fix ptrace vs tasklist_lock race" changed
ptrace_freeze_traced() to take task->saved_state into account, but
ptrace_unfreeze_traced() has the same problem and needs a similar fix:
it should check/update both ->state and ->saved_state.

Reported-by: Luis Claudio R. Goncalves <lgoncalv@redhat.com>
Fixes: "ptrace: fix ptrace vs tasklist_lock race"
Signed-off-by: Oleg Nesterov <oleg@redhat.com>
Signed-off-by: Sebastian Andrzej Siewior <bigeasy@linutronix.de>
Cc: stable-rt@vger.kernel.org
Signed-off-by: Steven Rostedt (VMware) <rostedt@goodmis.org>
---
 kernel/ptrace.c | 23 +++++++++++++++--------
 1 file changed, 15 insertions(+), 8 deletions(-)

diff --git a/kernel/ptrace.c b/kernel/ptrace.c
index 3075006d720e..3f7156f06b6c 100644
--- a/kernel/ptrace.c
+++ b/kernel/ptrace.c
@@ -197,8 +197,8 @@ static bool ptrace_freeze_traced(struct task_struct *task)
 
 static void ptrace_unfreeze_traced(struct task_struct *task)
 {
-	if (task->state != __TASK_TRACED)
-		return;
+	unsigned long flags;
+	bool frozen = true;
 
 	WARN_ON(!task->ptrace || task->parent != current);
 
@@ -207,12 +207,19 @@ static void ptrace_unfreeze_traced(struct task_struct *task)
 	 * Recheck state under the lock to close this race.
 	 */
 	spin_lock_irq(&task->sighand->siglock);
-	if (task->state == __TASK_TRACED) {
-		if (__fatal_signal_pending(task))
-			wake_up_state(task, __TASK_TRACED);
-		else
-			task->state = TASK_TRACED;
-	}
+
+	raw_spin_lock_irqsave(&task->pi_lock, flags);
+	if (task->state == __TASK_TRACED)
+		task->state = TASK_TRACED;
+	else if (task->saved_state == __TASK_TRACED)
+		task->saved_state = TASK_TRACED;
+	else
+		frozen = false;
+	raw_spin_unlock_irqrestore(&task->pi_lock, flags);
+
+	if (frozen && __fatal_signal_pending(task))
+		wake_up_state(task, __TASK_TRACED);
+
 	spin_unlock_irq(&task->sighand->siglock);
 }
 
-- 
2.28.0

[PATCH RT 4/5] timers: Dont block on ->expiry_lock for TIMER_IRQSAFE

[Steven Rostedt]

5.4.74-rt42-rc2 stable review patch.
If anyone has any objections, please let me know.

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

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

PREEMPT_RT does not spin and wait until a running timer completes its
callback but instead it blocks on a sleeping lock to prevent a deadlock.

This blocking can not be done for workqueue's IRQ_SAFE timer which will
be canceled in an IRQ-off region. It has to happen to in IRQ-off region
because changing the PENDING bit and clearing the timer must not be
interrupted to avoid a busy-loop.

The callback invocation of IRQSAFE timer is not preempted on PREEMPT_RT
so there is no need to synchronize on timer_base::expiry_lock.

Don't acquire the timer_base::expiry_lock for TIMER_IRQSAFE flagged
timer.
Add a lockdep annotation to ensure that this function is always invoked
in preemptible context on PREEMPT_RT.

Reported-by: Mike Galbraith <efault@gmx.de>
Signed-off-by: Sebastian Andrzej Siewior <bigeasy@linutronix.de>
Cc: stable-rt@vger.kernel.org
Signed-off-by: Steven Rostedt (VMware) <rostedt@goodmis.org>
---
 kernel/time/timer.c | 9 ++++++++-
 1 file changed, 8 insertions(+), 1 deletion(-)

diff --git a/kernel/time/timer.c b/kernel/time/timer.c
index 89078fd848b9..3e9d7f227a5c 100644
--- a/kernel/time/timer.c
+++ b/kernel/time/timer.c
@@ -1289,7 +1289,7 @@ static void del_timer_wait_running(struct timer_list *timer)
 	u32 tf;
 
 	tf = READ_ONCE(timer->flags);
-	if (!(tf & TIMER_MIGRATING)) {
+	if (!(tf & (TIMER_MIGRATING | TIMER_IRQSAFE))) {
 		struct timer_base *base = get_timer_base(tf);
 
 		/*
@@ -1373,6 +1373,13 @@ int del_timer_sync(struct timer_list *timer)
 	 */
 	WARN_ON(in_irq() && !(timer->flags & TIMER_IRQSAFE));
 
+	/*
+	 * Must be able to sleep on PREEMPT_RT because of the slowpath in
+	 * del_timer_wait_running().
+	 */
+	if (IS_ENABLED(CONFIG_PREEMPT_RT) && !(timer->flags & TIMER_IRQSAFE))
+		might_sleep();
+
 	do {
 		ret = try_to_del_timer_sync(timer);
 
-- 
2.28.0

[PATCH RT 5/5] Linux 5.4.74-rt42-rc2

[Steven Rostedt]

5.4.74-rt42-rc2 stable review patch.
If anyone has any objections, please let me know.

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

From: "Steven Rostedt (VMware)" <rostedt@goodmis.org>

---
 localversion-rt | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/localversion-rt b/localversion-rt
index 629e0b4384b8..9338de9953e3 100644
--- a/localversion-rt
+++ b/localversion-rt
@@ -1 +1 @@
--rt41
+-rt42-rc2
-- 
2.28.0

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

[Tom Zanussi]

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

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

[Tom Zanussi]

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

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

[Mike Galbraith]

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

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

[Tom Zanussi]

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
> 

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

[Mike Galbraith]

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

[Sebastian Andrzej Siewior]

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