[net-next PATCH 1/3] net: introduce napi_is_scheduled helper

[net-next PATCH 1/3] net: introduce napi_is_scheduled helper

[Christian Marangi]

We currently have napi_if_scheduled_mark_missed that can be used to
check if napi is scheduled but that does more thing than simply checking
it and return a bool. Some driver already implement custom function to
check if napi is scheduled.

Drop these custom function and introduce napi_is_scheduled that simply
check if napi is scheduled atomically.

Update any driver and code that implement a similar check and instead
use this new helper.

Signed-off-by: Christian Marangi <ansuelsmth@gmail.com>
---
 drivers/net/ethernet/chelsio/cxgb3/sge.c  | 8 --------
 drivers/net/wireless/realtek/rtw89/core.c | 2 +-
 include/linux/netdevice.h                 | 5 +++++
 net/core/dev.c                            | 2 +-
 4 files changed, 7 insertions(+), 10 deletions(-)

diff --git a/drivers/net/ethernet/chelsio/cxgb3/sge.c b/drivers/net/ethernet/chelsio/cxgb3/sge.c
index 2e9a74fe0970..71fa2dc19034 100644
--- a/drivers/net/ethernet/chelsio/cxgb3/sge.c
+++ b/drivers/net/ethernet/chelsio/cxgb3/sge.c
@@ -2501,14 +2501,6 @@ static int napi_rx_handler(struct napi_struct *napi, int budget)
 	return work_done;
 }
 
-/*
- * Returns true if the device is already scheduled for polling.
- */
-static inline int napi_is_scheduled(struct napi_struct *napi)
-{
-	return test_bit(NAPI_STATE_SCHED, &napi->state);
-}
-
 /**
  *	process_pure_responses - process pure responses from a response queue
  *	@adap: the adapter
diff --git a/drivers/net/wireless/realtek/rtw89/core.c b/drivers/net/wireless/realtek/rtw89/core.c
index 133bf289bacb..bbf4ea3639d4 100644
--- a/drivers/net/wireless/realtek/rtw89/core.c
+++ b/drivers/net/wireless/realtek/rtw89/core.c
@@ -1744,7 +1744,7 @@ static void rtw89_core_rx_to_mac80211(struct rtw89_dev *rtwdev,
 	struct napi_struct *napi = &rtwdev->napi;
 
 	/* In low power mode, napi isn't scheduled. Receive it to netif. */
-	if (unlikely(!test_bit(NAPI_STATE_SCHED, &napi->state)))
+	if (unlikely(!napi_is_scheduled(napi)))
 		napi = NULL;
 
 	rtw89_core_hw_to_sband_rate(rx_status);
diff --git a/include/linux/netdevice.h b/include/linux/netdevice.h
index db3d8429d50d..8eac00cd3b92 100644
--- a/include/linux/netdevice.h
+++ b/include/linux/netdevice.h
@@ -482,6 +482,11 @@ static inline bool napi_prefer_busy_poll(struct napi_struct *n)
 	return test_bit(NAPI_STATE_PREFER_BUSY_POLL, &n->state);
 }
 
+static inline bool napi_is_scheduled(struct napi_struct *n)
+{
+	return test_bit(NAPI_STATE_SCHED, &n->state);
+}
+
 bool napi_schedule_prep(struct napi_struct *n);
 
 /**
diff --git a/net/core/dev.c b/net/core/dev.c
index cc03a5758d2d..32ba8002f65a 100644
--- a/net/core/dev.c
+++ b/net/core/dev.c
@@ -6523,7 +6523,7 @@ static int __napi_poll(struct napi_struct *n, bool *repoll)
 	 * accidentally calling ->poll() when NAPI is not scheduled.
 	 */
 	work = 0;
-	if (test_bit(NAPI_STATE_SCHED, &n->state)) {
+	if (napi_is_scheduled(n)) {
 		work = n->poll(n, weight);
 		trace_napi_poll(n, work, weight);
 	}
-- 
2.40.1

[net-next PATCH 2/3] net: stmmac: improve TX timer arm logic

[Christian Marangi]

There is currently a problem with the TX timer getting armed multiple
unnecessary times causing big performance regression on some device that
suffer from heavy handling of hrtimer rearm.

The use of the TX timer is an old implementation that predates the napi
implementation and the interrupt enable/disable handling.

Due to stmmac being a very old code, the TX timer was never evaluated
again with this new implementation and was kept there causing
performance regression. The performance regression started to appear
with kernel version 4.19 with 8fce33317023 ("net: stmmac: Rework coalesce
timer and fix multi-queue races") where the timer was reduced to 1ms
causing it to be armed 40 times more than before.

Decreasing the timer made the problem more present and caused the
regression in the other of 600-700mbps on some device (regression where
this was notice is ipq806x).

The problem is in the fact that handling the hrtimer on some target is
expensive and recent kernel made the timer armed much more times.
A solution that was proposed was reverting the hrtimer change and use
mod_timer but such solution would still hide the real problem in the
current implementation.

To fix the regression, apply some additional logic and skip arming the
timer when not needed.

Arm the timer ONLY if a napi is not already scheduled. Running the timer
is redundant since the same function (stmmac_tx_clean) will run in the
napi TX poll. Also try to cancel any timer if a napi is scheduled to
prevent redundant run of TX call.

With the following new logic the original performance are restored while
keeping using the hrtimer.

Signed-off-by: Christian Marangi <ansuelsmth@gmail.com>
---
 .../net/ethernet/stmicro/stmmac/stmmac_main.c  | 18 +++++++++++++++---
 1 file changed, 15 insertions(+), 3 deletions(-)

diff --git a/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c b/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c
index 9201ed778ebc..14bf6fae6662 100644
--- a/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c
+++ b/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c
@@ -2994,13 +2994,25 @@ static void stmmac_tx_timer_arm(struct stmmac_priv *priv, u32 queue)
 {
 	struct stmmac_tx_queue *tx_q = &priv->dma_conf.tx_queue[queue];
 	u32 tx_coal_timer = priv->tx_coal_timer[queue];
+	struct stmmac_channel *ch;
+	struct napi_struct *napi;
 
 	if (!tx_coal_timer)
 		return;
 
-	hrtimer_start(&tx_q->txtimer,
-		      STMMAC_COAL_TIMER(tx_coal_timer),
-		      HRTIMER_MODE_REL);
+	ch = &priv->channel[tx_q->queue_index];
+	napi = tx_q->xsk_pool ? &ch->rxtx_napi : &ch->tx_napi;
+
+	/* Arm timer only if napi is not already scheduled.
+	 * Try to cancel any timer if napi is scheduled, timer will be armed
+	 * again in the next scheduled napi.
+	 */
+	if (unlikely(!napi_is_scheduled(napi)))
+		hrtimer_start(&tx_q->txtimer,
+			      STMMAC_COAL_TIMER(tx_coal_timer),
+			      HRTIMER_MODE_REL);
+	else
+		hrtimer_try_to_cancel(&tx_q->txtimer);
 }
 
 /**
-- 
2.40.1

Re: [net-next PATCH 2/3] net: stmmac: improve TX timer arm logic

[Vincent Whitchurch]

On Fri, 2023-09-22 at 13:12 +0200, Christian Marangi wrote:
> diff --git a/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c b/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c
> index 9201ed778ebc..14bf6fae6662 100644
> --- a/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c
> +++ b/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c
> @@ -2994,13 +2994,25 @@ static void stmmac_tx_timer_arm(struct stmmac_priv *priv, u32 queue)
>  {
>  	struct stmmac_tx_queue *tx_q = &priv->dma_conf.tx_queue[queue];
>  	u32 tx_coal_timer = priv->tx_coal_timer[queue];
> +	struct stmmac_channel *ch;
> +	struct napi_struct *napi;
>  
> 
>  	if (!tx_coal_timer)
>  		return;
>  
> 
> -	hrtimer_start(&tx_q->txtimer,
> -		      STMMAC_COAL_TIMER(tx_coal_timer),
> -		      HRTIMER_MODE_REL);
> +	ch = &priv->channel[tx_q->queue_index];
> +	napi = tx_q->xsk_pool ? &ch->rxtx_napi : &ch->tx_napi;
> +
> +	/* Arm timer only if napi is not already scheduled.
> +	 * Try to cancel any timer if napi is scheduled, timer will be armed
> +	 * again in the next scheduled napi.
> +	 */
> +	if (unlikely(!napi_is_scheduled(napi)))
> +		hrtimer_start(&tx_q->txtimer,
> +			      STMMAC_COAL_TIMER(tx_coal_timer),
> +			      HRTIMER_MODE_REL);
> +	else
> +		hrtimer_try_to_cancel(&tx_q->txtimer);

When this function is called from within the napi poll function
(stmmac_tx_clean()), NAPI_STATE_SCHED will always be set and so after
this patch the "We still have pending packets, let's call for a new
scheduling" logic will never start the timer.  Was that really
intentional?

Re: [net-next PATCH 2/3] net: stmmac: improve TX timer arm logic

[Christian Marangi]

On Fri, Sep 29, 2023 at 12:38:48PM +0000, Vincent Whitchurch wrote:
> On Fri, 2023-09-22 at 13:12 +0200, Christian Marangi wrote:
> > diff --git a/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c b/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c
> > index 9201ed778ebc..14bf6fae6662 100644
> > --- a/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c
> > +++ b/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c
> > @@ -2994,13 +2994,25 @@ static void stmmac_tx_timer_arm(struct stmmac_priv *priv, u32 queue)
> >  {
> >  	struct stmmac_tx_queue *tx_q = &priv->dma_conf.tx_queue[queue];
> >  	u32 tx_coal_timer = priv->tx_coal_timer[queue];
> > +	struct stmmac_channel *ch;
> > +	struct napi_struct *napi;
> >  
> > 
> >  	if (!tx_coal_timer)
> >  		return;
> >  
> > 
> > -	hrtimer_start(&tx_q->txtimer,
> > -		      STMMAC_COAL_TIMER(tx_coal_timer),
> > -		      HRTIMER_MODE_REL);
> > +	ch = &priv->channel[tx_q->queue_index];
> > +	napi = tx_q->xsk_pool ? &ch->rxtx_napi : &ch->tx_napi;
> > +
> > +	/* Arm timer only if napi is not already scheduled.
> > +	 * Try to cancel any timer if napi is scheduled, timer will be armed
> > +	 * again in the next scheduled napi.
> > +	 */
> > +	if (unlikely(!napi_is_scheduled(napi)))
> > +		hrtimer_start(&tx_q->txtimer,
> > +			      STMMAC_COAL_TIMER(tx_coal_timer),
> > +			      HRTIMER_MODE_REL);
> > +	else
> > +		hrtimer_try_to_cancel(&tx_q->txtimer);
> 
> When this function is called from within the napi poll function
> (stmmac_tx_clean()), NAPI_STATE_SCHED will always be set and so after
> this patch the "We still have pending packets, let's call for a new
> scheduling" logic will never start the timer.  Was that really
> intentional?
>

No and understanding the code flow of napi and tx-coal is hard... (also
problem with tx coal arise only with real world scenario and now with
synthetic tests like iperf.

I will shortly send a v2 of this that will just move the logic of arming
the TX timer outside napi call after DMA interrupt is enabled again.
Currently testing the new version on openwrt with ipq806x hoping
everything is good.

(same perf increase observed but no queue timeout)

-- 
	Ansuel

[net-next PATCH 3/3] net: stmmac: increase TX coalesce timer to 5ms

[Christian Marangi]

Commit 8fce33317023 ("net: stmmac: Rework coalesce timer and fix
multi-queue races") decreased the TX coalesce timer from 40ms to 1ms.

This caused some performance regression on some target (regression was
reported at least on ipq806x) in the order of 600mbps dropping from
gigabit handling to only 200mbps.

The problem was identified in the TX timer getting armed too much time.
While this was fixed and improved in another commit, performance can be
improved even further by increasing the timer delay a bit moving from
1ms to 5ms.

The value is a good balance between battery saving by prevending too
much interrupt to be generated and permitting good performance for
internet oriented devices.

Signed-off-by: Christian Marangi <ansuelsmth@gmail.com>
---
 drivers/net/ethernet/stmicro/stmmac/common.h | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/net/ethernet/stmicro/stmmac/common.h b/drivers/net/ethernet/stmicro/stmmac/common.h
index 403cb397d4d3..2d9f895c2193 100644
--- a/drivers/net/ethernet/stmicro/stmmac/common.h
+++ b/drivers/net/ethernet/stmicro/stmmac/common.h
@@ -290,7 +290,7 @@ struct stmmac_safety_stats {
 #define MIN_DMA_RIWT		0x10
 #define DEF_DMA_RIWT		0xa0
 /* Tx coalesce parameters */
-#define STMMAC_COAL_TX_TIMER	1000
+#define STMMAC_COAL_TX_TIMER	5000
 #define STMMAC_MAX_COAL_TX_TICK	100000
 #define STMMAC_TX_MAX_FRAMES	256
 #define STMMAC_TX_FRAMES	25
-- 
2.40.1

Re: [net-next PATCH 3/3] net: stmmac: increase TX coalesce timer to 5ms

[Andrew Lunn]

On Fri, Sep 22, 2023 at 01:12:47PM +0200, Christian Marangi wrote:
> Commit 8fce33317023 ("net: stmmac: Rework coalesce timer and fix
> multi-queue races") decreased the TX coalesce timer from 40ms to 1ms.
> 
> This caused some performance regression on some target (regression was
> reported at least on ipq806x) in the order of 600mbps dropping from
> gigabit handling to only 200mbps.
> 
> The problem was identified in the TX timer getting armed too much time.
> While this was fixed and improved in another commit, performance can be
> improved even further by increasing the timer delay a bit moving from
> 1ms to 5ms.
> 
> The value is a good balance between battery saving by prevending too
> much interrupt to be generated and permitting good performance for
> internet oriented devices.

ethtool has a settings you can use for this:

      ethtool -C|--coalesce devname [adaptive-rx on|off] [adaptive-tx on|off]
              [rx-usecs N] [rx-frames N] [rx-usecs-irq N] [rx-frames-irq N]
              [tx-usecs N] [tx-frames N] [tx-usecs-irq N] [tx-frames-irq N]
              [stats-block-usecs N] [pkt-rate-low N] [rx-usecs-low N]
              [rx-frames-low N] [tx-usecs-low N] [tx-frames-low N]
              [pkt-rate-high N] [rx-usecs-high N] [rx-frames-high N]
              [tx-usecs-high N] [tx-frames-high N] [sample-interval N]
              [cqe-mode-rx on|off] [cqe-mode-tx on|off] [tx-aggr-max-bytes N]
              [tx-aggr-max-frames N] [tx-aggr-time-usecs N]

If this is not implemented, i suggest you add support for it.

Changing the default might cause regressions. Say there is a VoIP
application which wants this low latency? It would be safer to allow
user space to configure it as wanted.

     Andrew

Re: [net-next PATCH 3/3] net: stmmac: increase TX coalesce timer to 5ms

[Christian Marangi]

On Fri, Sep 22, 2023 at 02:28:06PM +0200, Andrew Lunn wrote:
> On Fri, Sep 22, 2023 at 01:12:47PM +0200, Christian Marangi wrote:
> > Commit 8fce33317023 ("net: stmmac: Rework coalesce timer and fix
> > multi-queue races") decreased the TX coalesce timer from 40ms to 1ms.
> > 
> > This caused some performance regression on some target (regression was
> > reported at least on ipq806x) in the order of 600mbps dropping from
> > gigabit handling to only 200mbps.
> > 
> > The problem was identified in the TX timer getting armed too much time.
> > While this was fixed and improved in another commit, performance can be
> > improved even further by increasing the timer delay a bit moving from
> > 1ms to 5ms.
> > 
> > The value is a good balance between battery saving by prevending too
> > much interrupt to be generated and permitting good performance for
> > internet oriented devices.
> 
> ethtool has a settings you can use for this:
> 
>       ethtool -C|--coalesce devname [adaptive-rx on|off] [adaptive-tx on|off]
>               [rx-usecs N] [rx-frames N] [rx-usecs-irq N] [rx-frames-irq N]
>               [tx-usecs N] [tx-frames N] [tx-usecs-irq N] [tx-frames-irq N]
>               [stats-block-usecs N] [pkt-rate-low N] [rx-usecs-low N]
>               [rx-frames-low N] [tx-usecs-low N] [tx-frames-low N]
>               [pkt-rate-high N] [rx-usecs-high N] [rx-frames-high N]
>               [tx-usecs-high N] [tx-frames-high N] [sample-interval N]
>               [cqe-mode-rx on|off] [cqe-mode-tx on|off] [tx-aggr-max-bytes N]
>               [tx-aggr-max-frames N] [tx-aggr-time-usecs N]
> 
> If this is not implemented, i suggest you add support for it.
> 
> Changing the default might cause regressions. Say there is a VoIP
> application which wants this low latency? It would be safer to allow
> user space to configure it as wanted.
>

Yep stmmac already support it. Idea here was to not fallback to use
ethtool and find a good value.

Just for reference before one commit, the value was set to 40ms and
nobody ever pointed out regression about VoIP application. Wtih some
testing I found 5ms a small increase that restore original perf and
should not cause any regression.

(for reference keeping this to 1ms cause a lost of about 100-200mbps)
(also the tx timer implementation was created before any napi poll logic
and before dma interrupt handling was a thing, with the later change I
expect this timer to be very little used in VoIP scenario or similar
with continuous traffic as napi will take care of handling packet)

Aside from these reason I totally get the concern and totally ok with
this not getting applied, was just an idea to push for a common value.

Just preferred to handle this here instead of script+userspace :(
(the important part is the previous patch)

-- 
	Ansuel

Re: [net-next PATCH 3/3] net: stmmac: increase TX coalesce timer to 5ms

[Dave Taht]

On Fri, Sep 22, 2023 at 5:39 AM Christian Marangi <ansuelsmth@gmail.com> wrote:
>
> On Fri, Sep 22, 2023 at 02:28:06PM +0200, Andrew Lunn wrote:
> > On Fri, Sep 22, 2023 at 01:12:47PM +0200, Christian Marangi wrote:
> > > Commit 8fce33317023 ("net: stmmac: Rework coalesce timer and fix
> > > multi-queue races") decreased the TX coalesce timer from 40ms to 1ms.
> > >
> > > This caused some performance regression on some target (regression was
> > > reported at least on ipq806x) in the order of 600mbps dropping from
> > > gigabit handling to only 200mbps.
> > >
> > > The problem was identified in the TX timer getting armed too much time.
> > > While this was fixed and improved in another commit, performance can be
> > > improved even further by increasing the timer delay a bit moving from
> > > 1ms to 5ms.

I am always looking for finding ways to improve interrupt service
time, rather than paper over the problem by increasing batchi-ness.

http://www.taht.net/~d/broadcom_aug9_2018.pdf

But also looking for hard data, particularly as to observed power
savings. How much power does upping this number save?

I have tried to question other assumptions more modern kernels are
making, in particular I wish more folk would experience with
decreasing the overlarge (IMHO) NAPI default of 64 packets to, say 8
in the mq case, benefiting from multiple arm cores still equipped with
limited cache, as well as looking at the impact of TLB flushes. Other
deferred multi-core processing... that is looking good on a modern
xeon, but might not be so good on a more limited arm, worries me.

Over here there was an enormous test series recently run against a
bunch of older arm64s which appears to indicate that memory bandwidth
is a source of problems:

https://docs.google.com/document/d/1HxIU_TEBI6xG9jRHlr8rzyyxFEN43zMcJXUFlRuhiUI/edit

We are looking to add more devices to that testbed.

> > >
> > > The value is a good balance between battery saving by prevending too
> > > much interrupt to be generated and permitting good performance for
> > > internet oriented devices.
> >
> > ethtool has a settings you can use for this:
> >
> >       ethtool -C|--coalesce devname [adaptive-rx on|off] [adaptive-tx on|off]
> >               [rx-usecs N] [rx-frames N] [rx-usecs-irq N] [rx-frames-irq N]
> >               [tx-usecs N] [tx-frames N] [tx-usecs-irq N] [tx-frames-irq N]
> >               [stats-block-usecs N] [pkt-rate-low N] [rx-usecs-low N]
> >               [rx-frames-low N] [tx-usecs-low N] [tx-frames-low N]
> >               [pkt-rate-high N] [rx-usecs-high N] [rx-frames-high N]
> >               [tx-usecs-high N] [tx-frames-high N] [sample-interval N]
> >               [cqe-mode-rx on|off] [cqe-mode-tx on|off] [tx-aggr-max-bytes N]
> >               [tx-aggr-max-frames N] [tx-aggr-time-usecs N]
> >
> > If this is not implemented, i suggest you add support for it.
> >
> > Changing the default might cause regressions. Say there is a VoIP
> > application which wants this low latency? It would be safer to allow
> > user space to configure it as wanted.
> >
>
> Yep stmmac already support it. Idea here was to not fallback to use
> ethtool and find a good value.
>
> Just for reference before one commit, the value was set to 40ms and
> nobody ever pointed out regression about VoIP application. Wtih some
> testing I found 5ms a small increase that restore original perf and
> should not cause any regression.

Does this driver have BQL?

> (for reference keeping this to 1ms cause a lost of about 100-200mbps)
> (also the tx timer implementation was created before any napi poll logic
> and before dma interrupt handling was a thing, with the later change I
> expect this timer to be very little used in VoIP scenario or similar
> with continuous traffic as napi will take care of handling packet)

I would be pretty interested in a kernel flame graph of the before vs the after.

> Aside from these reason I totally get the concern and totally ok with
> this not getting applied, was just an idea to push for a common value.

I try to get people to run much longer and more complicated tests such
as the flent rrul test to see what kind of damage bigger buffers did
to latency, as well as how other problems might show up. Really
notable in the above test series was how badly various devices behaved
over time on that workload. Extremely notable in that test series
above was how badly the  jetson performed:

https://github.com/randomizedcoder/cake/blob/2023_09_02/pfifo_fast/jetson.png

And the nanopi was weird.

https://github.com/randomizedcoder/cake/blob/2023_09_02/pfifo_fast/nanopi-neo3.png

> Just preferred to handle this here instead of script+userspace :(
> (the important part is the previous patch)
>
> --
>         Ansuel
>
--
Oct 30: https://netdevconf.info/0x17/news/the-maestro-and-the-music-bof.html
Dave Täht CSO, LibreQos

RE: [net-next PATCH 1/3] net: introduce napi_is_scheduled helper

[Nambiar, Amritha]

> -----Original Message-----
> From: Christian Marangi <ansuelsmth@gmail.com>
> Sent: Friday, September 22, 2023 4:13 AM
> To: Vincent Whitchurch <vincent.whitchurch@axis.com>; Raju Rangoju
> <rajur@chelsio.com>; David S. Miller <davem@davemloft.net>; Eric Dumazet
> <edumazet@google.com>; Jakub Kicinski <kuba@kernel.org>; Paolo Abeni
> <pabeni@redhat.com>; Alexandre Torgue <alexandre.torgue@foss.st.com>;
> Jose Abreu <joabreu@synopsys.com>; Maxime Coquelin
> <mcoquelin.stm32@gmail.com>; Ping-Ke Shih <pkshih@realtek.com>; Kalle
> Valo <kvalo@kernel.org>; Simon Horman <horms@kernel.org>; Daniel
> Borkmann <daniel@iogearbox.net>; Jiri Pirko <jiri@resnulli.us>; Hangbin Liu
> <liuhangbin@gmail.com>; netdev@vger.kernel.org; linux-
> kernel@vger.kernel.org; linux-stm32@st-md-mailman.stormreply.com; linux-
> arm-kernel@lists.infradead.org; linux-wireless@vger.kernel.org
> Cc: Christian Marangi <ansuelsmth@gmail.com>
> Subject: [net-next PATCH 1/3] net: introduce napi_is_scheduled helper
> 
> We currently have napi_if_scheduled_mark_missed that can be used to
> check if napi is scheduled but that does more thing than simply checking
> it and return a bool. Some driver already implement custom function to
> check if napi is scheduled.
> 
> Drop these custom function and introduce napi_is_scheduled that simply
> check if napi is scheduled atomically.
> 
> Update any driver and code that implement a similar check and instead
> use this new helper.
> 
> Signed-off-by: Christian Marangi <ansuelsmth@gmail.com>

Looks good to me.
Reviewed-by: Amritha Nambiar <amritha.nambiar@intel.com>

> ---
>  drivers/net/ethernet/chelsio/cxgb3/sge.c  | 8 --------
>  drivers/net/wireless/realtek/rtw89/core.c | 2 +-
>  include/linux/netdevice.h                 | 5 +++++
>  net/core/dev.c                            | 2 +-
>  4 files changed, 7 insertions(+), 10 deletions(-)
> 
> diff --git a/drivers/net/ethernet/chelsio/cxgb3/sge.c
> b/drivers/net/ethernet/chelsio/cxgb3/sge.c
> index 2e9a74fe0970..71fa2dc19034 100644
> --- a/drivers/net/ethernet/chelsio/cxgb3/sge.c
> +++ b/drivers/net/ethernet/chelsio/cxgb3/sge.c
> @@ -2501,14 +2501,6 @@ static int napi_rx_handler(struct napi_struct *napi,
> int budget)
>  	return work_done;
>  }
> 
> -/*
> - * Returns true if the device is already scheduled for polling.
> - */
> -static inline int napi_is_scheduled(struct napi_struct *napi)
> -{
> -	return test_bit(NAPI_STATE_SCHED, &napi->state);
> -}
> -
>  /**
>   *	process_pure_responses - process pure responses from a response
> queue
>   *	@adap: the adapter
> diff --git a/drivers/net/wireless/realtek/rtw89/core.c
> b/drivers/net/wireless/realtek/rtw89/core.c
> index 133bf289bacb..bbf4ea3639d4 100644
> --- a/drivers/net/wireless/realtek/rtw89/core.c
> +++ b/drivers/net/wireless/realtek/rtw89/core.c
> @@ -1744,7 +1744,7 @@ static void rtw89_core_rx_to_mac80211(struct
> rtw89_dev *rtwdev,
>  	struct napi_struct *napi = &rtwdev->napi;
> 
>  	/* In low power mode, napi isn't scheduled. Receive it to netif. */
> -	if (unlikely(!test_bit(NAPI_STATE_SCHED, &napi->state)))
> +	if (unlikely(!napi_is_scheduled(napi)))
>  		napi = NULL;
> 
>  	rtw89_core_hw_to_sband_rate(rx_status);
> diff --git a/include/linux/netdevice.h b/include/linux/netdevice.h
> index db3d8429d50d..8eac00cd3b92 100644
> --- a/include/linux/netdevice.h
> +++ b/include/linux/netdevice.h
> @@ -482,6 +482,11 @@ static inline bool napi_prefer_busy_poll(struct
> napi_struct *n)
>  	return test_bit(NAPI_STATE_PREFER_BUSY_POLL, &n->state);
>  }
> 
> +static inline bool napi_is_scheduled(struct napi_struct *n)
> +{
> +	return test_bit(NAPI_STATE_SCHED, &n->state);
> +}
> +
>  bool napi_schedule_prep(struct napi_struct *n);
> 
>  /**
> diff --git a/net/core/dev.c b/net/core/dev.c
> index cc03a5758d2d..32ba8002f65a 100644
> --- a/net/core/dev.c
> +++ b/net/core/dev.c
> @@ -6523,7 +6523,7 @@ static int __napi_poll(struct napi_struct *n, bool
> *repoll)
>  	 * accidentally calling ->poll() when NAPI is not scheduled.
>  	 */
>  	work = 0;
> -	if (test_bit(NAPI_STATE_SCHED, &n->state)) {
> +	if (napi_is_scheduled(n)) {
>  		work = n->poll(n, weight);
>  		trace_napi_poll(n, work, weight);
>  	}
> --
> 2.40.1
> 

Re: [net-next PATCH 1/3] net: introduce napi_is_scheduled helper

[Eric Dumazet]

On Fri, Sep 22, 2023 at 1:13 PM Christian Marangi <ansuelsmth@gmail.com> wrote:
>
> We currently have napi_if_scheduled_mark_missed that can be used to
> check if napi is scheduled but that does more thing than simply checking
> it and return a bool. Some driver already implement custom function to
> check if napi is scheduled.
>
> Drop these custom function and introduce napi_is_scheduled that simply
> check if napi is scheduled atomically.
>
> Update any driver and code that implement a similar check and instead
> use this new helper.
>
> Signed-off-by: Christian Marangi <ansuelsmth@gmail.com>
> ---
>  drivers/net/ethernet/chelsio/cxgb3/sge.c  | 8 --------
>  drivers/net/wireless/realtek/rtw89/core.c | 2 +-
>  include/linux/netdevice.h                 | 5 +++++
>  net/core/dev.c                            | 2 +-
>  4 files changed, 7 insertions(+), 10 deletions(-)
>
> diff --git a/drivers/net/ethernet/chelsio/cxgb3/sge.c b/drivers/net/ethernet/chelsio/cxgb3/sge.c
> index 2e9a74fe0970..71fa2dc19034 100644
> --- a/drivers/net/ethernet/chelsio/cxgb3/sge.c
> +++ b/drivers/net/ethernet/chelsio/cxgb3/sge.c
> @@ -2501,14 +2501,6 @@ static int napi_rx_handler(struct napi_struct *napi, int budget)
>         return work_done;
>  }
>
> -/*
> - * Returns true if the device is already scheduled for polling.
> - */
> -static inline int napi_is_scheduled(struct napi_struct *napi)
> -{
> -       return test_bit(NAPI_STATE_SCHED, &napi->state);
> -}
> -
>  /**
>   *     process_pure_responses - process pure responses from a response queue
>   *     @adap: the adapter
> diff --git a/drivers/net/wireless/realtek/rtw89/core.c b/drivers/net/wireless/realtek/rtw89/core.c
> index 133bf289bacb..bbf4ea3639d4 100644
> --- a/drivers/net/wireless/realtek/rtw89/core.c
> +++ b/drivers/net/wireless/realtek/rtw89/core.c
> @@ -1744,7 +1744,7 @@ static void rtw89_core_rx_to_mac80211(struct rtw89_dev *rtwdev,
>         struct napi_struct *napi = &rtwdev->napi;
>
>         /* In low power mode, napi isn't scheduled. Receive it to netif. */
> -       if (unlikely(!test_bit(NAPI_STATE_SCHED, &napi->state)))
> +       if (unlikely(!napi_is_scheduled(napi)))
>                 napi = NULL;
>
>         rtw89_core_hw_to_sband_rate(rx_status);
> diff --git a/include/linux/netdevice.h b/include/linux/netdevice.h
> index db3d8429d50d..8eac00cd3b92 100644
> --- a/include/linux/netdevice.h
> +++ b/include/linux/netdevice.h
> @@ -482,6 +482,11 @@ static inline bool napi_prefer_busy_poll(struct napi_struct *n)
>         return test_bit(NAPI_STATE_PREFER_BUSY_POLL, &n->state);
>  }
>


In which context is it safe to call this helper ?

I fear that making this available will add more bugs.

For instance rspq_check_napi() seems buggy to me.

> +static inline bool napi_is_scheduled(struct napi_struct *n)

const ...

> +{
> +       return test_bit(NAPI_STATE_SCHED, &n->state);
> +}
> +
>  bool napi_schedule_prep(struct napi_struct *n);
>
>  /**
> diff --git a/net/core/dev.c b/net/core/dev.c
> index cc03a5758d2d..32ba8002f65a 100644
> --- a/net/core/dev.c
> +++ b/net/core/dev.c
> @@ -6523,7 +6523,7 @@ static int __napi_poll(struct napi_struct *n, bool *repoll)
>          * accidentally calling ->poll() when NAPI is not scheduled.
>          */
>         work = 0;
> -       if (test_bit(NAPI_STATE_SCHED, &n->state)) {
> +       if (napi_is_scheduled(n)) {
>                 work = n->poll(n, weight);
>                 trace_napi_poll(n, work, weight);
>         }
> --
> 2.40.1
>

Re: [net-next PATCH 1/3] net: introduce napi_is_scheduled helper

[Christian Marangi]

On Sat, Sep 30, 2023 at 01:59:53PM +0200, Eric Dumazet wrote:
> On Fri, Sep 22, 2023 at 1:13 PM Christian Marangi <ansuelsmth@gmail.com> wrote:
> >
> > We currently have napi_if_scheduled_mark_missed that can be used to
> > check if napi is scheduled but that does more thing than simply checking
> > it and return a bool. Some driver already implement custom function to
> > check if napi is scheduled.
> >
> > Drop these custom function and introduce napi_is_scheduled that simply
> > check if napi is scheduled atomically.
> >
> > Update any driver and code that implement a similar check and instead
> > use this new helper.
> >
> > Signed-off-by: Christian Marangi <ansuelsmth@gmail.com>
> > ---
> >  drivers/net/ethernet/chelsio/cxgb3/sge.c  | 8 --------
> >  drivers/net/wireless/realtek/rtw89/core.c | 2 +-
> >  include/linux/netdevice.h                 | 5 +++++
> >  net/core/dev.c                            | 2 +-
> >  4 files changed, 7 insertions(+), 10 deletions(-)
> >
> > diff --git a/drivers/net/ethernet/chelsio/cxgb3/sge.c b/drivers/net/ethernet/chelsio/cxgb3/sge.c
> > index 2e9a74fe0970..71fa2dc19034 100644
> > --- a/drivers/net/ethernet/chelsio/cxgb3/sge.c
> > +++ b/drivers/net/ethernet/chelsio/cxgb3/sge.c
> > @@ -2501,14 +2501,6 @@ static int napi_rx_handler(struct napi_struct *napi, int budget)
> >         return work_done;
> >  }
> >
> > -/*
> > - * Returns true if the device is already scheduled for polling.
> > - */
> > -static inline int napi_is_scheduled(struct napi_struct *napi)
> > -{
> > -       return test_bit(NAPI_STATE_SCHED, &napi->state);
> > -}
> > -
> >  /**
> >   *     process_pure_responses - process pure responses from a response queue
> >   *     @adap: the adapter
> > diff --git a/drivers/net/wireless/realtek/rtw89/core.c b/drivers/net/wireless/realtek/rtw89/core.c
> > index 133bf289bacb..bbf4ea3639d4 100644
> > --- a/drivers/net/wireless/realtek/rtw89/core.c
> > +++ b/drivers/net/wireless/realtek/rtw89/core.c
> > @@ -1744,7 +1744,7 @@ static void rtw89_core_rx_to_mac80211(struct rtw89_dev *rtwdev,
> >         struct napi_struct *napi = &rtwdev->napi;
> >
> >         /* In low power mode, napi isn't scheduled. Receive it to netif. */
> > -       if (unlikely(!test_bit(NAPI_STATE_SCHED, &napi->state)))
> > +       if (unlikely(!napi_is_scheduled(napi)))
> >                 napi = NULL;
> >
> >         rtw89_core_hw_to_sband_rate(rx_status);
> > diff --git a/include/linux/netdevice.h b/include/linux/netdevice.h
> > index db3d8429d50d..8eac00cd3b92 100644
> > --- a/include/linux/netdevice.h
> > +++ b/include/linux/netdevice.h
> > @@ -482,6 +482,11 @@ static inline bool napi_prefer_busy_poll(struct napi_struct *n)
> >         return test_bit(NAPI_STATE_PREFER_BUSY_POLL, &n->state);
> >  }
> >
> 
> 
> In which context is it safe to call this helper ?
>

test_bit is atomic so it should be always safe. Also the idea of this
check (and from what I can see this apply also to the other 2 user) is
somehow best effort, we check if in the current istant there is a napi
scheduled and we act.

> I fear that making this available will add more bugs.
> 
> For instance rspq_check_napi() seems buggy to me.
> 

Mhhh why? Am I opening a can of worms?

> > +static inline bool napi_is_scheduled(struct napi_struct *n)
> 
> const ...
> 

Will change in v2. Thanks!

> > +{
> > +       return test_bit(NAPI_STATE_SCHED, &n->state);
> > +}
> > +
> >  bool napi_schedule_prep(struct napi_struct *n);
> >
> >  /**
> > diff --git a/net/core/dev.c b/net/core/dev.c
> > index cc03a5758d2d..32ba8002f65a 100644
> > --- a/net/core/dev.c
> > +++ b/net/core/dev.c
> > @@ -6523,7 +6523,7 @@ static int __napi_poll(struct napi_struct *n, bool *repoll)
> >          * accidentally calling ->poll() when NAPI is not scheduled.
> >          */
> >         work = 0;
> > -       if (test_bit(NAPI_STATE_SCHED, &n->state)) {
> > +       if (napi_is_scheduled(n)) {
> >                 work = n->poll(n, weight);
> >                 trace_napi_poll(n, work, weight);
> >         }
> > --
> > 2.40.1
> >

-- 
	Ansuel

Re: [net-next PATCH 1/3] net: introduce napi_is_scheduled helper

[Eric Dumazet]

On Sat, Sep 30, 2023 at 2:11 PM Christian Marangi <ansuelsmth@gmail.com> wrote:
>
> On Sat, Sep 30, 2023 at 01:59:53PM +0200, Eric Dumazet wrote:
> > On Fri, Sep 22, 2023 at 1:13 PM Christian Marangi <ansuelsmth@gmail.com> wrote:
> > >
> > > We currently have napi_if_scheduled_mark_missed that can be used to
> > > check if napi is scheduled but that does more thing than simply checking
> > > it and return a bool. Some driver already implement custom function to
> > > check if napi is scheduled.
> > >
> > > Drop these custom function and introduce napi_is_scheduled that simply
> > > check if napi is scheduled atomically.
> > >
> > > Update any driver and code that implement a similar check and instead
> > > use this new helper.
> > >
> > > Signed-off-by: Christian Marangi <ansuelsmth@gmail.com>
> > > ---
> > >  drivers/net/ethernet/chelsio/cxgb3/sge.c  | 8 --------
> > >  drivers/net/wireless/realtek/rtw89/core.c | 2 +-
> > >  include/linux/netdevice.h                 | 5 +++++
> > >  net/core/dev.c                            | 2 +-
> > >  4 files changed, 7 insertions(+), 10 deletions(-)
> > >
> > > diff --git a/drivers/net/ethernet/chelsio/cxgb3/sge.c b/drivers/net/ethernet/chelsio/cxgb3/sge.c
> > > index 2e9a74fe0970..71fa2dc19034 100644
> > > --- a/drivers/net/ethernet/chelsio/cxgb3/sge.c
> > > +++ b/drivers/net/ethernet/chelsio/cxgb3/sge.c
> > > @@ -2501,14 +2501,6 @@ static int napi_rx_handler(struct napi_struct *napi, int budget)
> > >         return work_done;
> > >  }
> > >
> > > -/*
> > > - * Returns true if the device is already scheduled for polling.
> > > - */
> > > -static inline int napi_is_scheduled(struct napi_struct *napi)
> > > -{
> > > -       return test_bit(NAPI_STATE_SCHED, &napi->state);
> > > -}
> > > -
> > >  /**
> > >   *     process_pure_responses - process pure responses from a response queue
> > >   *     @adap: the adapter
> > > diff --git a/drivers/net/wireless/realtek/rtw89/core.c b/drivers/net/wireless/realtek/rtw89/core.c
> > > index 133bf289bacb..bbf4ea3639d4 100644
> > > --- a/drivers/net/wireless/realtek/rtw89/core.c
> > > +++ b/drivers/net/wireless/realtek/rtw89/core.c
> > > @@ -1744,7 +1744,7 @@ static void rtw89_core_rx_to_mac80211(struct rtw89_dev *rtwdev,
> > >         struct napi_struct *napi = &rtwdev->napi;
> > >
> > >         /* In low power mode, napi isn't scheduled. Receive it to netif. */
> > > -       if (unlikely(!test_bit(NAPI_STATE_SCHED, &napi->state)))
> > > +       if (unlikely(!napi_is_scheduled(napi)))
> > >                 napi = NULL;
> > >
> > >         rtw89_core_hw_to_sband_rate(rx_status);
> > > diff --git a/include/linux/netdevice.h b/include/linux/netdevice.h
> > > index db3d8429d50d..8eac00cd3b92 100644
> > > --- a/include/linux/netdevice.h
> > > +++ b/include/linux/netdevice.h
> > > @@ -482,6 +482,11 @@ static inline bool napi_prefer_busy_poll(struct napi_struct *n)
> > >         return test_bit(NAPI_STATE_PREFER_BUSY_POLL, &n->state);
> > >  }
> > >
> >
> >
> > In which context is it safe to call this helper ?
> >
>
> test_bit is atomic so it should be always safe. Also the idea of this
> check (and from what I can see this apply also to the other 2 user) is
> somehow best effort, we check if in the current istant there is a napi
> scheduled and we act.

I think testing a bit here is not enough to take any kind of useful decision,
unless used in a particular context.

>
> > I fear that making this available will add more bugs.
> >
> > For instance rspq_check_napi() seems buggy to me.
> >
>
> Mhhh why? Am I opening a can of worms?

Yes I think :/

Because only the thread that has grabbed the bit can make any sense of it.

Another thread reading it would not really know if the value is not going to
change immediately. So what would be the point ?

It seems rspq_check_napi() real intent was maybe the following,
but really this is hard to know if the current race in this code is okay or not.

diff --git a/drivers/net/ethernet/chelsio/cxgb3/sge.c
b/drivers/net/ethernet/chelsio/cxgb3/sge.c
index 2e9a74fe0970df333226b80af8716f30865c01b7..e153c9590b36b38e430bc93660146b428e9b3347
100644
--- a/drivers/net/ethernet/chelsio/cxgb3/sge.c
+++ b/drivers/net/ethernet/chelsio/cxgb3/sge.c
@@ -2676,8 +2676,10 @@ static int rspq_check_napi(struct sge_qset *qs)

        if (!napi_is_scheduled(&qs->napi) &&
            is_new_response(&q->desc[q->cidx], q)) {
-               napi_schedule(&qs->napi);
-               return 1;
+               if (napi_schedule_prep(&qs->napi)) {
+                       __napi_schedule(&qs->napi);
+                       return 1;
+               }
        }
        return 0;
 }

Re: [net-next PATCH 1/3] net: introduce napi_is_scheduled helper

[Christian Marangi]

On Sat, Sep 30, 2023 at 03:42:20PM +0200, Eric Dumazet wrote:
> On Sat, Sep 30, 2023 at 2:11 PM Christian Marangi <ansuelsmth@gmail.com> wrote:
> >
> > On Sat, Sep 30, 2023 at 01:59:53PM +0200, Eric Dumazet wrote:
> > > On Fri, Sep 22, 2023 at 1:13 PM Christian Marangi <ansuelsmth@gmail.com> wrote:
> > > >
> > > > We currently have napi_if_scheduled_mark_missed that can be used to
> > > > check if napi is scheduled but that does more thing than simply checking
> > > > it and return a bool. Some driver already implement custom function to
> > > > check if napi is scheduled.
> > > >
> > > > Drop these custom function and introduce napi_is_scheduled that simply
> > > > check if napi is scheduled atomically.
> > > >
> > > > Update any driver and code that implement a similar check and instead
> > > > use this new helper.
> > > >
> > > > Signed-off-by: Christian Marangi <ansuelsmth@gmail.com>
> > > > ---
> > > >  drivers/net/ethernet/chelsio/cxgb3/sge.c  | 8 --------
> > > >  drivers/net/wireless/realtek/rtw89/core.c | 2 +-
> > > >  include/linux/netdevice.h                 | 5 +++++
> > > >  net/core/dev.c                            | 2 +-
> > > >  4 files changed, 7 insertions(+), 10 deletions(-)
> > > >
> > > > diff --git a/drivers/net/ethernet/chelsio/cxgb3/sge.c b/drivers/net/ethernet/chelsio/cxgb3/sge.c
> > > > index 2e9a74fe0970..71fa2dc19034 100644
> > > > --- a/drivers/net/ethernet/chelsio/cxgb3/sge.c
> > > > +++ b/drivers/net/ethernet/chelsio/cxgb3/sge.c
> > > > @@ -2501,14 +2501,6 @@ static int napi_rx_handler(struct napi_struct *napi, int budget)
> > > >         return work_done;
> > > >  }
> > > >
> > > > -/*
> > > > - * Returns true if the device is already scheduled for polling.
> > > > - */
> > > > -static inline int napi_is_scheduled(struct napi_struct *napi)
> > > > -{
> > > > -       return test_bit(NAPI_STATE_SCHED, &napi->state);
> > > > -}
> > > > -
> > > >  /**
> > > >   *     process_pure_responses - process pure responses from a response queue
> > > >   *     @adap: the adapter
> > > > diff --git a/drivers/net/wireless/realtek/rtw89/core.c b/drivers/net/wireless/realtek/rtw89/core.c
> > > > index 133bf289bacb..bbf4ea3639d4 100644
> > > > --- a/drivers/net/wireless/realtek/rtw89/core.c
> > > > +++ b/drivers/net/wireless/realtek/rtw89/core.c
> > > > @@ -1744,7 +1744,7 @@ static void rtw89_core_rx_to_mac80211(struct rtw89_dev *rtwdev,
> > > >         struct napi_struct *napi = &rtwdev->napi;
> > > >
> > > >         /* In low power mode, napi isn't scheduled. Receive it to netif. */
> > > > -       if (unlikely(!test_bit(NAPI_STATE_SCHED, &napi->state)))
> > > > +       if (unlikely(!napi_is_scheduled(napi)))
> > > >                 napi = NULL;
> > > >
> > > >         rtw89_core_hw_to_sband_rate(rx_status);
> > > > diff --git a/include/linux/netdevice.h b/include/linux/netdevice.h
> > > > index db3d8429d50d..8eac00cd3b92 100644
> > > > --- a/include/linux/netdevice.h
> > > > +++ b/include/linux/netdevice.h
> > > > @@ -482,6 +482,11 @@ static inline bool napi_prefer_busy_poll(struct napi_struct *n)
> > > >         return test_bit(NAPI_STATE_PREFER_BUSY_POLL, &n->state);
> > > >  }
> > > >
> > >
> > >
> > > In which context is it safe to call this helper ?
> > >
> >
> > test_bit is atomic so it should be always safe. Also the idea of this
> > check (and from what I can see this apply also to the other 2 user) is
> > somehow best effort, we check if in the current istant there is a napi
> > scheduled and we act.
> 
> I think testing a bit here is not enough to take any kind of useful decision,
> unless used in a particular context.
>

Ehhh the idea here was to reduce code duplication since the very same
test will be done in stmmac. So I guess this code cleanup is a NACK and
I have to duplicate the test in the stmmac driver.

> >
> > > I fear that making this available will add more bugs.
> > >
> > > For instance rspq_check_napi() seems buggy to me.
> > >
> >
> > Mhhh why? Am I opening a can of worms?
> 
> Yes I think :/
> 
> Because only the thread that has grabbed the bit can make any sense of it.
> 
> Another thread reading it would not really know if the value is not going to
> change immediately. So what would be the point ?
> 
> It seems rspq_check_napi() real intent was maybe the following,
> but really this is hard to know if the current race in this code is okay or not.
> 
> diff --git a/drivers/net/ethernet/chelsio/cxgb3/sge.c
> b/drivers/net/ethernet/chelsio/cxgb3/sge.c
> index 2e9a74fe0970df333226b80af8716f30865c01b7..e153c9590b36b38e430bc93660146b428e9b3347
> 100644
> --- a/drivers/net/ethernet/chelsio/cxgb3/sge.c
> +++ b/drivers/net/ethernet/chelsio/cxgb3/sge.c
> @@ -2676,8 +2676,10 @@ static int rspq_check_napi(struct sge_qset *qs)
> 
>         if (!napi_is_scheduled(&qs->napi) &&
>             is_new_response(&q->desc[q->cidx], q)) {
> -               napi_schedule(&qs->napi);
> -               return 1;
> +               if (napi_schedule_prep(&qs->napi)) {
> +                       __napi_schedule(&qs->napi);
> +                       return 1;
> +               }
>         }
>         return 0;
>  }

-- 
	Ansuel

Re: [net-next PATCH 1/3] net: introduce napi_is_scheduled helper

[Eric Dumazet]

On Mon, Oct 2, 2023 at 2:29 PM Christian Marangi <ansuelsmth@gmail.com> wrote:

> Ehhh the idea here was to reduce code duplication since the very same
> test will be done in stmmac. So I guess this code cleanup is a NACK and
> I have to duplicate the test in the stmmac driver.

I simply wanted to add a comment in front of this function/helper,
advising not using it unless absolutely needed.

Thus my question "In which context is it safe to call this helper ?"

As long as it was private with a driver, I did not mind.

But if made public in include/linux/netdevice.h, I would rather not
have to explain
to future users why it can be problematic.

Re: [net-next PATCH 1/3] net: introduce napi_is_scheduled helper

[Christian Marangi]

On Mon, Oct 02, 2023 at 02:35:22PM +0200, Eric Dumazet wrote:
> On Mon, Oct 2, 2023 at 2:29 PM Christian Marangi <ansuelsmth@gmail.com> wrote:
> 
> > Ehhh the idea here was to reduce code duplication since the very same
> > test will be done in stmmac. So I guess this code cleanup is a NACK and
> > I have to duplicate the test in the stmmac driver.
> 
> I simply wanted to add a comment in front of this function/helper,
> advising not using it unless absolutely needed.
> 
> Thus my question "In which context is it safe to call this helper ?"
> 
> As long as it was private with a driver, I did not mind.
> 
> But if made public in include/linux/netdevice.h, I would rather not
> have to explain
> to future users why it can be problematic.

Oh ok!

We have plenty of case similar to this. (example some clock API very
internal that should not be used normally or regmap related)

I will include some comments warning that this should not be used in
normal circumstances and other warnings. If you have suggestion on what
to add feel free to write them.

Any clue on how to proceed with the sge driver? 

-- 
	Ansuel

Re: [net-next PATCH 1/3] net: introduce napi_is_scheduled helper

[Eric Dumazet]

On Mon, Oct 2, 2023 at 2:43 PM Christian Marangi <ansuelsmth@gmail.com> wrote:
>
> On Mon, Oct 02, 2023 at 02:35:22PM +0200, Eric Dumazet wrote:
> > On Mon, Oct 2, 2023 at 2:29 PM Christian Marangi <ansuelsmth@gmail.com> wrote:
> >
> > > Ehhh the idea here was to reduce code duplication since the very same
> > > test will be done in stmmac. So I guess this code cleanup is a NACK and
> > > I have to duplicate the test in the stmmac driver.
> >
> > I simply wanted to add a comment in front of this function/helper,
> > advising not using it unless absolutely needed.
> >
> > Thus my question "In which context is it safe to call this helper ?"
> >
> > As long as it was private with a driver, I did not mind.
> >
> > But if made public in include/linux/netdevice.h, I would rather not
> > have to explain
> > to future users why it can be problematic.
>
> Oh ok!
>
> We have plenty of case similar to this. (example some clock API very
> internal that should not be used normally or regmap related)
>
> I will include some comments warning that this should not be used in
> normal circumstances and other warnings. If you have suggestion on what
> to add feel free to write them.
>
> Any clue on how to proceed with the sge driver?
>

I would remove use of this helper for something with no race ?

Feel free to submit this :

(Alternative would be to change napi_schedule() to return a boolean)

diff --git a/drivers/net/ethernet/chelsio/cxgb3/sge.c
b/drivers/net/ethernet/chelsio/cxgb3/sge.c
index 2e9a74fe0970df333226b80af8716f30865c01b7..09d0e6aa4db982e3488e0c28bed33e83453801d0
100644
--- a/drivers/net/ethernet/chelsio/cxgb3/sge.c
+++ b/drivers/net/ethernet/chelsio/cxgb3/sge.c
@@ -2501,14 +2501,6 @@ static int napi_rx_handler(struct napi_struct
*napi, int budget)
        return work_done;
 }

-/*
- * Returns true if the device is already scheduled for polling.
- */
-static inline int napi_is_scheduled(struct napi_struct *napi)
-{
-       return test_bit(NAPI_STATE_SCHED, &napi->state);
-}
-
 /**
  *     process_pure_responses - process pure responses from a response queue
  *     @adap: the adapter
@@ -2674,9 +2666,9 @@ static int rspq_check_napi(struct sge_qset *qs)
 {
        struct sge_rspq *q = &qs->rspq;

-       if (!napi_is_scheduled(&qs->napi) &&
-           is_new_response(&q->desc[q->cidx], q)) {
-               napi_schedule(&qs->napi);
+       if (is_new_response(&q->desc[q->cidx], q) &&
+           napi_schedule_prep(&qs->napi)) {
+               __napi_schedule(&qs->napi);
                return 1;
        }
        return 0;

Re: [net-next PATCH 1/3] net: introduce napi_is_scheduled helper

[Christian Marangi]

On Mon, Oct 02, 2023 at 02:49:11PM +0200, Eric Dumazet wrote:
> On Mon, Oct 2, 2023 at 2:43 PM Christian Marangi <ansuelsmth@gmail.com> wrote:
> >
> > On Mon, Oct 02, 2023 at 02:35:22PM +0200, Eric Dumazet wrote:
> > > On Mon, Oct 2, 2023 at 2:29 PM Christian Marangi <ansuelsmth@gmail.com> wrote:
> > >
> > > > Ehhh the idea here was to reduce code duplication since the very same
> > > > test will be done in stmmac. So I guess this code cleanup is a NACK and
> > > > I have to duplicate the test in the stmmac driver.
> > >
> > > I simply wanted to add a comment in front of this function/helper,
> > > advising not using it unless absolutely needed.
> > >
> > > Thus my question "In which context is it safe to call this helper ?"
> > >
> > > As long as it was private with a driver, I did not mind.
> > >
> > > But if made public in include/linux/netdevice.h, I would rather not
> > > have to explain
> > > to future users why it can be problematic.
> >
> > Oh ok!
> >
> > We have plenty of case similar to this. (example some clock API very
> > internal that should not be used normally or regmap related)
> >
> > I will include some comments warning that this should not be used in
> > normal circumstances and other warnings. If you have suggestion on what
> > to add feel free to write them.
> >
> > Any clue on how to proceed with the sge driver?
> >
> 
> I would remove use of this helper for something with no race ?
> 
> Feel free to submit this :
> 
> (Alternative would be to change napi_schedule() to return a boolean)
>

Think mod napi_schedule() to return a bool would result in massive
warning (actually error with werror) with return value not handled.

I will submit with your Suggested-by. Ok for you?

> diff --git a/drivers/net/ethernet/chelsio/cxgb3/sge.c
> b/drivers/net/ethernet/chelsio/cxgb3/sge.c
> index 2e9a74fe0970df333226b80af8716f30865c01b7..09d0e6aa4db982e3488e0c28bed33e83453801d0
> 100644
> --- a/drivers/net/ethernet/chelsio/cxgb3/sge.c
> +++ b/drivers/net/ethernet/chelsio/cxgb3/sge.c
> @@ -2501,14 +2501,6 @@ static int napi_rx_handler(struct napi_struct
> *napi, int budget)
>         return work_done;
>  }
> 
> -/*
> - * Returns true if the device is already scheduled for polling.
> - */
> -static inline int napi_is_scheduled(struct napi_struct *napi)
> -{
> -       return test_bit(NAPI_STATE_SCHED, &napi->state);
> -}
> -
>  /**
>   *     process_pure_responses - process pure responses from a response queue
>   *     @adap: the adapter
> @@ -2674,9 +2666,9 @@ static int rspq_check_napi(struct sge_qset *qs)
>  {
>         struct sge_rspq *q = &qs->rspq;
> 
> -       if (!napi_is_scheduled(&qs->napi) &&
> -           is_new_response(&q->desc[q->cidx], q)) {
> -               napi_schedule(&qs->napi);
> +       if (is_new_response(&q->desc[q->cidx], q) &&
> +           napi_schedule_prep(&qs->napi)) {
> +               __napi_schedule(&qs->napi);
>                 return 1;
>         }
>         return 0;

-- 
	Ansuel

Re: [net-next PATCH 1/3] net: introduce napi_is_scheduled helper

[Eric Dumazet]

On Mon, Oct 2, 2023 at 2:55 PM Christian Marangi <ansuelsmth@gmail.com> wrote:
>
> On Mon, Oct 02, 2023 at 02:49:11PM +0200, Eric Dumazet wrote:
> > On Mon, Oct 2, 2023 at 2:43 PM Christian Marangi <ansuelsmth@gmail.com> wrote:
> > >
> > > On Mon, Oct 02, 2023 at 02:35:22PM +0200, Eric Dumazet wrote:
> > > > On Mon, Oct 2, 2023 at 2:29 PM Christian Marangi <ansuelsmth@gmail.com> wrote:
> > > >
> > > > > Ehhh the idea here was to reduce code duplication since the very same
> > > > > test will be done in stmmac. So I guess this code cleanup is a NACK and
> > > > > I have to duplicate the test in the stmmac driver.
> > > >
> > > > I simply wanted to add a comment in front of this function/helper,
> > > > advising not using it unless absolutely needed.
> > > >
> > > > Thus my question "In which context is it safe to call this helper ?"
> > > >
> > > > As long as it was private with a driver, I did not mind.
> > > >
> > > > But if made public in include/linux/netdevice.h, I would rather not
> > > > have to explain
> > > > to future users why it can be problematic.
> > >
> > > Oh ok!
> > >
> > > We have plenty of case similar to this. (example some clock API very
> > > internal that should not be used normally or regmap related)
> > >
> > > I will include some comments warning that this should not be used in
> > > normal circumstances and other warnings. If you have suggestion on what
> > > to add feel free to write them.
> > >
> > > Any clue on how to proceed with the sge driver?
> > >
> >
> > I would remove use of this helper for something with no race ?
> >
> > Feel free to submit this :
> >
> > (Alternative would be to change napi_schedule() to return a boolean)
> >
>
> Think mod napi_schedule() to return a bool would result in massive
> warning (actually error with werror) with return value not handled.
>

It should not, unless we added a __must_check

> I will submit with your Suggested-by. Ok for you?

Absolutely, thanks.

>
> > diff --git a/drivers/net/ethernet/chelsio/cxgb3/sge.c
> > b/drivers/net/ethernet/chelsio/cxgb3/sge.c
> > index 2e9a74fe0970df333226b80af8716f30865c01b7..09d0e6aa4db982e3488e0c28bed33e83453801d0
> > 100644
> > --- a/drivers/net/ethernet/chelsio/cxgb3/sge.c
> > +++ b/drivers/net/ethernet/chelsio/cxgb3/sge.c
> > @@ -2501,14 +2501,6 @@ static int napi_rx_handler(struct napi_struct
> > *napi, int budget)
> >         return work_done;
> >  }
> >
> > -/*
> > - * Returns true if the device is already scheduled for polling.
> > - */
> > -static inline int napi_is_scheduled(struct napi_struct *napi)
> > -{
> > -       return test_bit(NAPI_STATE_SCHED, &napi->state);
> > -}
> > -
> >  /**
> >   *     process_pure_responses - process pure responses from a response queue
> >   *     @adap: the adapter
> > @@ -2674,9 +2666,9 @@ static int rspq_check_napi(struct sge_qset *qs)
> >  {
> >         struct sge_rspq *q = &qs->rspq;
> >
> > -       if (!napi_is_scheduled(&qs->napi) &&
> > -           is_new_response(&q->desc[q->cidx], q)) {
> > -               napi_schedule(&qs->napi);
> > +       if (is_new_response(&q->desc[q->cidx], q) &&
> > +           napi_schedule_prep(&qs->napi)) {
> > +               __napi_schedule(&qs->napi);
> >                 return 1;
> >         }
> >         return 0;
>
> --
>         Ansuel

Re: [net-next PATCH 1/3] net: introduce napi_is_scheduled helper

[Eric Dumazet]

On Mon, Oct 2, 2023 at 2:56 PM Eric Dumazet <edumazet@google.com> wrote:
>
> On Mon, Oct 2, 2023 at 2:55 PM Christian Marangi <ansuelsmth@gmail.com> wrote:
> >
> > On Mon, Oct 02, 2023 at 02:49:11PM +0200, Eric Dumazet wrote:
> > > On Mon, Oct 2, 2023 at 2:43 PM Christian Marangi <ansuelsmth@gmail.com> wrote:
> > > >
> > > > On Mon, Oct 02, 2023 at 02:35:22PM +0200, Eric Dumazet wrote:
> > > > > On Mon, Oct 2, 2023 at 2:29 PM Christian Marangi <ansuelsmth@gmail.com> wrote:
> > > > >
> > > > > > Ehhh the idea here was to reduce code duplication since the very same
> > > > > > test will be done in stmmac. So I guess this code cleanup is a NACK and
> > > > > > I have to duplicate the test in the stmmac driver.
> > > > >
> > > > > I simply wanted to add a comment in front of this function/helper,
> > > > > advising not using it unless absolutely needed.
> > > > >
> > > > > Thus my question "In which context is it safe to call this helper ?"
> > > > >
> > > > > As long as it was private with a driver, I did not mind.
> > > > >
> > > > > But if made public in include/linux/netdevice.h, I would rather not
> > > > > have to explain
> > > > > to future users why it can be problematic.
> > > >
> > > > Oh ok!
> > > >
> > > > We have plenty of case similar to this. (example some clock API very
> > > > internal that should not be used normally or regmap related)
> > > >
> > > > I will include some comments warning that this should not be used in
> > > > normal circumstances and other warnings. If you have suggestion on what
> > > > to add feel free to write them.
> > > >
> > > > Any clue on how to proceed with the sge driver?
> > > >
> > >
> > > I would remove use of this helper for something with no race ?
> > >
> > > Feel free to submit this :
> > >
> > > (Alternative would be to change napi_schedule() to return a boolean)
> > >
> >
> > Think mod napi_schedule() to return a bool would result in massive
> > warning (actually error with werror) with return value not handled.
> >
>
> It should not, unless we added a __must_check

This was what I was thinking :

diff --git a/include/linux/netdevice.h b/include/linux/netdevice.h
index e070a4540fbaf4a9cf310d5f53c4401840c72776..6aa2bc315411d1a0f7db314f1fbfb11aae7c31fe
100644
--- a/include/linux/netdevice.h
+++ b/include/linux/netdevice.h
@@ -491,10 +491,13 @@ bool napi_schedule_prep(struct napi_struct *n);
  * Schedule NAPI poll routine to be called if it is not already
  * running.
  */
-static inline void napi_schedule(struct napi_struct *n)
+static inline bool napi_schedule(struct napi_struct *n)
 {
-       if (napi_schedule_prep(n))
+       if (napi_schedule_prep(n)) {
                __napi_schedule(n);
+               return true;
+       }
+       return false;
 }

 /**