Message-ID: <Zq0C8HdpMCAxya9P@nanopsycho.orion>
Date: Fri, 2 Aug 2024 18:01:52 +0200
From: Jiri Pirko <jiri@...nulli.us>
To: Paolo Abeni <pabeni@...hat.com>
Cc: netdev@...r.kernel.org, Jakub Kicinski <kuba@...nel.org>,
	Madhu Chittim <madhu.chittim@...el.com>,
	Sridhar Samudrala <sridhar.samudrala@...el.com>,
	Simon Horman <horms@...nel.org>,
	John Fastabend <john.fastabend@...il.com>,
	Sunil Kovvuri Goutham <sgoutham@...vell.com>,
	Jamal Hadi Salim <jhs@...atatu.com>
Subject: Re: [PATCH RFC v2 03/11] net-shapers: implement NL set and delete
 operations

Wed, Jul 24, 2024 at 10:24:49PM CEST, pabeni@...hat.com wrote:
>Both NL operations directly map on the homonymous device shaper
>callbacks and update accordingly the shapers cache.
>Implement the cache modification helpers to additionally deal with
>DETACHED scope shaper. That will be needed by the group() operation
>implemented in the next patch.
>
>Signed-off-by: Paolo Abeni <pabeni@...hat.com>
>---
> net/shaper/shaper.c | 323 +++++++++++++++++++++++++++++++++++++++++++-
> 1 file changed, 321 insertions(+), 2 deletions(-)
>
>diff --git a/net/shaper/shaper.c b/net/shaper/shaper.c
>index 93dd491ac7c2..7802c9ba6d9c 100644
>--- a/net/shaper/shaper.c
>+++ b/net/shaper/shaper.c
>@@ -19,6 +19,35 @@ struct net_shaper_nl_ctx {
> 	u32 start_handle;
> };
> 
>+static u32 default_parent(u32 handle)
>+{
>+	enum net_shaper_scope parent, scope = net_shaper_handle_scope(handle);

In the rest of the code you call this "pscope", could you rename?


>+
>+	switch (scope) {
>+	case NET_SHAPER_SCOPE_PORT:
>+	case NET_SHAPER_SCOPE_UNSPEC:
>+		parent = NET_SHAPER_SCOPE_UNSPEC;
>+		break;
>+
>+	case NET_SHAPER_SCOPE_QUEUE:
>+	case NET_SHAPER_SCOPE_DETACHED:

Why default parent of detached is netdev? Detached means "node in tree"
if IIUC. Can't port be parent?

>+		parent = NET_SHAPER_SCOPE_NETDEV;
>+		break;
>+
>+	case NET_SHAPER_SCOPE_NETDEV:
>+	case NET_SHAPER_SCOPE_VF:
>+		parent = NET_SHAPER_SCOPE_PORT;
>+		break;
>+	}
>+
>+	return net_shaper_make_handle(parent, 0);
>+}
>+
>+static bool is_detached(u32 handle)
>+{
>+	return net_shaper_handle_scope(handle) == NET_SHAPER_SCOPE_DETACHED;
>+}
>+
> static int fill_handle(struct sk_buff *msg, u32 handle, u32 type,
> 		       const struct genl_info *info)
> {
>@@ -117,6 +146,115 @@ static struct net_shaper_info *sc_lookup(struct net_device *dev, u32 handle)
> 	return xa ? xa_load(xa, handle) : NULL;
> }
> 
>+/* allocate on demand the per device shaper's cache */
>+static struct xarray *__sc_init(struct net_device *dev,
>+				struct netlink_ext_ack *extack)
>+{
>+	if (!dev->net_shaper_data) {
>+		dev->net_shaper_data = kmalloc(sizeof(*dev->net_shaper_data),
>+					       GFP_KERNEL);
>+		if (!dev->net_shaper_data) {
>+			NL_SET_ERR_MSG(extack, "Can't allocate memory for shaper data");
>+			return NULL;
>+		}
>+
>+		xa_init(&dev->net_shaper_data->shapers);
>+		idr_init(&dev->net_shaper_data->detached_ids);
>+	}
>+	return &dev->net_shaper_data->shapers;
>+}
>+
>+/* prepare the cache to actually insert the given shaper, doing
>+ * in advance the needed allocations

In comments (all), would it make sense to start the sentence with
capital letter and end it with "."?


>+ */
>+static int sc_prepare_insert(struct net_device *dev, u32 *handle,
>+			     struct netlink_ext_ack *extack)
>+{
>+	enum net_shaper_scope scope = net_shaper_handle_scope(*handle);
>+	struct xarray *xa = __sc_init(dev, extack);
>+	struct net_shaper_info *prev, *cur;
>+	bool id_allocated = false;
>+	int ret, id;
>+
>+	if (!xa)
>+		return -ENOMEM;
>+
>+	cur = xa_load(xa, *handle);
>+	if (cur)
>+		return 0;
>+
>+	/* allocated a new id, if needed */

s/allocated/allocate/ perhaps ?


>+	if (scope == NET_SHAPER_SCOPE_DETACHED &&
>+	    net_shaper_handle_id(*handle) == NET_SHAPER_ID_UNSPEC) {
>+		xa_lock(xa);
>+		id = idr_alloc(&dev->net_shaper_data->detached_ids, NULL,
>+			       0, NET_SHAPER_ID_UNSPEC, GFP_ATOMIC);
>+		xa_unlock(xa);
>+
>+		if (id < 0) {
>+			NL_SET_ERR_MSG(extack, "Can't allocate new id for detached shaper");
>+			return id;
>+		}
>+
>+		*handle = net_shaper_make_handle(scope, id);
>+		id_allocated = true;
>+	}
>+
>+	cur = kmalloc(sizeof(*cur), GFP_KERNEL | __GFP_ZERO);
>+	if (!cur) {
>+		NL_SET_ERR_MSG(extack, "Can't allocate memory for cached shaper");
>+		ret = -ENOMEM;
>+		goto free_id;
>+	}
>+
>+	/* mark 'tentative' shaper inside the cache */
>+	xa_lock(xa);
>+	prev = __xa_store(xa, *handle, cur, GFP_KERNEL);
>+	__xa_set_mark(xa, *handle, XA_MARK_0);

I think it is nice to actually have a define for the mark that indicates
what a certain mark represents.

Also, this patch introduces a bug in net_shaper_nl_get_dumpit()/net_shaper_nl_get_doit()
which now work with uncommitted shapers. You have to check the mark.


>+	xa_unlock(xa);
>+	if (xa_err(prev)) {
>+		NL_SET_ERR_MSG(extack, "Can't insert shaper into cache");
>+		kfree(cur);
>+		ret = xa_err(prev);
>+		goto free_id;
>+	}
>+	return 0;
>+
>+free_id:
>+	if (id_allocated) {
>+		xa_lock(xa);

May be worth mentioning you protect detached_ids with the xa lock.


>+		idr_remove(&dev->net_shaper_data->detached_ids,
>+			   net_shaper_handle_id(*handle));
>+		xa_unlock(xa);
>+	}
>+	return ret;
>+}
>+
>+/* commit the tentative insert with the actual values.
>+ * Must be called only after a successful sc_prepare_insert()
>+ */
>+static void sc_commit(struct net_device *dev, int nr_shapers,
>+		      const struct net_shaper_info *shapers)
>+{
>+	struct xarray *xa = __sc_container(dev);
>+	struct net_shaper_info *cur;
>+	int i;
>+
>+	xa_lock(xa);
>+	for (i = 0; i < nr_shapers; ++i) {
>+		cur = xa_load(xa, shapers[i].handle);
>+		if (WARN_ON_ONCE(!cur))
>+			continue;
>+
>+		/* successful update: drop the tentative mark
>+		 * and update the cache
>+		 */
>+		__xa_clear_mark(xa, shapers[i].handle, XA_MARK_0);
>+		*cur = shapers[i];
>+	}
>+	xa_unlock(xa);
>+}
>+
> static int parse_handle(const struct nlattr *attr, const struct genl_info *info,
> 			u32 *handle)
> {
>@@ -154,6 +292,68 @@ static int parse_handle(const struct nlattr *attr, const struct genl_info *info,
> 	return 0;
> }
> 
>+static int __parse_shaper(struct net_device *dev, struct nlattr **tb,

In general, "__" prefix indicates that reader should be careful for any
reason, locking for example. Here, It's a simple helper, isn't it? Why
"__" make sense here?



>+			  const struct genl_info *info,
>+			  struct net_shaper_info *shaper)
>+{
>+	struct net_shaper_info *old;
>+	int ret;
>+
>+	/* the shaper handle is the only mandatory attribute */
>+	if (NL_REQ_ATTR_CHECK(info->extack, NULL, tb, NET_SHAPER_A_HANDLE))
>+		return -EINVAL;
>+
>+	ret = parse_handle(tb[NET_SHAPER_A_HANDLE], info, &shaper->handle);
>+	if (ret)
>+		return ret;
>+
>+	/* fetch existing data, if any, so that user provide info will
>+	 * incrementally update the existing shaper configuration
>+	 */
>+	old = sc_lookup(dev, shaper->handle);
>+	if (old)
>+		*shaper = *old;
>+	else
>+		shaper->parent = default_parent(shaper->handle);
>+
>+	if (tb[NET_SHAPER_A_METRIC])
>+		shaper->metric = nla_get_u32(tb[NET_SHAPER_A_METRIC]);
>+
>+	if (tb[NET_SHAPER_A_BW_MIN])
>+		shaper->bw_min = nla_get_uint(tb[NET_SHAPER_A_BW_MIN]);
>+
>+	if (tb[NET_SHAPER_A_BW_MAX])
>+		shaper->bw_max = nla_get_uint(tb[NET_SHAPER_A_BW_MAX]);
>+
>+	if (tb[NET_SHAPER_A_BURST])
>+		shaper->burst = nla_get_uint(tb[NET_SHAPER_A_BURST]);
>+
>+	if (tb[NET_SHAPER_A_PRIORITY])
>+		shaper->priority = nla_get_u32(tb[NET_SHAPER_A_PRIORITY]);
>+
>+	if (tb[NET_SHAPER_A_WEIGHT])
>+		shaper->weight = nla_get_u32(tb[NET_SHAPER_A_WEIGHT]);
>+	return 0;
>+}
>+
>+/* fetch the cached shaper info and update them with the user-provided
>+ * attributes
>+ */
>+static int parse_shaper(struct net_device *dev, const struct nlattr *attr,
>+			const struct genl_info *info,
>+			struct net_shaper_info *shaper)
>+{
>+	struct nlattr *tb[NET_SHAPER_A_WEIGHT + 1];
>+	int ret;
>+
>+	ret = nla_parse_nested(tb, NET_SHAPER_A_WEIGHT, attr,
>+			       net_shaper_ns_info_nl_policy, info->extack);
>+	if (ret < 0)
>+		return ret;
>+
>+	return __parse_shaper(dev, tb, info, shaper);
>+}
>+
> int net_shaper_nl_get_doit(struct sk_buff *skb, struct genl_info *info)
> {
> 	struct net_shaper_info *shaper;
>@@ -239,14 +439,133 @@ int net_shaper_nl_get_dumpit(struct sk_buff *skb,
> 	return ret;
> }
> 
>+/* Update the H/W and on success update the local cache, too */
>+static int net_shaper_set(struct net_device *dev,
>+			  const struct net_shaper_info *shaper,
>+			  struct netlink_ext_ack *extack)
>+{
>+	enum net_shaper_scope scope;
>+	u32 handle = shaper->handle;
>+	int ret;
>+
>+	scope = net_shaper_handle_scope(handle);
>+	if (scope == NET_SHAPER_SCOPE_PORT ||

NET_SHAPER_SCOPE_PORT is not really used in this set. Why do you have
it? Same applies tol NET_SHAPER_SCOPE_NETDEV.


>+	    scope == NET_SHAPER_SCOPE_UNSPEC) {
>+		NL_SET_ERR_MSG_FMT(extack, "Can't set shaper %x with scope %d",
>+				   handle, scope);
>+		return -EINVAL;
>+	}
>+	if (scope == NET_SHAPER_SCOPE_DETACHED && !sc_lookup(dev, handle)) {
>+		NL_SET_ERR_MSG_FMT(extack, "Shaper %x with detached scope does not exist",
>+				   handle);
>+		return -EINVAL;
>+	}
>+
>+	ret = sc_prepare_insert(dev, &handle, extack);
>+	if (ret)
>+		return ret;
>+
>+	ret = dev->netdev_ops->net_shaper_ops->set(dev, shaper, extack);
>+	sc_commit(dev, 1, shaper);

Missing rollback?


>+	return ret;
>+}
>+
> int net_shaper_nl_set_doit(struct sk_buff *skb, struct genl_info *info)
> {
>-	return -EOPNOTSUPP;
>+	struct net_shaper_info shaper;
>+	struct net_device *dev;
>+	struct nlattr *attr;
>+	int ret;
>+
>+	if (GENL_REQ_ATTR_CHECK(info, NET_SHAPER_A_SHAPER))
>+		return -EINVAL;
>+
>+	ret = fetch_dev(info, &dev);
>+	if (ret)
>+		return ret;
>+
>+	attr = info->attrs[NET_SHAPER_A_SHAPER];
>+	ret = parse_shaper(dev, attr, info, &shaper);
>+	if (ret)
>+		goto put;
>+
>+	ret = net_shaper_set(dev, &shaper, info->extack);

Hmm, if I don't miss anything, this is lockless (.parallel_ops==true).
What's stopping user from performing the set action in parallel on a
single shaper? The locking scheme seems to be missing. Another thing to
describe in documentation, perhaps.



>+
>+put:
>+	dev_put(dev);
>+	return ret;
>+}
>+
>+static int net_shaper_delete(struct net_device *dev, u32 handle,
>+			     struct netlink_ext_ack *extack)
>+{
>+	struct net_shaper_info *parent, *shaper = sc_lookup(dev, handle);
>+	struct xarray *xa = __sc_container(dev);
>+	enum net_shaper_scope pscope;
>+	u32 parent_handle;
>+	int ret;
>+
>+	if (!xa || !shaper) {
>+		NL_SET_ERR_MSG_FMT(extack, "Shaper %x not found", handle);
>+		return -EINVAL;
>+	}
>+
>+	if (is_detached(handle) && shaper->children > 0) {
>+		NL_SET_ERR_MSG_FMT(extack, "Can't delete detached shaper %d with %d child nodes",
>+				   handle, shaper->children);
>+		return -EINVAL;
>+	}
>+
>+	while (shaper) {
>+		parent_handle = shaper->parent;
>+		pscope = net_shaper_handle_scope(parent_handle);
>+
>+		ret = dev->netdev_ops->net_shaper_ops->delete(dev, handle, extack);
>+		if (ret < 0)
>+			return ret;
>+
>+		xa_lock(xa);
>+		__xa_erase(xa, handle);

Hmm, did you think about free of dev->net_shaper_data in case this is
the last item? I think it would be correct to do that.


>+		if (is_detached(handle))
>+			idr_remove(&dev->net_shaper_data->detached_ids,
>+				   net_shaper_handle_id(handle));
>+		xa_unlock(xa);
>+		kfree(shaper);
>+		shaper = NULL;
>+
>+		if (pscope == NET_SHAPER_SCOPE_DETACHED) {
>+			parent = sc_lookup(dev, parent_handle);
>+			if (parent && !--parent->children) {
>+				shaper = parent;
>+				handle = parent_handle;
>+			}
>+		}
>+	}
>+	return 0;
> }
> 
> int net_shaper_nl_delete_doit(struct sk_buff *skb, struct genl_info *info)
> {
>-	return -EOPNOTSUPP;
>+	struct net_device *dev;
>+	u32 handle;
>+	int ret;
>+
>+	if (GENL_REQ_ATTR_CHECK(info, NET_SHAPER_A_HANDLE))
>+		return -EINVAL;
>+
>+	ret = fetch_dev(info, &dev);
>+	if (ret)
>+		return ret;
>+
>+	ret = parse_handle(info->attrs[NET_SHAPER_A_HANDLE], info, &handle);
>+	if (ret)
>+		goto put;
>+
>+	ret = net_shaper_delete(dev, handle, info->extack);
>+
>+put:
>+	dev_put(dev);
>+	return ret;
> }
> 
> int net_shaper_nl_group_doit(struct sk_buff *skb, struct genl_info *info)
>-- 
>2.45.2
>

Powered by blists - more mailing lists