Re: [PATCH v5 net-next 4/7] net: ethtool: let the core choose RSS context IDs

[Edward Cree <ecree.xilinx@gmail.com>]

On 19/06/2024 18:24, Jakub Kicinski wrote:
> On Tue, 18 Jun 2024 23:44:24 +0100 edward.cree@amd.com wrote:
>> + * @create_rxfh_context: Create a new RSS context with the specified RX flow
>> + *	hash indirection table, hash key, and hash function.
>> + *	Parameters which are set to %NULL or zero will be populated to
>> + *	appropriate defaults by the driver.
> 
> The defaults will most likely "inherit" whatever is set in context 0.
> So the driver _may_ init the values according to its preferences
> but they will not be used by the core (specifically not reported to
> user space via ethtool netlink)
> 
> Does that match your thinking?

Yes, that was what I had in mind.

> Indirection table needs to get reported.

Okay, I'll alter the documentation to say so, that notwithstanding
 this bit...

>> + *	The &struct ethtool_rxfh_context for this context is passed in @ctx;
>> + *	note that the indir table, hkey and hfunc are not yet populated as
>> + *	of this call.  The driver does not need to update these; the core
>> + *	will do so if this op succeeds.

... at least indir MUST, and the others MAY, be filled in by the
 driver if they weren't specified in params.
(sfc does this already, because it uses the ctx as a place to store
 the new table and/or key if it has to generate them.)

>> +	int	(*remove_rxfh_context)(struct net_device *,
>> +				       struct ethtool_rxfh_context *ctx,
>> +				       u32 rss_context);
> 
> Can we make remove void? It's sort of a cleanup, cleanups which can
> fail make life hard.

At least on sfc it's possible for it to fail.  Apart from anything
 else, I don't think there's anything in the core to catch the case
 of trying to remove a context while there are still ntuple filters
 targeting it; I believe that gets all the way down to the firmware,
 which responds EBUSY or something.  If that happens, we don't want
 to pretend it worked and delete the context from the xarray.

-ed