Message-ID: <IA1PR11MB626686775A30F79E8AE85905E4019@IA1PR11MB6266.namprd11.prod.outlook.com>
Date:   Thu, 10 Nov 2022 00:26:23 +0000
From:   "Mogilappagari, Sudheer" <sudheer.mogilappagari@...el.com>
To:     Jakub Kicinski <kuba@...nel.org>
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



> -----Original Message-----
> From: Jakub Kicinski <kuba@...nel.org>
> Sent: Monday, November 7, 2022 6:26 PM
> Subject: Re: [PATCH net-next v2] ethtool: add netlink based get rxfh
> support
> 
> On Fri,  4 Nov 2022 16:42:44 -0700 Sudheer Mogilappagari wrote:
> > Implement RXFH_GET request to get RSS table, hash key and hash
> > function of an interface. This is netlink equivalent implementation
> of
> > ETHTOOL_GRSSH ioctl request.
> 
> Motivation would be good to have, if any. Are you going to add new
> fields or is it simply to fill in the implementation gap we have in the
> netlink version?
> 

Will add more explanation here. Goal was to implement existing
functionality first and then extend by adding new context 
specific parameters.   

> > +RXFH_GET
> > +========
> > +
> > +Get RSS table, hash key and hash function info like
> ``ETHTOOL_GRSSH``
> > +ioctl request.
> 
> 
> 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 ? 

> > +  ``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.

> 
> > +  ``ETHTOOL_A_RXFH_RSS_CONFIG``        u32     Indir table and hkey
> bytes
> 
> These should really be separate attributes.
> 
> Do we even need the indir_size and key_size given every attribute has a
> length so user can just look at the length of the attrs to see the
> length?

We can split indir table and hkey in netlink implementation and sizes 
won't be needed anymore. Above format is based on ethtool_rxfh 
structure where indir table and hkey come together as last member of
structure. Will update it in next version.

> 
> > +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. 

> > +	if (ret < 0)
> > +		return ret;
> > +
> > +	ethnl_update_u32(&rxfh->rss_context,
> tb[ETHTOOL_A_RXFH_RSS_CONTEXT],
> > +			 &mod);
> 
> ethnl_update_u32() is for when you're updating the config.
> You can use plain netlink helpers to get request arguments.

Ack.

> > +	/* Some drivers don't handle rss_context */
> > +	if (rxfh->rss_context && !ops->get_rxfh_context) {
> > +		ret = -EOPNOTSUPP;
> > +		goto out_dev;
> > +	}
> > +
> > +	ret = ethnl_ops_begin(dev);
> > +	if (ret < 0)
> > +		goto out_dev;
> > +
> > +	if (ops->get_rxfh_indir_size)
> > +		indir_size = ops->get_rxfh_indir_size(dev);
> > +	if (ops->get_rxfh_key_size)
> > +		hkey_size = ops->get_rxfh_key_size(dev);
> > +
> > +	indir_bytes = indir_size * sizeof(rxfh->rss_config[0]);
> > +	total_size = indir_bytes + hkey_size;
> > +	rss_config = kzalloc(total_size, GFP_USER);
> 
> GFP_KERNEL is enough here
> 

Will fix in next version.

> > +	if (!rss_config) {
> > +		ret = -ENOMEM;
> > +		goto out_ops;
> > +	}
> > +
> > +	if (indir_size) {
> > +		data->rss_config = (u32 *)rss_config;
> > +		rxfh->indir_size = indir_size;
> > +	}
> > +
> > +	if (hkey_size) {
> > +		hkey = rss_config + indir_bytes;
> > +		rxfh->key_size = hkey_size;
> > +	}
> > +
> > +	if (rxfh->rss_context)
> > +		ret = ops->get_rxfh_context(dev, data->rss_config, hkey,
> > +					    &dev_hfunc, rxfh->rss_context);
> > +	else
> > +		ret = ops->get_rxfh(dev, data->rss_config, hkey,
> &dev_hfunc);
> 
> This will not be sufficient for dump, I'm afraid.
> 
> For dump we need to find a way to dump all contexts in all devices.
> Which may require extending the driver API. Maybe drop the dump
> implementation for now?
> 

Agree. Will remove dumpit for this command.

> > +	rxfh->hfunc = dev_hfunc;
> > +
> > +out_ops:
> > +	ethnl_ops_complete(dev);
> > +out_dev:
> > +	ethnl_parse_header_dev_put(&req_info);
> > +	return ret;
> > +}

Powered by blists - more mailing lists