Re: [net-next PATCH v2 5/9] netdev-genl: Add netlink framework functions for napi

[Jakub Kicinski <kuba@kernel.org>]

On Mon, 21 Aug 2023 16:25:36 -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
> queue[s] can be retrieved this way.
> 
> Signed-off-by: Amritha Nambiar <amritha.nambiar@intel.com>

> @@ -119,14 +134,158 @@ 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)
> +{
> +	struct netdev_rx_queue *rx_queue, *rxq;
> +	struct netdev_queue *tx_queue, *txq;
> +	unsigned int rx_qid, tx_qid;
> +	void *hdr;
> +
> +	if (!napi->dev)
> +		return -EINVAL;

WARN_ON_ONCE()? If this can be assumed not to happen.

> +	hdr = genlmsg_iput(rsp, info);
> +	if (!hdr)
> +		return -EMSGSIZE;
> +
> +	if (nla_put_u32(rsp, NETDEV_A_NAPI_NAPI_ID, napi->napi_id))

napi_id can be zero.

> +		goto nla_put_failure;
> +
> +	if (nla_put_u32(rsp, NETDEV_A_NAPI_IFINDEX, napi->dev->ifindex))
> +		goto nla_put_failure;

>  int netdev_nl_napi_get_doit(struct sk_buff *skb, struct genl_info *info)
>  {
> -	return -EOPNOTSUPP;
> +	struct net_device *netdev;
> +	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();
> +
> +	netdev = dev_get_by_napi_id(napi_id);

Why lookup the dev and not the NAPI?

> +	if (netdev)
> +		err  = netdev_nl_napi_fill(netdev, rsp, info, napi_id);
> +	else
> +		err = -ENODEV;
> +
> +	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, int *start)
> +{
> +	struct napi_struct *napi, *n;
> +	int err = 0;
> +	int i = 0;
> +
> +	list_for_each_entry_safe(napi, n, &netdev->napi_list, dev_list) {

Why _safe()? I think you need _rcu() instead?

> +		if (i < *start) {
> +			i++;
> +			continue;
> +		}
> +		err = netdev_nl_napi_fill_one(rsp, napi, info);
> +		if (err)
> +			break;
> +		*start = ++i;

Why count them instead of relying on the IDs?

>  int netdev_nl_napi_get_dumpit(struct sk_buff *skb, struct netlink_callback *cb)
>  {
> -	return -EOPNOTSUPP;
> +	const struct genl_dumpit_info *info = genl_dumpit_info(cb);

You can get genl_info_dump(cb) here, you don't use the genl_dumpit_info
AFAICT, only info->info.