| Message ID | 20240913093828.2549217-1-vladimir.oltean@nxp.com (mailing list archive) |
|---|---|
| State | New |
| Delegated to: | Michal Kubecek |
| Headers | show |
| Series | [ethtool] netlink: rss: retrieve ring count using ETHTOOL_GRXRINGS ioctl | expand |
| Context | Check | Description |
|---|---|---|
| netdev/tree_selection | success | Not a local patch |
On Fri, 13 Sep 2024 12:38:28 +0300 Vladimir Oltean wrote: > Several drivers regressed when ethtool --show-rxfh was converted from > ioctl to netlink. This is because ETHTOOL_GRXRINGS was converted to > ETHTOOL_MSG_CHANNELS_GET, which is semantically equivalent to > ETHTOOL_GCHANNELS but different from ETHTOOL_GRXRINGS. Drivers which > implement ETHTOOL_GRXRINGS do not necessarily implement ETHTOOL_GCHANNELS > or its netlink equivalent. > > According to the man page, "A channel is an IRQ and the set of queues > that can trigger that IRQ.", which is different from the definition of > a queue/ring. So we shouldn't be attempting to query the # of rings for > the ioctl variant, but the # of channels for the netlink variant anyway. > > Reimplement the args->num_rings retrieval as in do_grxfh(), aka using > the ETHTOOL_GRXRINGS ioctl. Acked-by: Jakub Kicinski <kuba@kernel.org> Thanks for fixing this! Not sure why Sudheer didn't.
> -----Original Message----- > From: Vladimir Oltean <vladimir.oltean@nxp.com> > Sent: Friday, September 13, 2024 2:38 AM > To: netdev@vger.kernel.org > Cc: Michal Kubecek <mkubecek@suse.cz>; Jakub Kicinski > <kuba@kernel.org>; Mogilappagari, Sudheer > <sudheer.mogilappagari@intel.com> > Subject: [PATCH ethtool] netlink: rss: retrieve ring count using > ETHTOOL_GRXRINGS ioctl > > Several drivers regressed when ethtool --show-rxfh was converted from > ioctl to netlink. This is because ETHTOOL_GRXRINGS was converted to > ETHTOOL_MSG_CHANNELS_GET, which is semantically equivalent to > ETHTOOL_GCHANNELS but different from ETHTOOL_GRXRINGS. Drivers which > implement ETHTOOL_GRXRINGS do not necessarily implement > ETHTOOL_GCHANNELS or its netlink equivalent. > > According to the man page, "A channel is an IRQ and the set of queues > that can trigger that IRQ.", which is different from the definition of > a queue/ring. So we shouldn't be attempting to query the # of rings for > the ioctl variant, but the # of channels for the netlink variant > anyway. > > Reimplement the args->num_rings retrieval as in do_grxfh(), aka using > the ETHTOOL_GRXRINGS ioctl. > > - if (tb[ETHTOOL_A_CHANNELS_RX_COUNT]) > - args->num_rings += > mnl_attr_get_u32(tb[ETHTOOL_A_CHANNELS_RX_COUNT]); > - return MNL_CB_OK; > + ret = ioctl_init(ctx, false); > + if (ret) > + return ret; > + > + ret = send_ioctl(ctx, &ring_count); > + if (ret) { > + perror("Cannot get RX ring count"); > + return ret; > + } > + > + args->num_rings = (u32)ring_count.data; > + > + return 0; > } > Hi Vladimir, my understanding is ioctls are not used in ethtool netlink path. Can we use ETHTOOL_MSG_RINGS_GET (tb[ETHTOOL_A_RINGS_RX] member) instead ? Couldn't work on this since I was on sabbatical till this week.
> -----Original Message----- > From: Mogilappagari, Sudheer > Sent: Friday, September 13, 2024 3:43 PM > To: Vladimir Oltean <vladimir.oltean@nxp.com>; netdev@vger.kernel.org > Cc: Michal Kubecek <mkubecek@suse.cz>; Jakub Kicinski <kuba@kernel.org> > Subject: RE: [PATCH ethtool] netlink: rss: retrieve ring count using > ETHTOOL_GRXRINGS ioctl > > > > > -----Original Message----- > > From: Vladimir Oltean <vladimir.oltean@nxp.com> > > Sent: Friday, September 13, 2024 2:38 AM > > To: netdev@vger.kernel.org > > Cc: Michal Kubecek <mkubecek@suse.cz>; Jakub Kicinski > > <kuba@kernel.org>; Mogilappagari, Sudheer > > <sudheer.mogilappagari@intel.com> > > Subject: [PATCH ethtool] netlink: rss: retrieve ring count using > > ETHTOOL_GRXRINGS ioctl > > > > Several drivers regressed when ethtool --show-rxfh was converted from > > ioctl to netlink. This is because ETHTOOL_GRXRINGS was converted to > > ETHTOOL_MSG_CHANNELS_GET, which is semantically equivalent to > > ETHTOOL_GCHANNELS but different from ETHTOOL_GRXRINGS. Drivers which > > implement ETHTOOL_GRXRINGS do not necessarily implement > > ETHTOOL_GCHANNELS or its netlink equivalent. > > > > According to the man page, "A channel is an IRQ and the set of queues > > that can trigger that IRQ.", which is different from the definition > of > > a queue/ring. So we shouldn't be attempting to query the # of rings > > for the ioctl variant, but the # of channels for the netlink variant > > anyway. > > > > Reimplement the args->num_rings retrieval as in do_grxfh(), aka using > > the ETHTOOL_GRXRINGS ioctl. > > > > > > - if (tb[ETHTOOL_A_CHANNELS_RX_COUNT]) > > - args->num_rings += > > mnl_attr_get_u32(tb[ETHTOOL_A_CHANNELS_RX_COUNT]); > > - return MNL_CB_OK; > > + ret = ioctl_init(ctx, false); > > + if (ret) > > + return ret; > > + > > + ret = send_ioctl(ctx, &ring_count); > > + if (ret) { > > + perror("Cannot get RX ring count"); > > + return ret; > > + } > > + > > + args->num_rings = (u32)ring_count.data; > > + > > + return 0; > > } > > > > Hi Vladimir, my understanding is ioctls are not used in ethtool netlink > path. Can we use ETHTOOL_MSG_RINGS_GET (tb[ETHTOOL_A_RINGS_RX] member) > instead ? > > Couldn't work on this since I was on sabbatical till this week. I see Jakub ack'ed this. Please ignore my comment above.
Hi Sudheer, On Fri, Sep 13, 2024 at 10:43:19PM +0000, Mogilappagari, Sudheer wrote: > Hi Vladimir, my understanding is ioctls are not used > in ethtool netlink path. Can we use ETHTOOL_MSG_RINGS_GET > (tb[ETHTOOL_A_RINGS_RX] member) instead ? You mean this? ``ETHTOOL_A_RINGS_RX`` u32 size of RX ring
> -----Original Message----- > From: Vladimir Oltean <vladimir.oltean@nxp.com> > Sent: Friday, September 13, 2024 3:49 PM > To: Mogilappagari, Sudheer <sudheer.mogilappagari@intel.com> > Cc: netdev@vger.kernel.org; Michal Kubecek <mkubecek@suse.cz>; Jakub > Kicinski <kuba@kernel.org> > Subject: Re: [PATCH ethtool] netlink: rss: retrieve ring count using > ETHTOOL_GRXRINGS ioctl > > Hi Sudheer, > > On Fri, Sep 13, 2024 at 10:43:19PM +0000, Mogilappagari, Sudheer wrote: > > Hi Vladimir, my understanding is ioctls are not used in ethtool > > netlink path. Can we use ETHTOOL_MSG_RINGS_GET > (tb[ETHTOOL_A_RINGS_RX] > > member) instead ? > > You mean this? > > ``ETHTOOL_A_RINGS_RX`` u32 size of RX ring Yes. I had meant ETHTOOL_A_RINGS_RX but I see it is ring size and not ring count. There seems to be no netlink message that fetches ringcount info.
On Fri, Sep 13, 2024 at 12:38:28PM +0300, Vladimir Oltean wrote: > Several drivers regressed when ethtool --show-rxfh was converted from > ioctl to netlink. This is because ETHTOOL_GRXRINGS was converted to > ETHTOOL_MSG_CHANNELS_GET, which is semantically equivalent to > ETHTOOL_GCHANNELS but different from ETHTOOL_GRXRINGS. Drivers which > implement ETHTOOL_GRXRINGS do not necessarily implement ETHTOOL_GCHANNELS > or its netlink equivalent. > > According to the man page, "A channel is an IRQ and the set of queues > that can trigger that IRQ.", which is different from the definition of > a queue/ring. So we shouldn't be attempting to query the # of rings for > the ioctl variant, but the # of channels for the netlink variant anyway. I have asked this multiple times but I never got a direct answer: is this only a formal terminology problem or is there an actual difference between the two? In particular: is someone aware of an example of a driver and device where these two counts differ? Or is there a reason to expect such device either exists or will exist in the future? (Actually, I seem to remember that the first time I asked, the general consensus was that combined + rx is indeed the value we need here - which is what current code does. But it has been few years so I would have to look it up to be sure.) If there is no real difference, it would be really unfortunate to have the same count presented under two different names via two different interfaces (->get_rxnfc() and ->get_channels() in ethtool_ops) with most drivers providing both but some only one and some only the other. Which seems to be the current state and the long term solution should be cleaning this up. If there is an actual difference, the long term solution would be providing the necessary information as part of the reply to ETHTOOL_MSG_RSS_GET requests - and the sooner we add it, the better. Short term, I'm afraid that however ugly this hack is, it's either it or disabling the netlink handler until we can get the information reliably from kernel. Michal
On Mon, 16 Sep 2024 21:38:14 +0200 Michal Kubecek wrote: > I have asked this multiple times but I never got a direct answer: is > this only a formal terminology problem or is there an actual difference > between the two? In particular: is someone aware of an example of > a driver and device where these two counts differ? Or is there a reason > to expect such device either exists or will exist in the future? > > (Actually, I seem to remember that the first time I asked, the general > consensus was that combined + rx is indeed the value we need here - > which is what current code does. But it has been few years so I would > have to look it up to be sure.) The change in perspective comes from the potential for dynamic queue allocation in the future. We don't have the exact details but it's possible that we'd choose to treat "channels" as NAPI instances rather than queues. And more queues can be dynamically opened by the applications and attached to the existing channels.
diff --git a/ethtool.c b/ethtool.c index 98690df05992..89c0edefe8eb 100644 --- a/ethtool.c +++ b/ethtool.c @@ -6369,7 +6369,7 @@ static int do_perqueue(struct cmd_context *ctx) return 0; } -static int ioctl_init(struct cmd_context *ctx, bool no_dev) +int ioctl_init(struct cmd_context *ctx, bool no_dev) { if (no_dev) { ctx->fd = -1; diff --git a/internal.h b/internal.h index 3923719c39d5..1bd5515fc9f7 100644 --- a/internal.h +++ b/internal.h @@ -293,6 +293,7 @@ int test_fclose(FILE *fh); #endif #endif +int ioctl_init(struct cmd_context *ctx, bool no_dev); int send_ioctl(struct cmd_context *ctx, void *cmd); void dump_hex(FILE *f, const u8 *data, int len, int offset); diff --git a/netlink/rss.c b/netlink/rss.c index 4ad6065ef698..d0045b1f0f8f 100644 --- a/netlink/rss.c +++ b/netlink/rss.c @@ -54,29 +54,29 @@ void dump_json_rss_info(struct cmd_context *ctx, u32 *indir_table, close_json_object(); } -int get_channels_cb(const struct nlmsghdr *nlhdr, void *data) +/* There is no netlink equivalent for ETHTOOL_GRXRINGS. */ +static int get_num_rings(struct cb_args *args) { - const struct nlattr *tb[ETHTOOL_A_CHANNELS_MAX + 1] = {}; - DECLARE_ATTR_TB_INFO(tb); - struct cb_args *args = data; struct nl_context *nlctx = args->nlctx; - bool silent; - int err_ret; + struct cmd_context *ctx = nlctx->ctx; + struct ethtool_rxnfc ring_count = { + .cmd = ETHTOOL_GRXRINGS, + }; int ret; - silent = nlctx->is_dump || nlctx->is_monitor; - err_ret = silent ? MNL_CB_OK : MNL_CB_ERROR; - ret = mnl_attr_parse(nlhdr, GENL_HDRLEN, attr_cb, &tb_info); - if (ret < 0) - return err_ret; - nlctx->devname = get_dev_name(tb[ETHTOOL_A_CHANNELS_HEADER]); - if (!dev_ok(nlctx)) - return err_ret; - if (tb[ETHTOOL_A_CHANNELS_COMBINED_COUNT]) - args->num_rings = mnl_attr_get_u32(tb[ETHTOOL_A_CHANNELS_COMBINED_COUNT]); - if (tb[ETHTOOL_A_CHANNELS_RX_COUNT]) - args->num_rings += mnl_attr_get_u32(tb[ETHTOOL_A_CHANNELS_RX_COUNT]); - return MNL_CB_OK; + ret = ioctl_init(ctx, false); + if (ret) + return ret; + + ret = send_ioctl(ctx, &ring_count); + if (ret) { + perror("Cannot get RX ring count"); + return ret; + } + + args->num_rings = (u32)ring_count.data; + + return 0; } int rss_reply_cb(const struct nlmsghdr *nlhdr, void *data) @@ -131,22 +131,7 @@ int rss_reply_cb(const struct nlmsghdr *nlhdr, void *data) if (ret < 0) return silent ? MNL_CB_OK : MNL_CB_ERROR; - nlctx->devname = get_dev_name(tb[ETHTOOL_A_RSS_HEADER]); - if (!dev_ok(nlctx)) - return MNL_CB_OK; - - /* Fetch ring count info into args->num_rings */ - ret = nlsock_prep_get_request(nlctx->ethnl2_socket, - ETHTOOL_MSG_CHANNELS_GET, - ETHTOOL_A_CHANNELS_HEADER, 0); - if (ret < 0) - return MNL_CB_ERROR; - - ret = nlsock_sendmsg(nlctx->ethnl2_socket, NULL); - if (ret < 0) - return MNL_CB_ERROR; - - ret = nlsock_process_reply(nlctx->ethnl2_socket, get_channels_cb, args); + ret = get_num_rings(args); if (ret < 0) return MNL_CB_ERROR;
Several drivers regressed when ethtool --show-rxfh was converted from ioctl to netlink. This is because ETHTOOL_GRXRINGS was converted to ETHTOOL_MSG_CHANNELS_GET, which is semantically equivalent to ETHTOOL_GCHANNELS but different from ETHTOOL_GRXRINGS. Drivers which implement ETHTOOL_GRXRINGS do not necessarily implement ETHTOOL_GCHANNELS or its netlink equivalent. According to the man page, "A channel is an IRQ and the set of queues that can trigger that IRQ.", which is different from the definition of a queue/ring. So we shouldn't be attempting to query the # of rings for the ioctl variant, but the # of channels for the netlink variant anyway. Reimplement the args->num_rings retrieval as in do_grxfh(), aka using the ETHTOOL_GRXRINGS ioctl. Link: https://lore.kernel.org/netdev/20240711114535.pfrlbih3ehajnpvh@skbuf/ Fixes: ffab99c1f382 ("netlink: add netlink handler for get rss (-x)") Signed-off-by: Vladimir Oltean <vladimir.oltean@nxp.com> --- ethtool.c | 2 +- internal.h | 1 + netlink/rss.c | 55 +++++++++++++++++++-------------------------------- 3 files changed, 22 insertions(+), 36 deletions(-)