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

["Nambiar, Amritha" <amritha.nambiar@intel.com>]

On 8/22/2023 5:51 PM, Jakub Kicinski wrote:
> 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.

Okay. Will fix in v3.

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

Will fix in v3.

> 
>> +		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?

Agree. Will fix in v3.

> 
>> +	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?

Agree. This is called within rtnl_lock. Will fix.

> 
>> +		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?

Makes sense. Will fix.

> 
>>   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.

Will fix in v3.