Message-ID: <dd842cbd-5282-423c-85cd-a8969da6e814@intel.com>
Date: Mon, 8 Apr 2024 12:45:49 -0700
From: "Nambiar, Amritha" <amritha.nambiar@...el.com>
To: David Wei <dw@...idwei.uk>, <netdev@...r.kernel.org>, <kuba@...nel.org>,
	<davem@...emloft.net>
CC: <edumazet@...gle.com>, <pabeni@...hat.com>, <ast@...nel.org>,
	<sdf@...gle.com>, <lorenzo@...nel.org>, <tariqt@...dia.com>,
	<daniel@...earbox.net>, <anthony.l.nguyen@...el.com>, <lucien.xin@...il.com>,
	<hawk@...nel.org>, <sridhar.samudrala@...el.com>
Subject: Re: [net-next, RFC PATCH 2/5] netdev-genl: Add netlink framework
 functions for queue-set NAPI

On 4/5/2024 5:20 PM, David Wei wrote:
> On 2024-04-05 13:09, Amritha Nambiar wrote:
>> Implement the netdev netlink framework functions for associating
>> a queue with NAPI ID.
>>
>> Signed-off-by: Amritha Nambiar <amritha.nambiar@...el.com>
>> ---
>>   include/linux/netdevice.h |    7 +++
>>   net/core/netdev-genl.c    |  117 +++++++++++++++++++++++++++++++++++++++------
>>   2 files changed, 108 insertions(+), 16 deletions(-)
>>
>> diff --git a/include/linux/netdevice.h b/include/linux/netdevice.h
>> index 0c198620ac93..70df1cec4a60 100644
>> --- a/include/linux/netdevice.h
>> +++ b/include/linux/netdevice.h
>> @@ -1351,6 +1351,10 @@ struct netdev_net_notifier {
>>    *			   struct kernel_hwtstamp_config *kernel_config,
>>    *			   struct netlink_ext_ack *extack);
>>    *	Change the hardware timestamping parameters for NIC device.
>> + *
>> + * int (*ndo_queue_set_napi)(struct net_device *dev, u32 q_idx, u32 q_type,
>> + *			     struct napi_struct *napi);
>> + *	Change the NAPI instance associated with the queue.
>>    */
>>   struct net_device_ops {
>>   	int			(*ndo_init)(struct net_device *dev);
>> @@ -1596,6 +1600,9 @@ struct net_device_ops {
>>   	int			(*ndo_hwtstamp_set)(struct net_device *dev,
>>   						    struct kernel_hwtstamp_config *kernel_config,
>>   						    struct netlink_ext_ack *extack);
>> +	int			(*ndo_queue_set_napi)(struct net_device *dev,
>> +						      u32 q_idx, u32 q_type,
>> +						      struct napi_struct *napi);
>>   };
>>   
>>   /**
>> diff --git a/net/core/netdev-genl.c b/net/core/netdev-genl.c
>> index d5b2e90e5709..6b3d3165d76e 100644
>> --- a/net/core/netdev-genl.c
>> +++ b/net/core/netdev-genl.c
>> @@ -288,12 +288,29 @@ int netdev_nl_napi_get_dumpit(struct sk_buff *skb, struct netlink_callback *cb)
>>   	return err;
>>   }
>>   
>> +/* must be called under rtnl_lock() */
>> +static struct napi_struct *
>> +napi_get_by_queue(struct net_device *netdev, u32 q_idx, u32 q_type)
>> +{
>> +	struct netdev_rx_queue *rxq;
>> +	struct netdev_queue *txq;
>> +
>> +	switch (q_type) {
>> +	case NETDEV_QUEUE_TYPE_RX:
>> +		rxq = __netif_get_rx_queue(netdev, q_idx);
>> +		return rxq->napi;
>> +	case NETDEV_QUEUE_TYPE_TX:
>> +		txq = netdev_get_tx_queue(netdev, q_idx);
>> +		return txq->napi;
>> +	}
>> +	return NULL;
>> +}
>> +
>>   static int
>>   netdev_nl_queue_fill_one(struct sk_buff *rsp, struct net_device *netdev,
>>   			 u32 q_idx, u32 q_type, const struct genl_info *info)
>>   {
>> -	struct netdev_rx_queue *rxq;
>> -	struct netdev_queue *txq;
>> +	struct napi_struct *napi;
>>   	void *hdr;
>>   
>>   	hdr = genlmsg_iput(rsp, info);
>> @@ -305,19 +322,9 @@ netdev_nl_queue_fill_one(struct sk_buff *rsp, struct net_device *netdev,
>>   	    nla_put_u32(rsp, NETDEV_A_QUEUE_IFINDEX, netdev->ifindex))
>>   		goto nla_put_failure;
>>   
>> -	switch (q_type) {
>> -	case NETDEV_QUEUE_TYPE_RX:
>> -		rxq = __netif_get_rx_queue(netdev, q_idx);
>> -		if (rxq->napi && nla_put_u32(rsp, NETDEV_A_QUEUE_NAPI_ID,
>> -					     rxq->napi->napi_id))
>> -			goto nla_put_failure;
>> -		break;
>> -	case NETDEV_QUEUE_TYPE_TX:
>> -		txq = netdev_get_tx_queue(netdev, q_idx);
>> -		if (txq->napi && nla_put_u32(rsp, NETDEV_A_QUEUE_NAPI_ID,
>> -					     txq->napi->napi_id))
>> -			goto nla_put_failure;
>> -	}
>> +	napi = napi_get_by_queue(netdev, q_idx, q_type);
>> +	if (napi && nla_put_u32(rsp, NETDEV_A_QUEUE_NAPI_ID, napi->napi_id))
>> +		goto nla_put_failure;
>>   
>>   	genlmsg_end(rsp, hdr);
>>   
>> @@ -674,9 +681,87 @@ int netdev_nl_qstats_get_dumpit(struct sk_buff *skb,
>>   	return err;
>>   }
>>   
>> +static int
>> +netdev_nl_queue_set_napi(struct sk_buff *rsp, struct net_device *netdev,
>> +			 u32 q_idx, u32 q_type, u32 napi_id,
>> +			 const struct genl_info *info)
>> +{
>> +	struct napi_struct *napi, *old_napi;
>> +	int err;
>> +
>> +	if (!(netdev->flags & IFF_UP))
>> +		return 0;
> 
> Should this be an error code?

Thought I can return 0 like the _get_ functions as this is a noop and 
not really an error.

> 
>> +
>> +	err = netdev_nl_queue_validate(netdev, q_idx, q_type);
>> +	if (err)
>> +		return err;
>> +
>> +	old_napi = napi_get_by_queue(netdev, q_idx, q_type);
>> +	if (old_napi && old_napi->napi_id == napi_id)
>> +		return 0;
> 
> Same as above, I think this should be an error.

I was thinking I can follow the ethtool semantics here, when there's no 
update/parameter values are not modified, it is a noop and proceed to 
display the current values instead of throwing error.

> 
>> +
>> +	napi = napi_by_id(napi_id);
>> +	if (!napi)
>> +		return -EINVAL;
>> +
>> +	err = netdev->netdev_ops->ndo_queue_set_napi(netdev, q_idx, q_type, napi);
>> +
>> +	return err;
> 
> nit: return ndo_queue_set_napi() would save two lines.

Sure, will fix in the next version.

> 
>> +}
>> +
>>   int netdev_nl_queue_set_doit(struct sk_buff *skb, struct genl_info *info)
>>   {
>> -	return -EOPNOTSUPP;
>> +	u32 q_id, q_type, ifindex;
> 
> nit: q_idx for consistency?

Sure, will fix.

> 
>> +	struct net_device *netdev;
>> +	struct sk_buff *rsp;
>> +	u32 napi_id = 0;
>> +	int err = 0;
>> +
>> +	if (GENL_REQ_ATTR_CHECK(info, NETDEV_A_QUEUE_ID) ||
>> +	    GENL_REQ_ATTR_CHECK(info, NETDEV_A_QUEUE_TYPE) ||
>> +	    GENL_REQ_ATTR_CHECK(info, NETDEV_A_QUEUE_IFINDEX))
>> +		return -EINVAL;
>> +
>> +	q_id = nla_get_u32(info->attrs[NETDEV_A_QUEUE_ID]);
>> +	q_type = nla_get_u32(info->attrs[NETDEV_A_QUEUE_TYPE]);
>> +	ifindex = nla_get_u32(info->attrs[NETDEV_A_QUEUE_IFINDEX]);
>> +
>> +	if (info->attrs[NETDEV_A_QUEUE_NAPI_ID]) {
>> +		napi_id = nla_get_u32(info->attrs[NETDEV_A_QUEUE_NAPI_ID]);
>> +		if (napi_id < MIN_NAPI_ID)
>> +			return -EINVAL;
>> +	}
>> +
>> +	rsp = genlmsg_new(GENLMSG_DEFAULT_SIZE, GFP_KERNEL);
>> +	if (!rsp)
>> +		return -ENOMEM;
>> +
>> +	rtnl_lock();
>> +
>> +	netdev = __dev_get_by_index(genl_info_net(info), ifindex);
>> +	if (netdev) {
>> +		if (!napi_id)
>> +			GENL_SET_ERR_MSG(info, "No queue parameters changed\n");
> 
> Could this be checked earlier outside of rtnl_lock? I feel like not
> setting a napi_id here is EINVAL.
> 

I agree this part is a little ambiguous as I was trying to handle a few 
things here that I was not really sure of. The idea was that 
netdev_nl_queue_set_doit() could be the one function to handle all set 
params for the queue object. Today, only napi-id is supported. In 
future, there may be additional configurable attributes for the queue 
object beyond napi-id. So, it should be possible to set those params as 
well from netdev_nl_queue_set_doit(), which implies that, not setting 
napi-id is not EINVAL, as some other param could be set. If none of the 
configurable params are set, display the message and the current 
settings for the queue.
So the code today handles this; if the user has given a napi-id and it 
is valid, then proceed to the ndo callback. If the user has not 
specified a napi-id to update, it is not an error case, display the 
current settings, same as ethtool-set params does (although it's done in 
the userspace in case of ethtool). Ideally, this should be handled in 
the userspace for netdev-genl, maybe some tricks in the YAML can achieve 
this for the set command to check for the presence of atleast one 
configurable attribute in netdev-user.c.
I think, the following in YAML would make napi-id mandatory:
name: napi-id
checks:
   min: 1

But, that's not exactly what we want for set any params, what I was 
aiming for was to have any one attribute required from among a group of 
attributes.

>> +		else
>> +			err = netdev_nl_queue_set_napi(rsp, netdev, q_id,
>> +						       q_type, napi_id, info);
>> +		if (!err)
>> +			err = netdev_nl_queue_fill_one(rsp, netdev, q_id,
>> +						       q_type, info);
>> +	} else {
>> +		err = -ENODEV;
> 
> Could be less nesty (completely untested):
> 

Sure, thanks. I'll make this less nesty.

> 	if (!netdev) {
> 		err = -ENODEV;
> 		goto err_rtnl_unlock;
> 	}
> 
> 	if (!napi_id) {
> 		GENL_SET_ERR_MSG(info, "No queue parameters changed\n");
> 		goto err_nonapi;
> 	}
> 
> 	err = netdev_nl_queue_set_napi(rsp, netdev, q_id,
> 				       q_type, napi_id, info);
> 	if (err)
> 		goto err_rtnl_unlock;
> 
> err_nonapi:
> 	err = netdev_nl_queue_fill_one(rsp, netdev, q_id,
> 				       q_type, info);
> 
> err_rtnl_unlock:
> 	rtnl_unlock();
> 
> 	if (!err)
> 		return genlmsg_reply(rsp, info);
> 
> err_free_msg:
> 	nlmsg_free(rsp);
> 	return err;
> 
>> +	}
>> +
>> +	rtnl_unlock();
>> +
>> +	if (err)
>> +		goto err_free_msg;
>> +
>> +	return genlmsg_reply(rsp, info);
>> +
>> +err_free_msg:
>> +	nlmsg_free(rsp);
>> +	return err;
>>   }
>>   
>>   static int netdev_genl_netdevice_event(struct notifier_block *nb,
>>

Powered by blists - more mailing lists