Re: [PATCH v5 net-next 04/12] net-shapers: implement NL group operation

[Jakub Kicinski <kuba@kernel.org>]

On Thu, 29 Aug 2024 17:16:57 +0200 Paolo Abeni wrote:
> Allow grouping multiple leaves shaper under the given root.
> The root and the leaves shapers are created, if needed, otherwise
> the existing shapers are re-linked as requested.
> 
> Try hard to pre-allocated the needed resources, to avoid non
> trivial H/W configuration rollbacks in case of any failure.

Need to s/root/parent/ the commit message?

> +static int __net_shaper_group(struct net_shaper_binding *binding,
> +			      int leaves_count,
> +			      const struct net_shaper_handle *leaves_handles,
> +			      struct net_shaper_info *leaves,
> +			      struct net_shaper_handle *node_handle,
> +			      struct net_shaper_info *node,
> +			      struct netlink_ext_ack *extack)
> +{
> +	const struct net_shaper_ops *ops = net_shaper_binding_ops(binding);
> +	struct net_shaper_info *parent = NULL;
> +	struct net_shaper_handle leaf_handle;
> +	int i, ret;
> +
> +	if (node_handle->scope == NET_SHAPER_SCOPE_NODE) {
> +		if (node_handle->id != NET_SHAPER_ID_UNSPEC &&
> +		    !net_shaper_cache_lookup(binding, node_handle)) {
> +			NL_SET_ERR_MSG_FMT(extack, "Node shaper %d:%d does not exists",
> +					   node_handle->scope, node_handle->id);

BAD_ATTR would do?

> +			return -ENOENT;
> +		}
> +
> +		/* When unspecified, the node parent scope is inherited from
> +		 * the leaves.
> +		 */
> +		if (node->parent.scope == NET_SHAPER_SCOPE_UNSPEC) {
> +			for (i = 1; i < leaves_count; ++i) {
> +				if (leaves[i].parent.scope !=
> +				    leaves[0].parent.scope ||
> +				    leaves[i].parent.id !=
> +				    leaves[0].parent.id) {

memcmp() ? put a BUILD_BUG_ON(sizeof() != 8) to make sure we double
check it if the struct grows?

> +					NL_SET_ERR_MSG_FMT(extack, "All the leaves shapers must have the same old parent");
> +					return -EINVAL;

5 indents is too many indents :( maybe make the for loop a helper?

> +				}
> +			}
> +
> +			if (leaves_count > 0)

how can we get here and not have leaves? :o

> +				node->parent = leaves[0].parent;
> +		}
> +
> +	} else {
> +		net_shaper_default_parent(node_handle, &node->parent);
> +	}

> +static int net_shaper_group_send_reply(struct genl_info *info,
> +				       struct net_shaper_handle *handle)
> +{
> +	struct net_shaper_binding *binding = info->user_ptr[0];
> +	struct sk_buff *msg;
> +	int ret = -EMSGSIZE;
> +	void *hdr;
> +
> +	/* Prepare the msg reply in advance, to avoid device operation
> +	 * rollback.
> +	 */
> +	msg = genlmsg_new(net_shaper_handle_size(), GFP_KERNEL);
> +	if (!msg)
> +		return ret;

return -ENOMEM;

> +
> +	hdr = genlmsg_iput(msg, info);
> +	if (!hdr)
> +		goto free_msg;
> +
> +	if (net_shaper_fill_binding(msg, binding, NET_SHAPER_A_IFINDEX))
> +		goto free_msg;
> +
> +	if (net_shaper_fill_handle(msg, handle, NET_SHAPER_A_HANDLE))

you can combine the two fill ifs into one with ||

> +		goto free_msg;
> +
> +	genlmsg_end(msg, hdr);
> +
> +	ret = genlmsg_reply(msg, info);
> +	if (ret)
> +		goto free_msg;

reply always eats the skb, just:

	return genlmsg_reply(msg, info);

> +
> +	return ret;
> +
> +free_msg:
> +	nlmsg_free(msg);
> +	return ret;

return -EMSGSIZE;

> +}
> +
> +int net_shaper_nl_group_doit(struct sk_buff *skb, struct genl_info *info)
> +{
> +	struct net_shaper_handle *leaves_handles, node_handle;
> +	struct net_shaper_info *leaves, node;
> +	struct net_shaper_binding *binding;
> +	int i, ret, rem, leaves_count;
> +	struct nlattr *attr;
> +
> +	if (GENL_REQ_ATTR_CHECK(info, NET_SHAPER_A_LEAVES) ||
> +	    GENL_REQ_ATTR_CHECK(info, NET_SHAPER_A_NODE))
> +		return -EINVAL;
> +
> +	binding = net_shaper_binding_from_ctx(info->user_ptr[0]);
> +	leaves_count = net_shaper_list_len(info, NET_SHAPER_A_LEAVES);
> +	leaves = kcalloc(leaves_count, sizeof(struct net_shaper_info) +
> +			 sizeof(struct net_shaper_handle), GFP_KERNEL);
> +	if (!leaves) {
> +		GENL_SET_ERR_MSG_FMT(info, "Can't allocate memory for %d leaves shapers",
> +				     leaves_count);
> +		return -ENOMEM;
> +	}
> +	leaves_handles = (struct net_shaper_handle *)&leaves[leaves_count];
> +
> +	ret = net_shaper_parse_node(binding, info->attrs[NET_SHAPER_A_NODE],
> +				    info, &node_handle, &node);
> +	if (ret)
> +		goto free_shapers;
> +
> +	i = 0;
> +	nla_for_each_attr_type(attr, NET_SHAPER_A_LEAVES,
> +			       genlmsg_data(info->genlhdr),
> +			       genlmsg_len(info->genlhdr), rem) {
> +		if (WARN_ON_ONCE(i >= leaves_count))
> +			goto free_shapers;
> +
> +		ret = net_shaper_parse_info_nest(binding, attr, info,
> +						 NET_SHAPER_SCOPE_QUEUE,
> +						 &leaves_handles[i],

Wouldn't it be convenient to store the handle in the "info" object?
AFAIU the handle is forever for an info, so no risk of it being out 
of sync...

> +						 &leaves[i]);
> +		if (ret)
> +			goto free_shapers;
> +		i++;
> +	}
> +
> +	ret = net_shaper_group(binding, leaves_count, leaves_handles, leaves,
> +			       &node_handle, &node, info->extack);

...and it'd be nice if group had 5 rather than 7 params

> +	if (ret < 0)
> +		goto free_shapers;
> +
> +	ret = net_shaper_group_send_reply(info, &node_handle);
> +	if (ret) {
> +		/* Error on reply is not fatal to avoid rollback a successful
> +		 * configuration.

Slight issues with the grammar here, but I think it should be fatal.
The sender will most likely block until they get a response.
Not to mention that the caller will not know what the handle 
we allocated is.

> +		 */
> +		GENL_SET_ERR_MSG_FMT(info, "Can't send reply %d", ret);
> +		ret = 0;
> +	}
> +
> +free_shapers:
> +	kfree(leaves);
> +	return ret;
> +}