Re: [PATCH net-next] net: drop the weight argument from netif_napi_add

Re: [PATCH net-next] net: drop the weight argument from netif_napi_add

[Marc Kleine-Budde]

[-- Attachment #1: Type: text/plain, Size: 877 bytes --]

On 27.09.2022 06:27:53, Jakub Kicinski wrote:
> We tell driver developers to always pass NAPI_POLL_WEIGHT
> as the weight to netif_napi_add(). This may be confusing
> to newcomers, drop the weight argument, those who really
> need to tweak the weight can use netif_napi_add_weight().
> 
> Signed-off-by: Jakub Kicinski <kuba@kernel.org>
> ---
>  drivers/net/can/ctucanfd/ctucanfd_base.c      |  2 +-
>  drivers/net/can/ifi_canfd/ifi_canfd.c         |  2 +-
>  drivers/net/can/m_can/m_can.c                 |  3 +--

Acked-by: Marc Kleine-Budde <mkl@pengutronix.de> # for CAN

regards,
Marc

-- 
Pengutronix e.K.                 | Marc Kleine-Budde           |
Embedded Linux                   | https://www.pengutronix.de  |
Vertretung West/Dortmund         | Phone: +49-231-2826-924     |
Amtsgericht Hildesheim, HRA 2686 | Fax:   +49-5121-206917-5555 |

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

Re: [PATCH net-next] net: drop the weight argument from netif_napi_add

[Jason A. Donenfeld]

On Tue, Sep 27, 2022 at 06:27:53AM -0700, Jakub Kicinski wrote:
> We tell driver developers to always pass NAPI_POLL_WEIGHT
> as the weight to netif_napi_add(). This may be confusing
> to newcomers, drop the weight argument, those who really
> need to tweak the weight can use netif_napi_add_weight().
> 
> Signed-off-by: Jakub Kicinski <kuba@kernel.org>
> ---
>  drivers/net/wireguard/peer.c                  |  3 +--
>
> diff --git a/drivers/net/wireguard/peer.c b/drivers/net/wireguard/peer.c
> index 1acd00ab2fbc..1cb502a932e0 100644
> --- a/drivers/net/wireguard/peer.c
> +++ b/drivers/net/wireguard/peer.c
> @@ -54,8 +54,7 @@ struct wg_peer *wg_peer_create(struct wg_device *wg,
>  	skb_queue_head_init(&peer->staged_packet_queue);
>  	wg_noise_reset_last_sent_handshake(&peer->last_sent_handshake);
>  	set_bit(NAPI_STATE_NO_BUSY_POLL, &peer->napi.state);
> -	netif_napi_add(wg->dev, &peer->napi, wg_packet_rx_poll,
> -		       NAPI_POLL_WEIGHT);
> +	netif_napi_add(wg->dev, &peer->napi, wg_packet_rx_poll);
>  	napi_enable(&peer->napi);
>  	list_add_tail(&peer->peer_list, &wg->peer_list);
>  	INIT_LIST_HEAD(&peer->allowedips_list);

For the wireguard part,

   Acked-by: Jason A. Donenfeld <Jason@zx2c4.com>

Re: [PATCH net-next] net: drop the weight argument from netif_napi_add

[Eric Dumazet]

On Tue, Sep 27, 2022 at 6:28 AM Jakub Kicinski <kuba@kernel.org> wrote:
>
> We tell driver developers to always pass NAPI_POLL_WEIGHT
> as the weight to netif_napi_add(). This may be confusing
> to newcomers, drop the weight argument, those who really
> need to tweak the weight can use netif_napi_add_weight().
>
> Signed-off-by: Jakub Kicinski <kuba@kernel.org>

Sure, but this kind of patch makes backports harder.
Not sure how confused are newcomers about this NAPI_POLL_WEIGHT....

Re: [PATCH net-next] net: drop the weight argument from netif_napi_add

[Jakub Kicinski]

On Tue, 27 Sep 2022 10:54:49 -0700 Eric Dumazet wrote:
> On Tue, Sep 27, 2022 at 6:28 AM Jakub Kicinski <kuba@kernel.org> wrote:
> > We tell driver developers to always pass NAPI_POLL_WEIGHT
> > as the weight to netif_napi_add(). This may be confusing
> > to newcomers, drop the weight argument, those who really
> > need to tweak the weight can use netif_napi_add_weight().
> >
> > Signed-off-by: Jakub Kicinski <kuba@kernel.org>  
> 
> Sure, but this kind of patch makes backports harder.
> Not sure how confused are newcomers about this NAPI_POLL_WEIGHT....

I maintained this patch in my tree for a couple of releases (because 
I was waiting for the _weight() version to propagate to non-netdev
trees) and the conflicts were minor. Three or so cases of new features
added to drivers which touched the NAPI calls (WiFi and embedded) and
the strlcpy -> strscpy patch, and, well, why did we take that in if we
worry about backports...

NAPI weight was already dead when I started hacking on the kernel
10 years ago. I don't think it's reasonable to keep dead stuff 
in our APIs for backport's sake. Adding Jiri to CC in case I need
someone to back me up :)

The idea for this patch came because I was reviewing a driver which 
was trying to do something clever with the weight.

Re: [PATCH net-next] net: drop the weight argument from netif_napi_add

[Dave Taht]

On Tue, Sep 27, 2022 at 11:21 AM Eric Dumazet <edumazet@google.com> wrote:
>
> On Tue, Sep 27, 2022 at 6:28 AM Jakub Kicinski <kuba@kernel.org> wrote:
> >
> > We tell driver developers to always pass NAPI_POLL_WEIGHT
> > as the weight to netif_napi_add(). This may be confusing
> > to newcomers, drop the weight argument, those who really
> > need to tweak the weight can use netif_napi_add_weight().
> >
> > Signed-off-by: Jakub Kicinski <kuba@kernel.org>
>
> Sure, but this kind of patch makes backports harder.
> Not sure how confused are newcomers about this NAPI_POLL_WEIGHT....

Ironically we've been fiddling with dramatically reducing the
NAPI_POLL_WEIGHT (8) on
several multicore arm systems, with good results, especially on ath10k.


-- 
FQ World Domination pending: https://blog.cerowrt.org/post/state_of_fq_codel/
Dave Täht CEO, TekLibre, LLC

Re: [PATCH net-next] net: drop the weight argument from netif_napi_add

[Eric Dumazet]

On Tue, Sep 27, 2022 at 11:18 AM Jakub Kicinski <kuba@kernel.org> wrote:
>
> On Tue, 27 Sep 2022 10:54:49 -0700 Eric Dumazet wrote:
> > On Tue, Sep 27, 2022 at 6:28 AM Jakub Kicinski <kuba@kernel.org> wrote:
> > > We tell driver developers to always pass NAPI_POLL_WEIGHT
> > > as the weight to netif_napi_add(). This may be confusing
> > > to newcomers, drop the weight argument, those who really
> > > need to tweak the weight can use netif_napi_add_weight().
> > >
> > > Signed-off-by: Jakub Kicinski <kuba@kernel.org>
> >
> > Sure, but this kind of patch makes backports harder.
> > Not sure how confused are newcomers about this NAPI_POLL_WEIGHT....
>
> I maintained this patch in my tree for a couple of releases (because
> I was waiting for the _weight() version to propagate to non-netdev
> trees) and the conflicts were minor. Three or so cases of new features
> added to drivers which touched the NAPI calls (WiFi and embedded) and
> the strlcpy -> strscpy patch, and, well, why did we take that in if we
> worry about backports...
>
> NAPI weight was already dead when I started hacking on the kernel
> 10 years ago. I don't think it's reasonable to keep dead stuff
> in our APIs for backport's sake. Adding Jiri to CC in case I need
> someone to back me up :)
>
> The idea for this patch came because I was reviewing a driver which
> was trying to do something clever with the weight.
>

No hard feelings, but the recent removal of netif_tx_napi_add() added
extra work for some of us ;)

Keeping around few helpers to keep API a bit stable would help I think.

Re: [PATCH net-next] net: drop the weight argument from netif_napi_add

[patchwork-bot+netdevbpf]

Hello:

This patch was applied to netdev/net-next.git (master)
by Jakub Kicinski <kuba@kernel.org>:

On Tue, 27 Sep 2022 06:27:53 -0700 you wrote:
> We tell driver developers to always pass NAPI_POLL_WEIGHT
> as the weight to netif_napi_add(). This may be confusing
> to newcomers, drop the weight argument, those who really
> need to tweak the weight can use netif_napi_add_weight().
> 
> Signed-off-by: Jakub Kicinski <kuba@kernel.org>
> 
> [...]

Here is the summary with links:
  - [net-next] net: drop the weight argument from netif_napi_add
    https://git.kernel.org/netdev/net-next/c/b48b89f9c189

You are awesome, thank you!
-- 
Deet-doot-dot, I am a bot.
https://korg.docs.kernel.org/patchwork/pwbot.html

Re: [PATCH net-next] net: drop the weight argument from netif_napi_add

[Guenter Roeck]

On Tue, Sep 27, 2022 at 06:27:53AM -0700, Jakub Kicinski wrote:
> We tell driver developers to always pass NAPI_POLL_WEIGHT
> as the weight to netif_napi_add(). This may be confusing
> to newcomers, drop the weight argument, those who really
> need to tweak the weight can use netif_napi_add_weight().
> 
> Signed-off-by: Jakub Kicinski <kuba@kernel.org>

That seems to have missed some (or at least one) file(s).

Building mips:cavium_octeon_defconfig ... failed
--------------
Error log:
drivers/net/ethernet/cavium/octeon/octeon_mgmt.c: In function 'octeon_mgmt_probe':
drivers/net/ethernet/cavium/octeon/octeon_mgmt.c:1399:9: error: too many arguments to function 'netif_napi_add'
 1399 |         netif_napi_add(netdev, &p->napi, octeon_mgmt_napi_poll,
      |         ^~~~~~~~~~~~~~
In file included from include/linux/etherdevice.h:21,
                 from drivers/net/ethernet/cavium/octeon/octeon_mgmt.c:11:
include/linux/netdevice.h:2562:1: note: declared here
 2562 | netif_napi_add(struct net_device *dev, struct napi_struct *napi,

Guenter

Re: [PATCH net-next] net: drop the weight argument from netif_napi_add

[Jakub Kicinski]

On Sun, 2 Oct 2022 10:24:27 -0700 Guenter Roeck wrote:
> On Tue, Sep 27, 2022 at 06:27:53AM -0700, Jakub Kicinski wrote:
> > We tell driver developers to always pass NAPI_POLL_WEIGHT
> > as the weight to netif_napi_add(). This may be confusing
> > to newcomers, drop the weight argument, those who really
> > need to tweak the weight can use netif_napi_add_weight().
> > 
> > Signed-off-by: Jakub Kicinski <kuba@kernel.org>  
> 
> That seems to have missed some (or at least one) file(s).
> 
> Building mips:cavium_octeon_defconfig ... failed
> --------------
> Error log:
> drivers/net/ethernet/cavium/octeon/octeon_mgmt.c: In function 'octeon_mgmt_probe':
> drivers/net/ethernet/cavium/octeon/octeon_mgmt.c:1399:9: error: too many arguments to function 'netif_napi_add'
>  1399 |         netif_napi_add(netdev, &p->napi, octeon_mgmt_napi_poll,
>       |         ^~~~~~~~~~~~~~
> In file included from include/linux/etherdevice.h:21,
>                  from drivers/net/ethernet/cavium/octeon/octeon_mgmt.c:11:
> include/linux/netdevice.h:2562:1: note: declared here
>  2562 | netif_napi_add(struct net_device *dev, struct napi_struct *napi,

Fix sent, sorry. I don't see any more problems grepping again now..

Re: [PATCH net-next] net: drop the weight argument from netif_napi_add

[Guenter Roeck]

On 10/2/22 10:59, Jakub Kicinski wrote:
> On Sun, 2 Oct 2022 10:24:27 -0700 Guenter Roeck wrote:
>> On Tue, Sep 27, 2022 at 06:27:53AM -0700, Jakub Kicinski wrote:
>>> We tell driver developers to always pass NAPI_POLL_WEIGHT
>>> as the weight to netif_napi_add(). This may be confusing
>>> to newcomers, drop the weight argument, those who really
>>> need to tweak the weight can use netif_napi_add_weight().
>>>
>>> Signed-off-by: Jakub Kicinski <kuba@kernel.org>
>>
>> That seems to have missed some (or at least one) file(s).
>>
>> Building mips:cavium_octeon_defconfig ... failed
>> --------------
>> Error log:
>> drivers/net/ethernet/cavium/octeon/octeon_mgmt.c: In function 'octeon_mgmt_probe':
>> drivers/net/ethernet/cavium/octeon/octeon_mgmt.c:1399:9: error: too many arguments to function 'netif_napi_add'
>>   1399 |         netif_napi_add(netdev, &p->napi, octeon_mgmt_napi_poll,
>>        |         ^~~~~~~~~~~~~~
>> In file included from include/linux/etherdevice.h:21,
>>                   from drivers/net/ethernet/cavium/octeon/octeon_mgmt.c:11:
>> include/linux/netdevice.h:2562:1: note: declared here
>>   2562 | netif_napi_add(struct net_device *dev, struct napi_struct *napi,
> 
> Fix sent, sorry. I don't see any more problems grepping again now..

I think that was the only one. The following coccinelle script helps.

Guenter

---
virtual report

@s@
expression a, b, c, d;
position p;
@@

   netif_napi_add@p(a, b, c, d);

@script:python depends on report@
p << s.p;
@@
print "Bad netif_napi_add() call at %s:%s" % (p[0].file, p[0].line)