| Message ID | 20201027225454.3492351-1-bigeasy@linutronix.de |
|---|---|
| Headers | show |
| Series | in_interrupt() cleanup, part 2 | expand |
On Tue, 27 Oct 2020 23:54:39 +0100 Sebastian Andrzej Siewior wrote: > Folks, > > in the discussion about preempt count consistency across kernel configurations: > > https://lore.kernel.org/r/20200914204209.256266093@linutronix.de/ > > Linus clearly requested that code in drivers and libraries which changes > behaviour based on execution context should either be split up so that > e.g. task context invocations and BH invocations have different interfaces > or if that's not possible the context information has to be provided by the > caller which knows in which context it is executing. > > This includes conditional locking, allocation mode (GFP_*) decisions and > avoidance of code paths which might sleep. > > In the long run, usage of 'preemptible, in_*irq etc.' should be banned from > driver code completely. > > This is part two addressing remaining drivers except for orinoco-usb. Freescale folks - can I get an ack for merging the last three patches into net-next?
On Tue, 27 Oct 2020 23:54:43 +0100 Sebastian Andrzej Siewior wrote: > mlx5_eq_async_int() uses in_irq() to decide whether eq::lock needs to be > acquired and released with spin_[un]lock() or the irq saving/restoring > variants. > > The usage of in_*() in drivers is phased out and Linus clearly requested > that code which changes behaviour depending on context should either be > seperated or the context be conveyed in an argument passed by the caller, > which usually knows the context. > > mlx5_eq_async_int() knows the context via the action argument already so > using it for the lock variant decision is a straight forward replacement > for in_irq(). > > Signed-off-by: Sebastian Andrzej Siewior <bigeasy@linutronix.de> > Cc: Saeed Mahameed <saeedm@nvidia.com> > Cc: Leon Romanovsky <leon@kernel.org> > Cc: "David S. Miller" <davem@davemloft.net> > Cc: Jakub Kicinski <kuba@kernel.org> > Cc: linux-rdma@vger.kernel.org Saeed, please pick this up into your tree.
On Tue, 27 Oct 2020 23:54:39 +0100 Sebastian Andrzej Siewior wrote: > Folks, > > in the discussion about preempt count consistency across kernel configurations: > > https://lore.kernel.org/r/20200914204209.256266093@linutronix.de/ > > Linus clearly requested that code in drivers and libraries which changes > behaviour based on execution context should either be split up so that > e.g. task context invocations and BH invocations have different interfaces > or if that's not possible the context information has to be provided by the > caller which knows in which context it is executing. > > This includes conditional locking, allocation mode (GFP_*) decisions and > avoidance of code paths which might sleep. > > In the long run, usage of 'preemptible, in_*irq etc.' should be banned from > driver code completely. > > This is part two addressing remaining drivers except for orinoco-usb. Sebastian, thanks for there, I picked up only the Ethernet patches: 5ce7f3f46f6b net: neterion: s2io: Replace in_interrupt() for context detection dc5e8bfcd12e net: forcedeth: Replace context and lock check with a lockdep_assert() beca92820dc4 net: tlan: Replace in_irq() usage Please repost the wireless stuff for Kalle to linux-wireless@vger, and repost the fsl stuff separately (our patchwork queue is huge, I can't keep this waiting for an ack any longer).
On Tue, 27 Oct 2020 23:54:53 +0100 Sebastian Andrzej Siewior wrote: > The driver uses in_irq() + in_serving_softirq() magic to decide if NAPI > scheduling is required or packet processing. > > The usage of in_*() in drivers is phased out and Linus clearly requested > that code which changes behaviour depending on context should either be > seperated or the context be conveyed in an argument passed by the caller, > which usually knows the context. > > Use the `napi' argument passed by the callback. It is set true if > called from the interrupt handler and NAPI should be scheduled. > > Signed-off-by: Sebastian Andrzej Siewior <bigeasy@linutronix.de> > Cc: "Horia Geantă" <horia.geanta@nxp.com> > Cc: Aymen Sghaier <aymen.sghaier@nxp.com> > Cc: Herbert Xu <herbert@gondor.apana.org.au> > Cc: "David S. Miller" <davem@davemloft.net> > Cc: Madalin Bucur <madalin.bucur@nxp.com> > Cc: Jakub Kicinski <kuba@kernel.org> > Cc: Li Yang <leoyang.li@nxp.com> > Cc: linux-crypto@vger.kernel.org > Cc: netdev@vger.kernel.org > Cc: linuxppc-dev@lists.ozlabs.org > Cc: linux-arm-kernel@lists.infradead.org > --- > drivers/net/ethernet/freescale/dpaa/dpaa_eth.c | 12 ++++++------ > 1 file changed, 6 insertions(+), 6 deletions(-) > > diff --git a/drivers/net/ethernet/freescale/dpaa/dpaa_eth.c b/drivers/net/ethernet/freescale/dpaa/dpaa_eth.c > index 27835310b718e..2c949acd74c67 100644 > --- a/drivers/net/ethernet/freescale/dpaa/dpaa_eth.c > +++ b/drivers/net/ethernet/freescale/dpaa/dpaa_eth.c > @@ -2300,9 +2300,9 @@ static void dpaa_tx_conf(struct net_device *net_dev, > } > > static inline int dpaa_eth_napi_schedule(struct dpaa_percpu_priv *percpu_priv, > - struct qman_portal *portal) > + struct qman_portal *portal, bool napi) > { > - if (unlikely(in_irq() || !in_serving_softirq())) { > + if (napi) { > /* Disable QMan IRQ and invoke NAPI */ > qman_p_irqsource_remove(portal, QM_PIRQ_DQRI); > Nit: some networking drivers have a bool napi which means "are we running in napi context", the semantics here feel a little backwards, at least to me. But if I'm the only one thinking this, so be it.
On 2020-10-31 10:12:15 [-0700], Jakub Kicinski wrote: > Nit: some networking drivers have a bool napi which means "are we > running in napi context", the semantics here feel a little backwards, > at least to me. But if I'm the only one thinking this, so be it. I renamed it to `sched_napi'. Sebastian
On Sat, 2020-10-31 at 09:59 -0700, Jakub Kicinski wrote: > On Tue, 27 Oct 2020 23:54:43 +0100 Sebastian Andrzej Siewior wrote: > > mlx5_eq_async_int() uses in_irq() to decide whether eq::lock needs > > to be > > acquired and released with spin_[un]lock() or the irq > > saving/restoring > > variants. > > > > The usage of in_*() in drivers is phased out and Linus clearly > > requested > > that code which changes behaviour depending on context should > > either be > > seperated or the context be conveyed in an argument passed by the > > caller, > > which usually knows the context. > > > > mlx5_eq_async_int() knows the context via the action argument > > already so > > using it for the lock variant decision is a straight forward > > replacement > > for in_irq(). > > > > Signed-off-by: Sebastian Andrzej Siewior <bigeasy@linutronix.de> > > Cc: Saeed Mahameed <saeedm@nvidia.com> > > Cc: Leon Romanovsky <leon@kernel.org> > > Cc: "David S. Miller" <davem@davemloft.net> > > Cc: Jakub Kicinski <kuba@kernel.org> > > Cc: linux-rdma@vger.kernel.org > > Saeed, please pick this up into your tree. Ack
On Sat, 2020-10-31 at 09:59 -0700, Jakub Kicinski wrote: > On Tue, 27 Oct 2020 23:54:43 +0100 Sebastian Andrzej Siewior wrote: > > mlx5_eq_async_int() uses in_irq() to decide whether eq::lock needs > > to be > > acquired and released with spin_[un]lock() or the irq > > saving/restoring > > variants. > > > > The usage of in_*() in drivers is phased out and Linus clearly > > requested > > that code which changes behaviour depending on context should > > either be > > seperated or the context be conveyed in an argument passed by the > > caller, > > which usually knows the context. > > > > mlx5_eq_async_int() knows the context via the action argument > > already so > > using it for the lock variant decision is a straight forward > > replacement > > for in_irq(). > > > > Signed-off-by: Sebastian Andrzej Siewior <bigeasy@linutronix.de> > > Cc: Saeed Mahameed <saeedm@nvidia.com> > > Cc: Leon Romanovsky <leon@kernel.org> > > Cc: "David S. Miller" <davem@davemloft.net> > > Cc: Jakub Kicinski <kuba@kernel.org> > > Cc: linux-rdma@vger.kernel.org > > Saeed, please pick this up into your tree. Applied to net-next-mlx5 will submit to net-next shortly.