[PATCH] workqueue: Use raise_softirq() to trigger softirq in irq_work handler

[PATCH] workqueue: Use raise_softirq() to trigger softirq in irq_work handler

[Zqiang]

The bh_pool_kick_normal() and bh_pool_kick_highpri() are registered via
init_irq_work() without IRQ_WORK_HARD_IRQ flag. On PREEMPT_RT, such
work items are processed by the per-CPU irq_workd kthread in preemptible
task context with IRQs enabled. However calling raise_softirq_irqoff()
requires IRQS must be disabled, This commit therefore replace
raise_softirq_irqoff() with raise_softirq() in irq_work handler.

Signed-off-by: Zqiang <qiang.zhang@linux.dev>
---
 kernel/workqueue.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/kernel/workqueue.c b/kernel/workqueue.c
index 78068ae8f28a..0bb978df0200 100644
--- a/kernel/workqueue.c
+++ b/kernel/workqueue.c
@@ -7923,12 +7923,12 @@ static inline void wq_watchdog_init(void) { }
 
 static void bh_pool_kick_normal(struct irq_work *irq_work)
 {
-	raise_softirq_irqoff(TASKLET_SOFTIRQ);
+	raise_softirq(TASKLET_SOFTIRQ);
 }
 
 static void bh_pool_kick_highpri(struct irq_work *irq_work)
 {
-	raise_softirq_irqoff(HI_SOFTIRQ);
+	raise_softirq(HI_SOFTIRQ);
 }
 
 static void __init restrict_unbound_cpumask(const char *name, const struct cpumask *mask)
-- 
2.17.1

Re: [PATCH] workqueue: Use raise_softirq() to trigger softirq in irq_work handler

[Bradley Morgan]

> The bh_pool_kick_normal() and bh_pool_kick_highpri() are registered via
> init_irq_work() without IRQ_WORK_HARD_IRQ flag. On PREEMPT_RT, such
> work items are processed by the per-CPU irq_workd kthread in preemptible
> task context with IRQs enabled. However calling raise_softirq_irqoff()
> requires IRQS must be disabled, This commit therefore replace
> raise_softirq_irqoff() with raise_softirq() in irq_work handler.

Hi zhang!

Could you add

Fixes: 4cb1ef64609f ("workqueue: Implement BH workqueues to eventually replace tasklets")

?

Also, if you would like this to go to stable, could you write a small
part of the description about the user-space effects and why this is
bad?


After all changes.


Reviewed-by: Bradley Morgan <include@grrlz.net>


> Signed-off-by: Zqiang <qiang.zhang@linux.dev>
> ---
>  kernel/workqueue.c | 4 ++--
>  1 file changed, 2 insertions(+), 2 deletions(-)
>
> diff --git a/kernel/workqueue.c b/kernel/workqueue.c
> index 78068ae8f28a..0bb978df0200 100644
> --- a/kernel/workqueue.c
> +++ b/kernel/workqueue.c
> @@ -7923,12 +7923,12 @@ static inline void wq_watchdog_init(void) { }
>  
>  static void bh_pool_kick_normal(struct irq_work *irq_work)
>  {
> -	raise_softirq_irqoff(TASKLET_SOFTIRQ);
> +	raise_softirq(TASKLET_SOFTIRQ);
>  }
>  
>  static void bh_pool_kick_highpri(struct irq_work *irq_work)
>  {
> -	raise_softirq_irqoff(HI_SOFTIRQ);
> +	raise_softirq(HI_SOFTIRQ);
>  }
>  
>  static void __init restrict_unbound_cpumask(const char *name, const struct cpumask *mask)
> -- 
> 2.17.1

Thanks!

Re: [PATCH] workqueue: Use raise_softirq() to trigger softirq in irq_work handler

[Zqiang]

> 
> > 
> > The bh_pool_kick_normal() and bh_pool_kick_highpri() are registered via
> >  init_irq_work() without IRQ_WORK_HARD_IRQ flag. On PREEMPT_RT, such
> >  work items are processed by the per-CPU irq_workd kthread in preemptible
> >  task context with IRQs enabled. However calling raise_softirq_irqoff()
> >  requires IRQS must be disabled, This commit therefore replace
> >  raise_softirq_irqoff() with raise_softirq() in irq_work handler.
> > 
> Hi zhang!
> 
> Could you add
> 
> Fixes: 4cb1ef64609f ("workqueue: Implement BH workqueues to eventually replace tasklets")
> 
> ?
> 
> Also, if you would like this to go to stable, could you write a small
> part of the description about the user-space effects and why this is
> bad?

The or_softirq_pending(nr) be translated into the following three
assembly instructions(arm64 platform)：

ldr w0, [x18, #softirq_pending_offset]

by interrupt:
calling  __raise_softirq_irqoff(NET_RX_SOFTIRQ)

orr w0, w0, 1<< nr
str w0, [x18, #softirq_pending_offset] <-- still use old value, the NET_RX_SOFTIRQ maybe lose.


Thanks
Zqiang


> 
> After all changes.
> 
> Reviewed-by: Bradley Morgan <include@grrlz.net>
> 
> > 
> > Signed-off-by: Zqiang <qiang.zhang@linux.dev>
> >  ---
> >  kernel/workqueue.c | 4 ++--
> >  1 file changed, 2 insertions(+), 2 deletions(-)
> > 
> >  diff --git a/kernel/workqueue.c b/kernel/workqueue.c
> >  index 78068ae8f28a..0bb978df0200 100644
> >  --- a/kernel/workqueue.c
> >  +++ b/kernel/workqueue.c
> >  @@ -7923,12 +7923,12 @@ static inline void wq_watchdog_init(void) { }
> >  
> >  static void bh_pool_kick_normal(struct irq_work *irq_work)
> >  {
> >  - raise_softirq_irqoff(TASKLET_SOFTIRQ);
> >  + raise_softirq(TASKLET_SOFTIRQ);
> >  }
> >  
> >  static void bh_pool_kick_highpri(struct irq_work *irq_work)
> >  {
> >  - raise_softirq_irqoff(HI_SOFTIRQ);
> >  + raise_softirq(HI_SOFTIRQ);
> >  }
> >  
> >  static void __init restrict_unbound_cpumask(const char *name, const struct cpumask *mask)
> >  -- 
> >  2.17.1
> > 
> Thanks!
>

Re: [PATCH] workqueue: Use raise_softirq() to trigger softirq in irq_work handler

[Tejun Heo]

Hello,

Applied to wq/for-7.1-fixes with the description reworded and the
following tags added:

  Fixes: 2f34d7337d98 ("workqueue: Fix queue_work_on() with BH workqueues")
  Cc: stable@vger.kernel.org # v6.9+

The Fixes target differs from the suggested 4cb1ef64609f. There, the
raises ran under pool->lock with IRQs disabled. The irq_work handlers
were added later by 2f34d7337d98 which moved the remote-CPU raise out of
kick_pool(). Also picked up Bradley's Reviewed-by.

Thanks.

-- 
tejun

Re: [PATCH] workqueue: Use raise_softirq() to trigger softirq in irq_work handler

[Zqiang]

> 
> Hello,
> 
> Applied to wq/for-7.1-fixes with the description reworded and the
> following tags added:
> 
>  Fixes: 2f34d7337d98 ("workqueue: Fix queue_work_on() with BH workqueues")
>  Cc: stable@vger.kernel.org # v6.9+
> 
> The Fixes target differs from the suggested 4cb1ef64609f. There, the
> raises ran under pool->lock with IRQs disabled. The irq_work handlers
> were added later by 2f34d7337d98 which moved the remote-CPU raise out of
> kick_pool(). Also picked up Bradley's Reviewed-by.

Hello, Tejun

Thank you very much for reword description!


> 
> Thanks.
> 
> -- 
> tejun
>