Re: [PATCH net-next v2] ethtool: add netlink based get rxfh support

[Jakub Kicinski <kuba@kernel.org>]

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.