| Message ID | 20240913093828.2549217-1-vladimir.oltean@nxp.com (mailing list archive) |
|---|---|
| State | Accepted, archived |
| 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.
On 9/13/2024 4:07 PM, Mogilappagari, Sudheer wrote: > > >> -----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. > We could extend the netlink interface to add this over netlink instead of ioctl, but that hasn't been done yet.
Hello Jake, On Thu, Sep 19, 2024 at 10:58:48AM -0700, Jacob Keller wrote: > We could extend the netlink interface to add this over netlink instead > of ioctl, but that hasn't been done yet. Let me understand how this will play out. So you're proposing that we add a new netlink attribute in ETHTOOL_MSG_RSS_GET_REPLY, with Fixes: 7112a04664bf ("ethtool: add netlink based get rss support"), and this gets backported to all stable kernels which include this commit? And then also 'fix' ethtool to parse this new netlink attribute instead of ETHTOOL_A_CHANNELS_RX_COUNT? Thus introducing another breakage: when the new ethtool program runs with the old (aka unpatched stable) kernel?
On Thu, Oct 03, 2024 at 12:18:10PM +0300, Vladimir Oltean wrote: > Hello Jake, > > On Thu, Sep 19, 2024 at 10:58:48AM -0700, Jacob Keller wrote: > > We could extend the netlink interface to add this over netlink instead > > of ioctl, but that hasn't been done yet. > > Let me understand how this will play out. So you're proposing that we > add a new netlink attribute in ETHTOOL_MSG_RSS_GET_REPLY, with Fixes: > 7112a04664bf ("ethtool: add netlink based get rss support"), and this > gets backported to all stable kernels which include this commit? > > And then also 'fix' ethtool to parse this new netlink attribute instead > of ETHTOOL_A_CHANNELS_RX_COUNT? Thus introducing another breakage: when > the new ethtool program runs with the old (aka unpatched stable) kernel? I'm afraid we will have to keep the unfortunate ioctl fallback for quite long. The only other option would be to only use netlink for RSS against kernel which provides full information and use only ioctl against those which don't. Michal
On Thu, Oct 03, 2024 at 01:09:47PM +0200, Michal Kubecek wrote: > I'm afraid we will have to keep the unfortunate ioctl fallback for quite > long. The only other option would be to only use netlink for RSS against > kernel which provides full information and use only ioctl against those > which don't. > > Michal So, then, is there anything blocking this patch?
On Thu, Oct 03, 2024 at 04:49:16PM +0300, Vladimir Oltean wrote: > On Thu, Oct 03, 2024 at 01:09:47PM +0200, Michal Kubecek wrote: > > I'm afraid we will have to keep the unfortunate ioctl fallback for quite > > long. The only other option would be to only use netlink for RSS against > > kernel which provides full information and use only ioctl against those > > which don't. > > > > Michal > > So, then, is there anything blocking this patch? I'm still not fully convinced that this mix of netlink and ioctl is actually better than fully reverting to ioctl until we can get all information via netlink. Either way, I'm going to handle this before the end of this week so that ethtool 6.11 can be released. At the moment I'm in favor of your patch, however unhappy I'm about it. Michal
> -----Original Message----- > From: Michal Kubecek <mkubecek@suse.cz> > Sent: Thursday, October 3, 2024 4:20 PM > To: Vladimir Oltean <vladimir.oltean@nxp.com> > Cc: Keller, Jacob E <jacob.e.keller@intel.com>; Mogilappagari, Sudheer > <sudheer.mogilappagari@intel.com>; netdev@vger.kernel.org; Jakub Kicinski > <kuba@kernel.org> > Subject: Re: [PATCH ethtool] netlink: rss: retrieve ring count using > ETHTOOL_GRXRINGS ioctl > > On Thu, Oct 03, 2024 at 04:49:16PM +0300, Vladimir Oltean wrote: > > On Thu, Oct 03, 2024 at 01:09:47PM +0200, Michal Kubecek wrote: > > > I'm afraid we will have to keep the unfortunate ioctl fallback for quite > > > long. The only other option would be to only use netlink for RSS against > > > kernel which provides full information and use only ioctl against those > > > which don't. > > > > > > Michal > > > > So, then, is there anything blocking this patch? > > I'm still not fully convinced that this mix of netlink and ioctl is > actually better than fully reverting to ioctl until we can get all > information via netlink. > > Either way, I'm going to handle this before the end of this week so that > ethtool 6.11 can be released. At the moment I'm in favor of your patch, > however unhappy I'm about it. > > Michal I have no objection to your patch, I think its correct to do now. My suggestion was that we can improve the netlink interface for the future, and I believe we can make ethtool continue to use the existing ioctl interface on older kernels, but use the netlink interface once its available.
On Fri, Oct 04, 2024 at 07:19:24PM +0000, Keller, Jacob E wrote: > I have no objection to your patch, I think its correct to do now. My > suggestion was that we can improve the netlink interface for the > future, and I believe we can make ethtool continue to use the existing > ioctl interface on older kernels, but use the netlink interface once > its available. This is not the problem. It's what we have been doing since netlink interface was introduced and it's what we are going to be doing for quite long. What I'm unhappy about is the mix of netlink and ioctl where we use netlink request for RSS but also an ioctl request to get the ring count. I can't help wondering if it wouldn't make more sense to fallback to ioctl fully unless we can retrieve full information via netlink. Michal
On 10/5/2024 12:46 AM, Michal Kubecek wrote: > On Fri, Oct 04, 2024 at 07:19:24PM +0000, Keller, Jacob E wrote: >> I have no objection to your patch, I think its correct to do now. My >> suggestion was that we can improve the netlink interface for the >> future, and I believe we can make ethtool continue to use the existing >> ioctl interface on older kernels, but use the netlink interface once >> its available. > > This is not the problem. It's what we have been doing since netlink > interface was introduced and it's what we are going to be doing for > quite long. > > What I'm unhappy about is the mix of netlink and ioctl where we use > netlink request for RSS but also an ioctl request to get the ring count. > I can't help wondering if it wouldn't make more sense to fallback to > ioctl fully unless we can retrieve full information via netlink. > > Michal That seems reasonable to me
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(-)