Re: [net-next PATCH v4 04/10] netdev-genl: Add netlink framework functions for queue

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

On 10/10/2023 7:25 PM, Jakub Kicinski wrote:
> On Fri, 06 Oct 2023 02:14:59 -0700 Amritha Nambiar wrote:
>> +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;
>> +	void *hdr;
>> +
>> +	hdr = genlmsg_iput(rsp, info);
>> +	if (!hdr)
>> +		return -EMSGSIZE;
>> +
>> +	if (nla_put_u32(rsp, NETDEV_A_QUEUE_QUEUE_ID, q_idx))
>> +		goto nla_put_failure;
>> +
>> +	if (nla_put_u32(rsp, NETDEV_A_QUEUE_QUEUE_TYPE, q_type))
>> +		goto nla_put_failure;
>> +
>> +	if (nla_put_u32(rsp, NETDEV_A_QUEUE_IFINDEX, netdev->ifindex))
>> +		goto nla_put_failure;
> 
> You can combine these ifs in a single one using ||
> 
Okay, will fix in v5.

>> +	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,
> 
>> +static int netdev_nl_queue_validate(struct net_device *netdev, u32 q_id,
>> +				    u32 q_type)
>> +{
>> +	switch (q_type) {
>> +	case NETDEV_QUEUE_TYPE_RX:
>> +		if (q_id >= netdev->real_num_rx_queues)
>> +			return -EINVAL;
>> +		return 0;
>> +	case NETDEV_QUEUE_TYPE_TX:
>> +		if (q_id >= netdev->real_num_tx_queues)
>> +			return -EINVAL;
>> +		return 0;
>> +	default:
>> +		return -EOPNOTSUPP;
> 
> Doesn't the netlink policy prevent this already?

For this, should I be using "checks: max:" as an attribute property for 
the 'queue-id' attribute in the yaml. If so, not sure how I can give a 
custom value (not hard-coded) for max.
Or should I include a pre-doit hook.

> 
>> +	}
>> +}
> 
>>   int netdev_nl_queue_get_doit(struct sk_buff *skb, struct genl_info *info)
>>   {
>> -	return -EOPNOTSUPP;
>> +	u32 q_id, q_type, ifindex;
>> +	struct net_device *netdev;
>> +	struct sk_buff *rsp;
>> +	int err;
>> +
>> +	if (GENL_REQ_ATTR_CHECK(info, NETDEV_A_QUEUE_QUEUE_ID))
>> +		return -EINVAL;
>> +
>> +	if (GENL_REQ_ATTR_CHECK(info, NETDEV_A_QUEUE_QUEUE_TYPE))
>> +		return -EINVAL;
>> +
>> +	if (GENL_REQ_ATTR_CHECK(info, NETDEV_A_QUEUE_IFINDEX))
>> +		return -EINVAL;
> 
> You can combine these checks in a single if using ||

Will fix in v5.

> 
>> +	q_id = nla_get_u32(info->attrs[NETDEV_A_QUEUE_QUEUE_ID]);
>> +
>> +	q_type = nla_get_u32(info->attrs[NETDEV_A_QUEUE_QUEUE_TYPE]);
>> +
>> +	ifindex = nla_get_u32(info->attrs[NETDEV_A_QUEUE_IFINDEX]);
> 
> No need for the empty lines between these.

Will fix.

> 
>> +	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)
>> +		err  = netdev_nl_queue_fill(rsp, netdev, q_id, q_type, info);
> 
> double space after =

Will fix.

> 
>> +	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_queue_dump_one(struct net_device *netdev, struct sk_buff *rsp,
>> +			 const struct genl_info *info, unsigned int *start_rx,
>> +			 unsigned int *start_tx)
>> +{
> 
> Hm. Not sure why you don't operate directly on ctx here.
> Why pass the indexes by pointer individually?
> 

Makes sense. Will fix in v5.

>> +	int err = 0;
>> +	int i;
>> +
>> +	for (i = *start_rx; i < netdev->real_num_rx_queues;) {
>> +		err = netdev_nl_queue_fill_one(rsp, netdev, i,
>> +					       NETDEV_QUEUE_TYPE_RX, info);
>> +		if (err)
>> +			goto out_err;
> 
> return, no need to goto if all it does is returns

Will fix.

> 
>> +		*start_rx = i++;
>> +	}
>> +	for (i = *start_tx; i < netdev->real_num_tx_queues;) {
>> +		err = netdev_nl_queue_fill_one(rsp, netdev, i,
>> +					       NETDEV_QUEUE_TYPE_TX, info);
>> +		if (err)
>> +			goto out_err;
>> +		*start_tx = i++;
>> +	}
>> +out_err:
>> +	return err;
>>   }
>>   
>>   int netdev_nl_queue_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 rxq_idx = ctx->rxq_idx;
>> +	unsigned int txq_idx = ctx->txq_idx;
>> +	struct net_device *netdev;
>> +	u32 ifindex = 0;
>> +	int err = 0;
>> +
>> +	if (info->attrs[NETDEV_A_QUEUE_IFINDEX])
>> +		ifindex = nla_get_u32(info->attrs[NETDEV_A_QUEUE_IFINDEX]);
>> +
>> +	rtnl_lock();
>> +	if (ifindex) {
>> +		netdev = __dev_get_by_index(net, ifindex);
>> +		if (netdev)
>> +			err = netdev_nl_queue_dump_one(netdev, skb, info,
>> +						       &rxq_idx, &txq_idx);
>> +		else
>> +			err = -ENODEV;
>> +	} else {
>> +		for_each_netdev_dump(net, netdev, ctx->ifindex) {
>> +			err = netdev_nl_queue_dump_one(netdev, skb, info,
>> +						       &rxq_idx, &txq_idx);
>> +
> 
> unnecessary new line

Will fix.

> 
>> +			if (err < 0)
>> +				break;
>> +			if (!err) {
> 
> it only returns 0 or negative errno, doesn't it?
> 
Will fix in v5.

>> +				rxq_idx = 0;
>> +				txq_idx = 0;
>> +			}
>> +		}
>> +	}
>> +	rtnl_unlock();
>> +
>> +	if (err != -EMSGSIZE)
>> +		return err;
>> +
>> +	ctx->rxq_idx = rxq_idx;
>> +	ctx->txq_idx = txq_idx;
>> +	return skb->len;
>>   }
>>   
>>   static int netdev_genl_netdevice_event(struct notifier_block *nb,
>>
> 
>