Message ID | 20200803201009.613147-2-olteanv@gmail.com |
---|---|
State | Accepted |
Delegated to: | David Miller |
Headers | show |
Series | [net-next,1/2] dpaa2-eth: use napi_schedule to be compatible with PREEMPT_RT | expand |
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
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; }