netdev - Re: [net-next PATCH v3 06/10] netdev-genl: Add netlink framework functions for napi

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <1f55345f58987dbcfd566e1f61aee55d2c78f788.camel@redhat.com>
Date: Thu, 28 Sep 2023 13:19:48 +0200
From: Paolo Abeni <pabeni@...hat.com>
To: Amritha Nambiar <amritha.nambiar@...el.com>, netdev@...r.kernel.org, 
	kuba@...nel.org
Cc: sridhar.samudrala@...el.com
Subject: Re: [net-next PATCH v3 06/10] netdev-genl: Add netlink framework
 functions for napi

On Tue, 2023-09-19 at 15:27 -0700, Amritha Nambiar wrote:
> Implement the netdev netlink framework functions for
> napi support. The netdev structure tracks all the napi
> instances and napi fields. The napi instances and associated
> parameters can be retrieved this way.
> 
> Signed-off-by: Amritha Nambiar <amritha.nambiar@...el.com>
> Reviewed-by: Sridhar Samudrala <sridhar.samudrala@...el.com>
> ---
>  include/linux/netdevice.h |    2 +
>  net/core/dev.c            |    4 +-
>  net/core/netdev-genl.c    |  117 ++++++++++++++++++++++++++++++++++++++++++++-
>  3 files changed, 119 insertions(+), 4 deletions(-)
> 
> diff --git a/include/linux/netdevice.h b/include/linux/netdevice.h
> index 69e363918e4b..e7321178dc1a 100644
> --- a/include/linux/netdevice.h
> +++ b/include/linux/netdevice.h
> @@ -536,6 +536,8 @@ static inline bool napi_complete(struct napi_struct *n)
>  	return napi_complete_done(n, 0);
>  }
>  
> +struct napi_struct *napi_by_id(unsigned int napi_id);
> +
>  int dev_set_threaded(struct net_device *dev, bool threaded);
>  
>  /**
> diff --git a/net/core/dev.c b/net/core/dev.c
> index 508b1d799681..ea6b3115ee8b 100644
> --- a/net/core/dev.c
> +++ b/net/core/dev.c
> @@ -165,7 +165,6 @@ static int netif_rx_internal(struct sk_buff *skb);
>  static int call_netdevice_notifiers_extack(unsigned long val,
>  					   struct net_device *dev,
>  					   struct netlink_ext_ack *extack);
> -static struct napi_struct *napi_by_id(unsigned int napi_id);
>  
>  /*
>   * The @dev_base_head list is protected by @dev_base_lock and the rtnl
> @@ -6133,7 +6132,7 @@ bool napi_complete_done(struct napi_struct *n, int work_done)
>  EXPORT_SYMBOL(napi_complete_done);
>  
>  /* must be called under rcu_read_lock(), as we dont take a reference */
> -static struct napi_struct *napi_by_id(unsigned int napi_id)
> +struct napi_struct *napi_by_id(unsigned int napi_id)
>  {
>  	unsigned int hash = napi_id % HASH_SIZE(napi_hash);
>  	struct napi_struct *napi;
> @@ -6144,6 +6143,7 @@ static struct napi_struct *napi_by_id(unsigned int napi_id)
>  
>  	return NULL;
>  }
> +EXPORT_SYMBOL(napi_by_id);
>  
>  #if defined(CONFIG_NET_RX_BUSY_POLL)
>  
> diff --git a/net/core/netdev-genl.c b/net/core/netdev-genl.c
> index 8609884fefe4..6f4ed21ebd15 100644
> --- a/net/core/netdev-genl.c
> +++ b/net/core/netdev-genl.c
> @@ -7,6 +7,7 @@
>  #include <net/sock.h>
>  #include <net/xdp.h>
>  #include <net/netdev_rx_queue.h>
> +#include <net/busy_poll.h>
>  
>  #include "netdev-genl-gen.h"
>  
> @@ -14,6 +15,7 @@ struct netdev_nl_dump_ctx {
>  	unsigned long	ifindex;
>  	unsigned int	rxq_idx;
>  	unsigned int	txq_idx;
> +	unsigned int	napi_id;

Why you need to add napi_id to the context? don't you need just such
field to do the traversing? e.g. use directly single cb[0]

[...]
> +static int
> +netdev_nl_napi_dump_one(struct net_device *netdev, struct sk_buff *rsp,
> +			const struct genl_info *info, unsigned int *start)
> +{
> +	struct napi_struct *napi;
> +	int err = 0;
> +
> +	list_for_each_entry_rcu(napi, &netdev->napi_list, dev_list) {

I'm sorry for the conflicting feedback here, but I think you don't need
any special variant, just 'list_for_each_entry'. This is under rtnl
lock, the list can't change under the hood. Also I think you should get
a RCU splat here when building with CONFIG_PROVE_RCU_LIST.


Cheers,

Paolo