| Message ID | 20240803042624.970352-10-kuba@kernel.org (mailing list archive) |
|---|---|
| State | Superseded |
| Delegated to: | Netdev Maintainers |
| Headers | show |
| Series | ethtool: rss: driver tweaks and netlink context dumps | expand |
On Fri, Aug 02, 2024 at 09:26:21PM -0700, Jakub Kicinski wrote: > Now that we track RSS contexts in the core we can easily dump > them. This is a major introspection improvement, as previously > the only way to find all contexts would be to try all ids > (of which there may be 2^32 - 1). > > Signed-off-by: Jakub Kicinski <kuba@kernel.org> > --- Thanks for doing this important and extremely useful work. I am personally very excited to see this data available to userland. [...] > diff --git a/net/ethtool/rss.c b/net/ethtool/rss.c > index 023782ca1230..62e7b6fe605d 100644 > --- a/net/ethtool/rss.c > +++ b/net/ethtool/rss.c > @@ -208,6 +208,139 @@ static void rss_cleanup_data(struct ethnl_reply_data *reply_base) > kfree(data->indir_table); > } > > +struct rss_nl_dump_ctx { > + unsigned long ifindex; > + unsigned long ctx_idx; > + > + unsigned int one_ifindex; My apologies: I'm probably just not familiar enough with the code, but I'm having a hard time understanding what the purpose of one_ifindex is. I read both ethnl_rss_dump_start and ethnl_rss_dumpit, but I'm still not following what this is used for; it'll probably be obvious in retrospect once you explain it, but I suppose my feedback is that a comment or something would be really helpful :) > +}; > + > +static struct rss_nl_dump_ctx *rss_dump_ctx(struct netlink_callback *cb) > +{ > + NL_ASSERT_DUMP_CTX_FITS(struct rss_nl_dump_ctx); > + > + return (struct rss_nl_dump_ctx *)cb->ctx; > +} > + > +int ethnl_rss_dump_start(struct netlink_callback *cb) > +{ > + const struct genl_info *info = genl_info_dump(cb); > + struct rss_nl_dump_ctx *ctx = rss_dump_ctx(cb); > + struct ethnl_req_info req_info = {}; > + struct nlattr **tb = info->attrs; > + int ret; > + > + /* Filtering by context not supported */ > + if (tb[ETHTOOL_A_RSS_CONTEXT]) { > + NL_SET_BAD_ATTR(info->extack, tb[ETHTOOL_A_RSS_CONTEXT]); > + return -EINVAL; > + } > + > + ret = ethnl_parse_header_dev_get(&req_info, > + tb[ETHTOOL_A_RSS_HEADER], > + sock_net(cb->skb->sk), cb->extack, > + false); > + if (req_info.dev) { > + ctx->one_ifindex = req_info.dev->ifindex; > + ctx->ifindex = ctx->one_ifindex; > + ethnl_parse_header_dev_put(&req_info); > + req_info.dev = NULL; > + } > + > + return ret; > +} > + > +static int > +rss_dump_one_ctx(struct sk_buff *skb, struct netlink_callback *cb, > + struct net_device *dev, u32 rss_context) > +{ > + const struct genl_info *info = genl_info_dump(cb); > + struct rss_reply_data data = {}; > + struct rss_req_info req = {}; > + void *ehdr; > + int ret; > + > + req.rss_context = rss_context; > + > + ehdr = ethnl_dump_put(skb, cb, ETHTOOL_MSG_RSS_GET_REPLY); > + if (!ehdr) > + return -EMSGSIZE; > + > + ret = ethnl_fill_reply_header(skb, dev, ETHTOOL_A_RSS_HEADER); > + if (ret < 0) > + goto err_cancel; > + > + if (!rss_context) > + ret = rss_prepare_get(&req, dev, &data, info); > + else > + ret = rss_prepare_ctx(&req, dev, &data, info); > + if (ret) > + goto err_cancel; > + > + ret = rss_fill_reply(skb, &req.base, &data.base); > + if (ret) > + goto err_cleanup; > + genlmsg_end(skb, ehdr); > + > + rss_cleanup_data(&data.base); > + return 0; > + > +err_cleanup: > + rss_cleanup_data(&data.base); > +err_cancel: > + genlmsg_cancel(skb, ehdr); > + return ret; > +} > + > +static int > +rss_dump_one_dev(struct sk_buff *skb, struct netlink_callback *cb, > + struct net_device *dev) > +{ > + struct rss_nl_dump_ctx *ctx = rss_dump_ctx(cb); > + int ret; > + > + if (!dev->ethtool_ops->get_rxfh) > + return 0; > + > + if (!ctx->ctx_idx) { > + ret = rss_dump_one_ctx(skb, cb, dev, 0); > + if (ret) > + return ret; > + ctx->ctx_idx++; > + } > + > + for (; xa_find(&dev->ethtool->rss_ctx, &ctx->ctx_idx, > + ULONG_MAX, XA_PRESENT); ctx->ctx_idx++) { > + ret = rss_dump_one_ctx(skb, cb, dev, ctx->ctx_idx); > + if (ret) > + return ret; > + } > + ctx->ctx_idx = 0; > + > + return 0; > +} > + > +int ethnl_rss_dumpit(struct sk_buff *skb, struct netlink_callback *cb) > +{ > + struct rss_nl_dump_ctx *ctx = rss_dump_ctx(cb); > + struct net *net = sock_net(skb->sk); > + struct net_device *dev; > + int ret = 0; > + > + rtnl_lock(); > + for_each_netdev_dump(net, dev, ctx->ifindex) { > + if (ctx->one_ifindex && ctx->one_ifindex != ctx->ifindex) > + break; > + > + ret = rss_dump_one_dev(skb, cb, dev); > + if (ret) > + break; > + } > + rtnl_unlock(); > + > + return ret; > +} > + > const struct ethnl_request_ops ethnl_rss_request_ops = { > .request_cmd = ETHTOOL_MSG_RSS_GET, > .reply_cmd = ETHTOOL_MSG_RSS_GET_REPLY, > -- > 2.45.2 >
On Sat, 3 Aug 2024 19:11:28 +0100 Joe Damato wrote: > > +struct rss_nl_dump_ctx { > > + unsigned long ifindex; > > + unsigned long ctx_idx; > > + > > + unsigned int one_ifindex; > > My apologies: I'm probably just not familiar enough with the code, > but I'm having a hard time understanding what the purpose of > one_ifindex is. > > I read both ethnl_rss_dump_start and ethnl_rss_dumpit, but I'm still > not following what this is used for; it'll probably be obvious in > retrospect once you explain it, but I suppose my feedback is that a > comment or something would be really helpful :) Better name would probably help, but can't think of any. User can (optionally) pass an ifindex/ifname to the dump, to dump contexts only for the specified ifindex. If they do we "preset" the ifindex and one_ifindex: + if (req_info.dev) { + ctx->one_ifindex = req_info.dev->ifindex; + ctx->ifindex = ctx->one_ifindex; + ethnl_parse_header_dev_put(&req_info); + req_info.dev = NULL; + } and then the iteration is stopped after first full pass: + rtnl_lock(); + for_each_netdev_dump(net, dev, ctx->ifindex) { + if (ctx->one_ifindex && ctx->one_ifindex != ctx->ifindex) + break; Unfortunately we don't have any best practice for handling filtering in dumps. I find this cleaner than approaches I previously tried, but we'll see if it stands the test of time. I'll add the following comment: /* User wants to dump contexts for one ifindex only */
On Mon, Aug 05, 2024 at 02:59:33PM -0700, Jakub Kicinski wrote: > On Sat, 3 Aug 2024 19:11:28 +0100 Joe Damato wrote: > > > +struct rss_nl_dump_ctx { > > > + unsigned long ifindex; > > > + unsigned long ctx_idx; > > > + > > > + unsigned int one_ifindex; > > > > My apologies: I'm probably just not familiar enough with the code, > > but I'm having a hard time understanding what the purpose of > > one_ifindex is. > > > > I read both ethnl_rss_dump_start and ethnl_rss_dumpit, but I'm still > > not following what this is used for; it'll probably be obvious in > > retrospect once you explain it, but I suppose my feedback is that a > > comment or something would be really helpful :) > > Better name would probably help, but can't think of any. > > User can (optionally) pass an ifindex/ifname to the dump, to dump > contexts only for the specified ifindex. If they do we "preset" > the ifindex and one_ifindex: > + if (req_info.dev) { > + ctx->one_ifindex = req_info.dev->ifindex; > + ctx->ifindex = ctx->one_ifindex; > + ethnl_parse_header_dev_put(&req_info); > + req_info.dev = NULL; > + } > > and then the iteration is stopped after first full pass: > > + rtnl_lock(); > + for_each_netdev_dump(net, dev, ctx->ifindex) { > + if (ctx->one_ifindex && ctx->one_ifindex != ctx->ifindex) > + break; Ah, OK; that all makes sense. Thanks for the explanation. > Unfortunately we don't have any best practice for handling filtering > in dumps. I find this cleaner than approaches I previously tried, but > we'll see if it stands the test of time. > > I'll add the following comment: > > /* User wants to dump contexts for one ifindex only */ Sounds good. If you like, you can also add: Reviewed-by: Joe Damato <jdamato@fastly.com>
On 8/5/24 23:59, Jakub Kicinski wrote: > On Sat, 3 Aug 2024 19:11:28 +0100 Joe Damato wrote: >>> +struct rss_nl_dump_ctx { >>> + unsigned long ifindex; >>> + unsigned long ctx_idx; >>> + >>> + unsigned int one_ifindex; >> >> My apologies: I'm probably just not familiar enough with the code, >> but I'm having a hard time understanding what the purpose of >> one_ifindex is. >> >> I read both ethnl_rss_dump_start and ethnl_rss_dumpit, but I'm still >> not following what this is used for; it'll probably be obvious in >> retrospect once you explain it, but I suppose my feedback is that a >> comment or something would be really helpful :) > > Better name would probably help, but can't think of any. perhaps: ifindex -> if_iter one_ifindex -> if_requested (I would also like 'ifc' instead of 'if', for the obvious reasons) > > User can (optionally) pass an ifindex/ifname to the dump, to dump > contexts only for the specified ifindex. If they do we "preset" > the ifindex and one_ifindex: > > + if (req_info.dev) { > + ctx->one_ifindex = req_info.dev->ifindex; > + ctx->ifindex = ctx->one_ifindex; > + ethnl_parse_header_dev_put(&req_info); > + req_info.dev = NULL; > + } > > and then the iteration is stopped after first full pass: > > + rtnl_lock(); > + for_each_netdev_dump(net, dev, ctx->ifindex) { > + if (ctx->one_ifindex && ctx->one_ifindex != ctx->ifindex) > + break; > > Unfortunately we don't have any best practice for handling filtering > in dumps. I find this cleaner than approaches I previously tried, but > we'll see if it stands the test of time. This code is clean, but by just looking at the struct one could not expect it though :/ Perhaps a rename could help, or just wait until people learn it (I remember you have recently explained this dump scheme to me :)) > > I'll add the following comment: > > /* User wants to dump contexts for one ifindex only */
On 05/08/2024 22:59, Jakub Kicinski wrote: > On Sat, 3 Aug 2024 19:11:28 +0100 Joe Damato wrote: >>> +struct rss_nl_dump_ctx { >>> + unsigned long ifindex; >>> + unsigned long ctx_idx; >>> + >>> + unsigned int one_ifindex; >> >> My apologies: I'm probably just not familiar enough with the code, >> but I'm having a hard time understanding what the purpose of >> one_ifindex is. >> >> I read both ethnl_rss_dump_start and ethnl_rss_dumpit, but I'm still >> not following what this is used for; it'll probably be obvious in >> retrospect once you explain it, but I suppose my feedback is that a >> comment or something would be really helpful :) > > Better name would probably help, but can't think of any. 'only_ifindex'? 'match_ifindex'?
On Tue, 6 Aug 2024 14:58:59 +0100 Edward Cree wrote: > > Better name would probably help, but can't think of any. > > 'only_ifindex'? 'match_ifindex'? 'match' sounds good, I'll use that, thanks!
On 03/08/2024 05:26, Jakub Kicinski wrote: > Now that we track RSS contexts in the core we can easily dump > them. This is a major introspection improvement, as previously > the only way to find all contexts would be to try all ids > (of which there may be 2^32 - 1). > > Signed-off-by: Jakub Kicinski <kuba@kernel.org> ... > + if (!ctx->ctx_idx) { > + ret = rss_dump_one_ctx(skb, cb, dev, 0); > + if (ret) > + return ret; > + ctx->ctx_idx++; > + } Maybe comment this block with something like "context 0 is not stored in the XArray" to make clear why this is split out from a loop that looks like it should be able to handle it. > + > + for (; xa_find(&dev->ethtool->rss_ctx, &ctx->ctx_idx, > + ULONG_MAX, XA_PRESENT); ctx->ctx_idx++) { > + ret = rss_dump_one_ctx(skb, cb, dev, ctx->ctx_idx); > + if (ret) > + return ret; > + } > + ctx->ctx_idx = 0; Feels like there has to be a way to do this with xa_for_each_start()? Something like (untested): struct ethtool_rxfh_context *rss_ctx; xa_for_each_start(&dev->ethtool->rss_ctx, ctx->ctx_idx, rss_ctx, ctx->ctx_idx) { ret = rss_dump_one_ctx(skb, cb, dev, ctx->ctx_idx); if (ret) return ret; } ctx->ctx_idx = 0;
On Tue, 6 Aug 2024 15:24:50 +0100 Edward Cree wrote: > > + if (!ctx->ctx_idx) { > > + ret = rss_dump_one_ctx(skb, cb, dev, 0); > > + if (ret) > > + return ret; > > + ctx->ctx_idx++; > > + } > > Maybe comment this block with something like "context 0 is > not stored in the XArray" to make clear why this is split > out from a loop that looks like it should be able to handle > it. Will do. > > + > > + for (; xa_find(&dev->ethtool->rss_ctx, &ctx->ctx_idx, > > + ULONG_MAX, XA_PRESENT); ctx->ctx_idx++) { > > + ret = rss_dump_one_ctx(skb, cb, dev, ctx->ctx_idx); > > + if (ret) > > + return ret; > > + } > > + ctx->ctx_idx = 0; > > Feels like there has to be a way to do this with > xa_for_each_start()? Something like (untested): It may work in the current code but I prefer to stay away from xarray iterators in netlink code. They cause too many bugs. They do not invalidate / move the index past the end of the array once they are done. Which means if the dump ever gets called again after finishing we'll re-dump the last element.
diff --git a/Documentation/netlink/specs/ethtool.yaml b/Documentation/netlink/specs/ethtool.yaml index ea21fe135b97..cf69eedae51d 100644 --- a/Documentation/netlink/specs/ethtool.yaml +++ b/Documentation/netlink/specs/ethtool.yaml @@ -1749,12 +1749,12 @@ doc: Partial family for Ethtool Netlink. attribute-set: rss - do: &rss-get-op + do: request: attributes: - header - context - reply: + reply: &rss-reply attributes: - header - context @@ -1762,6 +1762,11 @@ doc: Partial family for Ethtool Netlink. - indir - hkey - input_xfrm + dump: + request: + attributes: + - header + reply: *rss-reply - name: plca-get-cfg doc: Get PLCA params. diff --git a/net/ethtool/netlink.c b/net/ethtool/netlink.c index cb1eea00e349..041548e5f5e6 100644 --- a/net/ethtool/netlink.c +++ b/net/ethtool/netlink.c @@ -1128,6 +1128,8 @@ static const struct genl_ops ethtool_genl_ops[] = { { .cmd = ETHTOOL_MSG_RSS_GET, .doit = ethnl_default_doit, + .start = ethnl_rss_dump_start, + .dumpit = ethnl_rss_dumpit, .policy = ethnl_rss_get_policy, .maxattr = ARRAY_SIZE(ethnl_rss_get_policy) - 1, }, diff --git a/net/ethtool/netlink.h b/net/ethtool/netlink.h index 46ec273a87c5..919371383b23 100644 --- a/net/ethtool/netlink.h +++ b/net/ethtool/netlink.h @@ -464,6 +464,8 @@ int ethnl_tunnel_info_doit(struct sk_buff *skb, struct genl_info *info); int ethnl_tunnel_info_start(struct netlink_callback *cb); int ethnl_tunnel_info_dumpit(struct sk_buff *skb, struct netlink_callback *cb); int ethnl_act_module_fw_flash(struct sk_buff *skb, struct genl_info *info); +int ethnl_rss_dump_start(struct netlink_callback *cb); +int ethnl_rss_dumpit(struct sk_buff *skb, struct netlink_callback *cb); extern const char stats_std_names[__ETHTOOL_STATS_CNT][ETH_GSTRING_LEN]; extern const char stats_eth_phy_names[__ETHTOOL_A_STATS_ETH_PHY_CNT][ETH_GSTRING_LEN]; diff --git a/net/ethtool/rss.c b/net/ethtool/rss.c index 023782ca1230..62e7b6fe605d 100644 --- a/net/ethtool/rss.c +++ b/net/ethtool/rss.c @@ -208,6 +208,139 @@ static void rss_cleanup_data(struct ethnl_reply_data *reply_base) kfree(data->indir_table); } +struct rss_nl_dump_ctx { + unsigned long ifindex; + unsigned long ctx_idx; + + unsigned int one_ifindex; +}; + +static struct rss_nl_dump_ctx *rss_dump_ctx(struct netlink_callback *cb) +{ + NL_ASSERT_DUMP_CTX_FITS(struct rss_nl_dump_ctx); + + return (struct rss_nl_dump_ctx *)cb->ctx; +} + +int ethnl_rss_dump_start(struct netlink_callback *cb) +{ + const struct genl_info *info = genl_info_dump(cb); + struct rss_nl_dump_ctx *ctx = rss_dump_ctx(cb); + struct ethnl_req_info req_info = {}; + struct nlattr **tb = info->attrs; + int ret; + + /* Filtering by context not supported */ + if (tb[ETHTOOL_A_RSS_CONTEXT]) { + NL_SET_BAD_ATTR(info->extack, tb[ETHTOOL_A_RSS_CONTEXT]); + return -EINVAL; + } + + ret = ethnl_parse_header_dev_get(&req_info, + tb[ETHTOOL_A_RSS_HEADER], + sock_net(cb->skb->sk), cb->extack, + false); + if (req_info.dev) { + ctx->one_ifindex = req_info.dev->ifindex; + ctx->ifindex = ctx->one_ifindex; + ethnl_parse_header_dev_put(&req_info); + req_info.dev = NULL; + } + + return ret; +} + +static int +rss_dump_one_ctx(struct sk_buff *skb, struct netlink_callback *cb, + struct net_device *dev, u32 rss_context) +{ + const struct genl_info *info = genl_info_dump(cb); + struct rss_reply_data data = {}; + struct rss_req_info req = {}; + void *ehdr; + int ret; + + req.rss_context = rss_context; + + ehdr = ethnl_dump_put(skb, cb, ETHTOOL_MSG_RSS_GET_REPLY); + if (!ehdr) + return -EMSGSIZE; + + ret = ethnl_fill_reply_header(skb, dev, ETHTOOL_A_RSS_HEADER); + if (ret < 0) + goto err_cancel; + + if (!rss_context) + ret = rss_prepare_get(&req, dev, &data, info); + else + ret = rss_prepare_ctx(&req, dev, &data, info); + if (ret) + goto err_cancel; + + ret = rss_fill_reply(skb, &req.base, &data.base); + if (ret) + goto err_cleanup; + genlmsg_end(skb, ehdr); + + rss_cleanup_data(&data.base); + return 0; + +err_cleanup: + rss_cleanup_data(&data.base); +err_cancel: + genlmsg_cancel(skb, ehdr); + return ret; +} + +static int +rss_dump_one_dev(struct sk_buff *skb, struct netlink_callback *cb, + struct net_device *dev) +{ + struct rss_nl_dump_ctx *ctx = rss_dump_ctx(cb); + int ret; + + if (!dev->ethtool_ops->get_rxfh) + return 0; + + if (!ctx->ctx_idx) { + ret = rss_dump_one_ctx(skb, cb, dev, 0); + if (ret) + return ret; + ctx->ctx_idx++; + } + + for (; xa_find(&dev->ethtool->rss_ctx, &ctx->ctx_idx, + ULONG_MAX, XA_PRESENT); ctx->ctx_idx++) { + ret = rss_dump_one_ctx(skb, cb, dev, ctx->ctx_idx); + if (ret) + return ret; + } + ctx->ctx_idx = 0; + + return 0; +} + +int ethnl_rss_dumpit(struct sk_buff *skb, struct netlink_callback *cb) +{ + struct rss_nl_dump_ctx *ctx = rss_dump_ctx(cb); + struct net *net = sock_net(skb->sk); + struct net_device *dev; + int ret = 0; + + rtnl_lock(); + for_each_netdev_dump(net, dev, ctx->ifindex) { + if (ctx->one_ifindex && ctx->one_ifindex != ctx->ifindex) + break; + + ret = rss_dump_one_dev(skb, cb, dev); + if (ret) + break; + } + rtnl_unlock(); + + return ret; +} + const struct ethnl_request_ops ethnl_rss_request_ops = { .request_cmd = ETHTOOL_MSG_RSS_GET, .reply_cmd = ETHTOOL_MSG_RSS_GET_REPLY,
Now that we track RSS contexts in the core we can easily dump them. This is a major introspection improvement, as previously the only way to find all contexts would be to try all ids (of which there may be 2^32 - 1). Signed-off-by: Jakub Kicinski <kuba@kernel.org> --- Documentation/netlink/specs/ethtool.yaml | 9 +- net/ethtool/netlink.c | 2 + net/ethtool/netlink.h | 2 + net/ethtool/rss.c | 133 +++++++++++++++++++++++ 4 files changed, 144 insertions(+), 2 deletions(-)