[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]
Date: Wed, 9 Nov 2022 16:46:03 -0800
From: Jakub Kicinski <kuba@...nel.org>
To: "Mogilappagari, Sudheer" <sudheer.mogilappagari@...el.com>
Cc: "netdev@...r.kernel.org" <netdev@...r.kernel.org>,
"mkubecek@...e.cz" <mkubecek@...e.cz>,
"andrew@...n.ch" <andrew@...n.ch>,
"corbet@....net" <corbet@....net>,
"Samudrala, Sridhar" <sridhar.samudrala@...el.com>,
"Nguyen, Anthony L" <anthony.l.nguyen@...el.com>
Subject: Re: [PATCH net-next v2] ethtool: add netlink based get rxfh support
On Thu, 10 Nov 2022 00:26:23 +0000 Mogilappagari, Sudheer wrote:
> > Can we describe in more detail which commands are reimplemented?
> > Otherwise calling the command RXFH makes little sense.
> > We may be better of using RSS in the name in the first place.
>
> This is the ethtool command being reimplemented.
> ethtool -x|--show-rxfh-indir DEVNAME Show Rx flow hash indirection table and/or RSS hash key
> [ context %d ]
>
> Picked RXFH based on existing function names and ethtool_rxfh
> structure. If it needs to change, how about RSS_CTX or just RSS ?
I vote for just RSS.
> > > + ``ETHTOOL_A_RXFH_HEADER`` nested reply header
> > > + ``ETHTOOL_A_RXFH_RSS_CONTEXT`` u32 RSS context number
> > > + ``ETHTOOL_A_RXFH_INDIR_SIZE`` u32 RSS Indirection table size
> > > + ``ETHTOOL_A_RXFH_KEY_SIZE`` u32 RSS hash key size
> > > + ``ETHTOOL_A_RXFH_HFUNC`` u32 RSS hash func
> >
> > This is u8 in the implementation, please make the implementation u32 as
> > documented.
>
> This should have been u8 instead. Will make it consistent.
u32 is better, data sizes in netlink are rounded up to 4 bytes anyway,
so u8 is 1 usable byte and 3 bytes of padding. Much better to use u32.
> > > +static int rxfh_prepare_data(const struct ethnl_req_info *req_base,
> > > + struct ethnl_reply_data *reply_base,
> > > + struct genl_info *info)
> > > +{
> > > + struct rxfh_reply_data *data = RXFH_REPDATA(reply_base);
> > > + struct net_device *dev = reply_base->dev;
> > > + struct ethtool_rxfh *rxfh = &data->rxfh;
> > > + struct ethnl_req_info req_info = {};
> > > + struct nlattr **tb = info->attrs;
> > > + u32 indir_size = 0, hkey_size = 0;
> > > + const struct ethtool_ops *ops;
> > > + u32 total_size, indir_bytes;
> > > + bool mod = false;
> > > + u8 dev_hfunc = 0;
> > > + u8 *hkey = NULL;
> > > + u8 *rss_config;
> > > + int ret;
> > > +
> > > + ops = dev->ethtool_ops;
> > > + if (!ops->get_rxfh)
> > > + return -EOPNOTSUPP;
> > > +
> > > + ret = ethnl_parse_header_dev_get(&req_info,
> > > + tb[ETHTOOL_A_RXFH_HEADER],
> > > + genl_info_net(info), info->extack,
> > > + true);
> >
> > Why are you parsing again?
> >
> > You hook in ethnl_default_doit() and ethnl_default_dumpit(), which
> > should call the parsing for you already.
>
> My bad. Had used other netlink get command implementation as reference
> and overlooked request_ops->parse_request.
Right, as you probably discovered the core ethtool code can do a lot of
work for you if the command doesn't have special requirements and you
can rely on the default doit / dumpit handling.
Powered by blists - more mailing lists