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

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

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.

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

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

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

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