diff mbox series

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

Message ID 169516246697.7377.18105000910116179893.stgit@anambiarhost.jf.intel.com (mailing list archive)
State Changes Requested
Delegated to: Netdev Maintainers
Headers show
Series Introduce queue and NAPI support in netdev-genl (Was: Introduce NAPI queues support) | expand

Checks

Context Check Description
netdev/series_format success Posting correctly formatted
netdev/tree_selection success Clearly marked for net-next, async
netdev/fixes_present success Fixes tag not required for -next series
netdev/header_inline success No static functions without inline keyword in header files
netdev/build_32bit success Errors and warnings before: 5499 this patch: 5499
netdev/cc_maintainers warning 4 maintainers not CCed: edumazet@google.com pabeni@redhat.com daniel@iogearbox.net davem@davemloft.net
netdev/build_clang success Errors and warnings before: 1672 this patch: 1672
netdev/verify_signedoff success Signed-off-by tag matches author and committer
netdev/deprecated_api success None detected
netdev/check_selftest success No net selftest shell script
netdev/verify_fixes success No Fixes tag
netdev/build_allmodconfig_warn success Errors and warnings before: 5879 this patch: 5879
netdev/checkpatch success total: 0 errors, 0 warnings, 0 checks, 171 lines checked
netdev/kdoc success Errors and warnings before: 0 this patch: 0
netdev/source_inline success Was 0 now: 0

Commit Message

Nambiar, Amritha Sept. 19, 2023, 10:27 p.m. UTC 
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@intel.com>
Reviewed-by: Sridhar Samudrala <sridhar.samudrala@intel.com>
---
 include/linux/netdevice.h |    2 +
 net/core/dev.c            |    4 +-
 net/core/netdev-genl.c    |  117 ++++++++++++++++++++++++++++++++++++++++++++-
 3 files changed, 119 insertions(+), 4 deletions(-)

Comments

Paolo Abeni Sept. 28, 2023, 11:19 a.m. UTC | #1 
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@intel.com>
> Reviewed-by: Sridhar Samudrala <sridhar.samudrala@intel.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
Nambiar, Amritha Sept. 28, 2023, 11:34 p.m. UTC | #2 
On 9/28/2023 4:19 AM, Paolo Abeni wrote:
> 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@intel.com>
>> Reviewed-by: Sridhar Samudrala <sridhar.samudrala@intel.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]
> 

Not sure I follow this comment. Could you please clarify.

I introduced the netdev_nl_dump_ctx structure to save the 
states/counters for dump (for ifindex, queue indices, NAPI IDs etc.) if 
the dump was interrupted due to EMSGSIZE error. Also, I decided to use 
cb->ctx instead of cb->args[] due to a comment in the definition of 
struct netlink_callback in include/linux/netlink.h that says "args is 
deprecated. Cast a struct over ctx instead for proper type safety."

The 'napi_id' is saved into netdev_nl_dump_ctx since there is no need 
for a separate counter when we can rely on the NAPI IDs itself (as 
pointed out by Jakub's review comment on v2).

> [...]
>> +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.
> 

Makes sense. Access is protected by the rtnl_lock, so _rcu protection 
isn't needed, I can just use 'list_for_each_entry' for list traversal.

> 
> Cheers,
> 
> Paolo
>
diff mbox series

Patch

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;
 };
 
 static inline struct netdev_nl_dump_ctx *
@@ -145,14 +147,125 @@  int netdev_nl_dev_get_dumpit(struct sk_buff *skb, struct netlink_callback *cb)
 	return skb->len;
 }
 
+static int
+netdev_nl_napi_fill_one(struct sk_buff *rsp, struct napi_struct *napi,
+			const struct genl_info *info)
+{
+	void *hdr;
+
+	if (WARN_ON_ONCE(!napi->dev))
+		return -EINVAL;
+
+	hdr = genlmsg_iput(rsp, info);
+	if (!hdr)
+		return -EMSGSIZE;
+
+	if (napi->napi_id >= MIN_NAPI_ID &&
+	    nla_put_u32(rsp, NETDEV_A_NAPI_NAPI_ID, napi->napi_id))
+		goto nla_put_failure;
+
+	if (nla_put_u32(rsp, NETDEV_A_NAPI_IFINDEX, napi->dev->ifindex))
+		goto nla_put_failure;
+
+	genlmsg_end(rsp, hdr);
+	return 0;
+
+nla_put_failure:
+	genlmsg_cancel(rsp, hdr);
+	return -EMSGSIZE;
+}
+
 int netdev_nl_napi_get_doit(struct sk_buff *skb, struct genl_info *info)
 {
-	return -EOPNOTSUPP;
+	struct napi_struct *napi;
+	struct sk_buff *rsp;
+	u32 napi_id;
+	int err;
+
+	if (GENL_REQ_ATTR_CHECK(info, NETDEV_A_NAPI_NAPI_ID))
+		return -EINVAL;
+
+	napi_id = nla_get_u32(info->attrs[NETDEV_A_NAPI_NAPI_ID]);
+
+	rsp = genlmsg_new(GENLMSG_DEFAULT_SIZE, GFP_KERNEL);
+	if (!rsp)
+		return -ENOMEM;
+
+	rtnl_lock();
+
+	napi = napi_by_id(napi_id);
+	if (napi)
+		err  = netdev_nl_napi_fill_one(rsp, napi, info);
+	else
+		err = -EINVAL;
+
+	rtnl_unlock();
+
+	if (err)
+		goto err_free_msg;
+
+	return genlmsg_reply(rsp, info);
+
+err_free_msg:
+	nlmsg_free(rsp);
+	return err;
+}
+
+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) {
+		if (*start && napi->napi_id >= *start)
+			continue;
+
+		err = netdev_nl_napi_fill_one(rsp, napi, info);
+		if (err)
+			break;
+		*start = napi->napi_id;
+	}
+	return err;
 }
 
 int netdev_nl_napi_get_dumpit(struct sk_buff *skb, struct netlink_callback *cb)
 {
-	return -EOPNOTSUPP;
+	struct netdev_nl_dump_ctx *ctx = netdev_dump_ctx(cb);
+	const struct genl_info *info = genl_info_dump(cb);
+	struct net *net = sock_net(skb->sk);
+	unsigned int n_id = ctx->napi_id;
+	struct net_device *netdev;
+	u32 ifindex = 0;
+	int err = 0;
+
+	if (info->attrs[NETDEV_A_NAPI_IFINDEX])
+		ifindex = nla_get_u32(info->attrs[NETDEV_A_NAPI_IFINDEX]);
+
+	rtnl_lock();
+	if (ifindex) {
+		netdev = __dev_get_by_index(net, ifindex);
+		if (netdev)
+			err = netdev_nl_napi_dump_one(netdev, skb, info, &n_id);
+		else
+			err = -ENODEV;
+	} else {
+		for_each_netdev_dump(net, netdev, ctx->ifindex) {
+			err = netdev_nl_napi_dump_one(netdev, skb, info, &n_id);
+			if (err < 0)
+				break;
+			if (!err)
+				n_id = 0;
+		}
+	}
+	rtnl_unlock();
+
+	if (err != -EMSGSIZE)
+		return err;
+
+	ctx->napi_id = n_id;
+	return skb->len;
 }
 
 static int