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

[Jakub Kicinski <kuba@kernel.org>]

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

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

> +	}
> +}

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

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

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

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

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

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

> +			if (err < 0)
> +				break;
> +			if (!err) {

it only returns 0 or negative errno, doesn't it?

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