| Message ID | e79b8d955a854772b11b84997c4627794ad160ee.1722357745.git.pabeni@redhat.com (mailing list archive) |
|---|---|
| State | Changes Requested |
| Delegated to: | Netdev Maintainers |
| Headers | show |
| Series | net: introduce TX H/W shaping API | expand |
On Tue, 30 Jul 2024 22:39:47 +0200 Paolo Abeni wrote: > +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); below the print format for shaper id is %d also just point at the attribute with NL_SET_BAD_ATTR(), we shouldn't bloat the kernel with strings for trivial errors > + return -EINVAL; ENOENT > + } > + > + 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; I'd say EBUSY > + } > + > + 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); > + if (is_detached(handle)) > + idr_remove(&dev->net_shaper_data->detached_ids, > + net_shaper_handle_id(handle)); > + xa_unlock(xa); > + kfree(shaper); > + shaper = NULL; IIUC child is the input / ingress node? (The parentage terminology is a bit ambiguous in this case, although I must admit I keep catching myself trying to use it, too). Does "deleting a queue" return it to the "implicit mux" at the global level? If we look at the delegation use case - when queue is "deleted" from a container-controlled mux it should go back to the group created by the orchestrator, not "escpate" to global scope, right? > + 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; Possibly a candidate for a "pre" handler, since multiple ops call it?
On 8/1/24 17:00, Jakub Kicinski wrote: > On Tue, 30 Jul 2024 22:39:47 +0200 Paolo Abeni wrote: >> + 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); >> + if (is_detached(handle)) >> + idr_remove(&dev->net_shaper_data->detached_ids, >> + net_shaper_handle_id(handle)); >> + xa_unlock(xa); >> + kfree(shaper); >> + shaper = NULL; > > IIUC child is the input / ingress node? Yes. > Does "deleting a queue" return it to the "implicit mux" at the > global level? Yes > If we look at the delegation use case - when queue > is "deleted" from a container-controlled mux it should go back to > the group created by the orchestrator, not "escpate" to global scope, > right? When deleting a queue-level shaper, the orchestrator is "returning" the ownership of the queue from the container to the host. If the container wants to move the queue around e.g. from: q1 ----- \ q2 - \SP1/ RR1 q3 - / \ q4 - \ RR2 -> RR(root) q5 - / / q6 - \ RR3 q7 - / to: q1 ----- \ q2 ----- RR1 q3 ---- / \ q4 - \ RR2 -> RR(root) q5 - / / q6 - \ RR3 q7 - / It can do it with a group() operation: group(inputs:[q2,q3],output:[RR1]) That will implicitly also delete SP1. Side note, I just noticed that the current code is bugged WRT this last operation and will not delete SP1. Cheers, Paolo
On Thu, 1 Aug 2024 17:25:50 +0200 Paolo Abeni wrote: > When deleting a queue-level shaper, the orchestrator is "returning" the > ownership of the queue from the container to the host. If the container > wants to move the queue around e.g. from: > > q1 ----- \ > q2 - \SP1/ RR1 > q3 - / \ > q4 - \ RR2 -> RR(root) > q5 - / / > q6 - \ RR3 > q7 - / > > to: > > q1 ----- \ > q2 ----- RR1 > q3 ---- / \ > q4 - \ RR2 -> RR(root) > q5 - / / > q6 - \ RR3 > q7 - / > > It can do it with a group() operation: > > group(inputs:[q2,q3],output:[RR1]) Isn't that a bit odd? The container was not supposed to know / care about RR1's existence. We achieve this with group() by implicitly inheriting the egress node if all grouped entities shared one. Delete IMO should act here like a "ungroup" operation, meaning that: 1) we're deleting SP1, not q1, q2 2) inputs go "downstream" instead getting ejected into global level Also, in the first example from the cover letter we "set" a shaper on the queue, it feels a little ambiguous whether "delete queue" is purely clearing such per-queue shaping, or also has implications for the hierarchy. Coincidentally, others may disagree, but I'd point to tests in patch 8 for examples of how the thing works, instead the cover letter samples. > That will implicitly also delete SP1.
Thu, Aug 01, 2024 at 05:39:24PM CEST, kuba@kernel.org wrote: >On Thu, 1 Aug 2024 17:25:50 +0200 Paolo Abeni wrote: >> When deleting a queue-level shaper, the orchestrator is "returning" the >> ownership of the queue from the container to the host. If the container What do you meam by "orchestrator" and "container" here? I'm missing these from the picture. >> wants to move the queue around e.g. from: >> >> q1 ----- \ >> q2 - \SP1/ RR1 What "sp" and "rr" stand for. What are the "scopes" of these? >> q3 - / \ >> q4 - \ RR2 -> RR(root) >> q5 - / / >> q6 - \ RR3 >> q7 - / >> >> to: >> >> q1 ----- \ >> q2 ----- RR1 >> q3 ---- / \ >> q4 - \ RR2 -> RR(root) >> q5 - / / >> q6 - \ RR3 >> q7 - / >> >> It can do it with a group() operation: >> >> group(inputs:[q2,q3],output:[RR1]) > >Isn't that a bit odd? The container was not supposed to know / care >about RR1's existence. We achieve this with group() by implicitly >inheriting the egress node if all grouped entities shared one. > >Delete IMO should act here like a "ungroup" operation, meaning that: > 1) we're deleting SP1, not q1, q2 Does current code support removing SP1? I mean, if the scope is detached, I don't think so. > 2) inputs go "downstream" instead getting ejected into global level > >Also, in the first example from the cover letter we "set" a shaper on >the queue, it feels a little ambiguous whether "delete queue" is >purely clearing such per-queue shaping, or also has implications >for the hierarchy. > >Coincidentally, others may disagree, but I'd point to tests in patch >8 for examples of how the thing works, instead the cover letter samples. Examples in cover letter are generally beneficial. Don't remove them :) > >> That will implicitly also delete SP1.
On Fri, 2 Aug 2024 18:15:32 +0200 Jiri Pirko wrote: > Thu, Aug 01, 2024 at 05:39:24PM CEST, kuba@kernel.org wrote: > >On Thu, 1 Aug 2024 17:25:50 +0200 Paolo Abeni wrote: > >> When deleting a queue-level shaper, the orchestrator is "returning" the > >> ownership of the queue from the container to the host. If the container > > What do you meam by "orchestrator" and "container" here? I'm missing > these from the picture. Container (as in docker) and orchestrator. > >> wants to move the queue around e.g. from: > >> > >> q1 ----- \ > >> q2 - \SP1/ RR1 > > What "sp" and "rr" stand for. What are the "scopes" of these? "scopes" I agree are confusing, but: sp = strict priority rr = round robin > >> q3 - / \ > >> q4 - \ RR2 -> RR(root) > >> q5 - / / > >> q6 - \ RR3 > >> q7 - / > >> > >> to: > >> > >> q1 ----- \ > >> q2 ----- RR1 > >> q3 ---- / \ > >> q4 - \ RR2 -> RR(root) > >> q5 - / / > >> q6 - \ RR3 > >> q7 - / > >> > >> It can do it with a group() operation: > >> > >> group(inputs:[q2,q3],output:[RR1]) > > > >Isn't that a bit odd? The container was not supposed to know / care > >about RR1's existence. We achieve this with group() by implicitly > >inheriting the egress node if all grouped entities shared one. > > > >Delete IMO should act here like a "ungroup" operation, meaning that: > > 1) we're deleting SP1, not q1, q2 > > Does current code support removing SP1? I mean, if the scope is > detached, I don't think so. that's my reading too, fwiw > > 2) inputs go "downstream" instead getting ejected into global level > > > >Also, in the first example from the cover letter we "set" a shaper on > >the queue, it feels a little ambiguous whether "delete queue" is > >purely clearing such per-queue shaping, or also has implications > >for the hierarchy. > > > >Coincidentally, others may disagree, but I'd point to tests in patch > >8 for examples of how the thing works, instead the cover letter samples. > > Examples in cover letter are generally beneficial. Don't remove them :) They are beneficial, but if I was to order the following three forms of documentation by priority: - ReST under Documentation/ - clear selftests with comments - cover letter I'm uncertain which will be first, but cover letter is definitely last :( With the examples in the cover letter its unclear what the expected start and end state are. And where the values come from. I feel like selftest would make it clearer. But I don't feel strongly. Such newfangled ideas will take a while to take root :)
On 8/2/24 18:15, Jiri Pirko wrote: > Thu, Aug 01, 2024 at 05:39:24PM CEST, kuba@kernel.org wrote: >> On Thu, 1 Aug 2024 17:25:50 +0200 Paolo Abeni wrote: >>> When deleting a queue-level shaper, the orchestrator is "returning" the >>> ownership of the queue from the container to the host. If the container > > What do you meam by "orchestrator" and "container" here? I'm missing > these from the picture. > > >>> wants to move the queue around e.g. from: >>> >>> q1 ----- \ >>> q2 - \SP1/ RR1 > > What "sp" and "rr" stand for. What are the "scopes" of these? The scope is 'detached' >>> q3 - / \ >>> q4 - \ RR2 -> RR(root) >>> q5 - / / >>> q6 - \ RR3 >>> q7 - / >>> >>> to: >>> >>> q1 ----- \ >>> q2 ----- RR1 >>> q3 ---- / \ >>> q4 - \ RR2 -> RR(root) >>> q5 - / / >>> q6 - \ RR3 >>> q7 - / >>> >>> It can do it with a group() operation: >>> >>> group(inputs:[q2,q3],output:[RR1]) >> >> Isn't that a bit odd? The container was not supposed to know / care >> about RR1's existence. We achieve this with group() by implicitly >> inheriting the egress node if all grouped entities shared one. >> >> Delete IMO should act here like a "ungroup" operation, meaning that: >> 1) we're deleting SP1, not q1, q2 > > Does current code support removing SP1? I mean, if the scope is > detached, I don't think so. The current code explicitly prevents the above. We can change such behavior, if there is agreement. My understanding is that Donald is against such option. >> 2) inputs go "downstream" instead getting ejected into global level >> >> Also, in the first example from the cover letter we "set" a shaper on >> the queue, it feels a little ambiguous whether "delete queue" is >> purely clearing such per-queue shaping, or also has implications >> for the hierarchy. >> >> Coincidentally, others may disagree, but I'd point to tests in patch >> 8 for examples of how the thing works, instead the cover letter samples. > > Examples in cover letter are generally beneficial. Don't remove them :) No problem to keep both examples and self-tests. Thanks, Paolo
diff --git a/net/shaper/shaper.c b/net/shaper/shaper.c index 5d1d6e600a6a..8ecbfd9002be 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); + + 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: + 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 + */ +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 */ + 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); + 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); + 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, + 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,134 @@ 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 || + 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); + 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); + +put: + netdev_put(dev, NULL); + 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); + 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: + netdev_put(dev, NULL); + return ret; } int net_shaper_nl_group_doit(struct sk_buff *skb, struct genl_info *info)
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@redhat.com> --- RFC v2 -> RFC v3: - dev_put() -> netdev_put() --- net/shaper/shaper.c | 324 +++++++++++++++++++++++++++++++++++++++++++- 1 file changed, 322 insertions(+), 2 deletions(-)