[PATCH net-next 1/2] dpaa2-eth: use napi_schedule to be compatible with PREEMPT_RT

[PATCH net-next 1/2] dpaa2-eth: use napi_schedule to be compatible with PREEMPT_RT

[Vladimir Oltean]

From: Jiafei Pan <Jiafei.Pan@nxp.com>

The driver calls napi_schedule_irqoff() from a context where, in RT,
hardirqs are not disabled, since the IRQ handler is force-threaded.

In the call path of this function, __raise_softirq_irqoff() is modifying
its per-CPU mask of pending softirqs that must be processed, using
or_softirq_pending(). The or_softirq_pending() function is not atomic,
but since interrupts are supposed to be disabled, nobody should be
preempting it, and the operation should be safe.

Nonetheless, when running with hardirqs on, as in the PREEMPT_RT case,
it isn't safe, and the pending softirqs mask can get corrupted,
resulting in softirqs being lost and never processed.

To have common code that works with PREEMPT_RT and with mainline Linux,
we can use plain napi_schedule() instead. The difference is that
napi_schedule() (via __napi_schedule) also calls local_irq_save, which
disables hardirqs if they aren't already. But, since they already are
disabled in non-RT, this means that in practice we don't see any
measurable difference in throughput or latency with this patch.

Signed-off-by: Jiafei Pan <Jiafei.Pan@nxp.com>
Signed-off-by: Vladimir Oltean <vladimir.oltean@nxp.com>
---
 drivers/net/ethernet/freescale/dpaa2/dpaa2-eth.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/net/ethernet/freescale/dpaa2/dpaa2-eth.c b/drivers/net/ethernet/freescale/dpaa2/dpaa2-eth.c
index 9b4028c0e34c..50f52fe2012f 100644
--- a/drivers/net/ethernet/freescale/dpaa2/dpaa2-eth.c
+++ b/drivers/net/ethernet/freescale/dpaa2/dpaa2-eth.c
@@ -2330,7 +2330,7 @@ static void cdan_cb(struct dpaa2_io_notification_ctx *ctx)
 	/* Update NAPI statistics */
 	ch->stats.cdan++;
 
-	napi_schedule_irqoff(&ch->napi);
+	napi_schedule(&ch->napi);
 }
 
 /* Allocate and configure a DPCON object */
-- 
2.25.1

[PATCH net-next 2/2] enetc: use napi_schedule to be compatible with PREEMPT_RT

[Vladimir Oltean]

From: Jiafei Pan <Jiafei.Pan@nxp.com>

The driver calls napi_schedule_irqoff() from a context where, in RT,
hardirqs are not disabled, since the IRQ handler is force-threaded.

In the call path of this function, __raise_softirq_irqoff() is modifying
its per-CPU mask of pending softirqs that must be processed, using
or_softirq_pending(). The or_softirq_pending() function is not atomic,
but since interrupts are supposed to be disabled, nobody should be
preempting it, and the operation should be safe.

Nonetheless, when running with hardirqs on, as in the PREEMPT_RT case,
it isn't safe, and the pending softirqs mask can get corrupted,
resulting in softirqs being lost and never processed.

To have common code that works with PREEMPT_RT and with mainline Linux,
we can use plain napi_schedule() instead. The difference is that
napi_schedule() (via __napi_schedule) also calls local_irq_save, which
disables hardirqs if they aren't already. But, since they already are
disabled in non-RT, this means that in practice we don't see any
measurable difference in throughput or latency with this patch.

Signed-off-by: Jiafei Pan <Jiafei.Pan@nxp.com>
Signed-off-by: Vladimir Oltean <vladimir.oltean@nxp.com>
---
 drivers/net/ethernet/freescale/enetc/enetc.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/net/ethernet/freescale/enetc/enetc.c b/drivers/net/ethernet/freescale/enetc/enetc.c
index f50353cbb4db..f78ca7b343d2 100644
--- a/drivers/net/ethernet/freescale/enetc/enetc.c
+++ b/drivers/net/ethernet/freescale/enetc/enetc.c
@@ -270,7 +270,7 @@ static irqreturn_t enetc_msix(int irq, void *data)
 	for_each_set_bit(i, &v->tx_rings_map, ENETC_MAX_NUM_TXQS)
 		enetc_wr_reg(v->tbier_base + ENETC_BDR_OFF(i), 0);
 
-	napi_schedule_irqoff(&v->napi);
+	napi_schedule(&v->napi);
 
 	return IRQ_HANDLED;
 }
-- 
2.25.1

Re: [PATCH net-next 1/2] dpaa2-eth: use napi_schedule to be compatible with PREEMPT_RT

[David Miller]

From: Vladimir Oltean <olteanv@gmail.com>
Date: Mon,  3 Aug 2020 23:10:08 +0300

> From: Jiafei Pan <Jiafei.Pan@nxp.com>
> 
> The driver calls napi_schedule_irqoff() from a context where, in RT,
> hardirqs are not disabled, since the IRQ handler is force-threaded.
> 
> In the call path of this function, __raise_softirq_irqoff() is modifying
> its per-CPU mask of pending softirqs that must be processed, using
> or_softirq_pending(). The or_softirq_pending() function is not atomic,
> but since interrupts are supposed to be disabled, nobody should be
> preempting it, and the operation should be safe.
> 
> Nonetheless, when running with hardirqs on, as in the PREEMPT_RT case,
> it isn't safe, and the pending softirqs mask can get corrupted,
> resulting in softirqs being lost and never processed.
> 
> To have common code that works with PREEMPT_RT and with mainline Linux,
> we can use plain napi_schedule() instead. The difference is that
> napi_schedule() (via __napi_schedule) also calls local_irq_save, which
> disables hardirqs if they aren't already. But, since they already are
> disabled in non-RT, this means that in practice we don't see any
> measurable difference in throughput or latency with this patch.
> 
> Signed-off-by: Jiafei Pan <Jiafei.Pan@nxp.com>
> Signed-off-by: Vladimir Oltean <vladimir.oltean@nxp.com>

Applied.

Re: [PATCH net-next 2/2] enetc: use napi_schedule to be compatible with PREEMPT_RT

[David Miller]

From: Vladimir Oltean <olteanv@gmail.com>
Date: Mon,  3 Aug 2020 23:10:09 +0300

> From: Jiafei Pan <Jiafei.Pan@nxp.com>
> 
> The driver calls napi_schedule_irqoff() from a context where, in RT,
> hardirqs are not disabled, since the IRQ handler is force-threaded.
> 
> In the call path of this function, __raise_softirq_irqoff() is modifying
> its per-CPU mask of pending softirqs that must be processed, using
> or_softirq_pending(). The or_softirq_pending() function is not atomic,
> but since interrupts are supposed to be disabled, nobody should be
> preempting it, and the operation should be safe.
> 
> Nonetheless, when running with hardirqs on, as in the PREEMPT_RT case,
> it isn't safe, and the pending softirqs mask can get corrupted,
> resulting in softirqs being lost and never processed.
> 
> To have common code that works with PREEMPT_RT and with mainline Linux,
> we can use plain napi_schedule() instead. The difference is that
> napi_schedule() (via __napi_schedule) also calls local_irq_save, which
> disables hardirqs if they aren't already. But, since they already are
> disabled in non-RT, this means that in practice we don't see any
> measurable difference in throughput or latency with this patch.
> 
> Signed-off-by: Jiafei Pan <Jiafei.Pan@nxp.com>
> Signed-off-by: Vladimir Oltean <vladimir.oltean@nxp.com>

Applied.

Re: [PATCH net-next 2/2] enetc: use napi_schedule to be compatible with PREEMPT_RT

[Sebastian Andrzej Siewior]

On 2020-08-03 18:21:45 [-0700], David Miller wrote:
> From: Vladimir Oltean <olteanv@gmail.com>
> > The driver calls napi_schedule_irqoff() from a context where, in RT,
> > hardirqs are not disabled, since the IRQ handler is force-threaded.
…
> > 
> > Signed-off-by: Jiafei Pan <Jiafei.Pan@nxp.com>
> > Signed-off-by: Vladimir Oltean <vladimir.oltean@nxp.com>
> 
> Applied.

Could these two patches be forwarded -stable, please? The changelog
describes this as a problem on PREEMPT_RT but this also happens on !RT
with the `threadirqs' commandline switch.

Sebastian

Re: [PATCH net-next 2/2] enetc: use napi_schedule to be compatible with PREEMPT_RT

[Vladimir Oltean]

On Wed, Aug 12, 2020 at 03:51:44PM +0200, Sebastian Andrzej Siewior wrote:
> On 2020-08-03 18:21:45 [-0700], David Miller wrote:
> > From: Vladimir Oltean <olteanv@gmail.com>
> > > The driver calls napi_schedule_irqoff() from a context where, in RT,
> > > hardirqs are not disabled, since the IRQ handler is force-threaded.
> …
> > > 
> > > Signed-off-by: Jiafei Pan <Jiafei.Pan@nxp.com>
> > > Signed-off-by: Vladimir Oltean <vladimir.oltean@nxp.com>
> > 
> > Applied.
> 
> Could these two patches be forwarded -stable, please? The changelog
> describes this as a problem on PREEMPT_RT but this also happens on !RT
> with the `threadirqs' commandline switch.
> 
> Sebastian

I expect the driver maintainers to have something to say about this. I
didn't test on stable kernels, and at least for dpaa2-eth, the change
would need to go pretty deep down the stable line.

Also, not really sure who is using the threadirqs option except for
testing purposes.

Thanks,
-Vladimir

Re: [PATCH net-next 2/2] enetc: use napi_schedule to be compatible with PREEMPT_RT

[Sebastian Andrzej Siewior]

On 2020-08-12 17:34:30 [+0300], Vladimir Oltean wrote:
> I expect the driver maintainers to have something to say about this. I
> didn't test on stable kernels, and at least for dpaa2-eth, the change
> would need to go pretty deep down the stable line.

Yes, each affected and maintained stable kernel. This would also ensure
that it is part stable-RT trees.

> Also, not really sure who is using the threadirqs option except for
> testing purposes.

Oh.

> Thanks,
> -Vladimir

Sebastian

Re: [PATCH net-next 2/2] enetc: use napi_schedule to be compatible with PREEMPT_RT

[Vladimir Oltean]

Hi Sasha,

On Wed, Aug 12, 2020 at 05:34:30PM +0300, Vladimir Oltean wrote:
> On Wed, Aug 12, 2020 at 03:51:44PM +0200, Sebastian Andrzej Siewior wrote:
> > On 2020-08-03 18:21:45 [-0700], David Miller wrote:
> > > From: Vladimir Oltean <olteanv@gmail.com>
> > > > The driver calls napi_schedule_irqoff() from a context where, in RT,
> > > > hardirqs are not disabled, since the IRQ handler is force-threaded.
> > …
> > > > 
> > > > Signed-off-by: Jiafei Pan <Jiafei.Pan@nxp.com>
> > > > Signed-off-by: Vladimir Oltean <vladimir.oltean@nxp.com>
> > > 
> > > Applied.
> > 
> > Could these two patches be forwarded -stable, please? The changelog
> > describes this as a problem on PREEMPT_RT but this also happens on !RT
> > with the `threadirqs' commandline switch.
> > 
> > Sebastian
> 
> I expect the driver maintainers to have something to say about this. I
> didn't test on stable kernels, and at least for dpaa2-eth, the change
> would need to go pretty deep down the stable line.
> 
> Also, not really sure who is using the threadirqs option except for
> testing purposes.
> 
> Thanks,
> -Vladimir

Do you think that this type of request is something that AUTOSEL can
handle?

Thanks,
-Vladimir