netdev - Re: [PATCH net-next v6 1/7] net: ethtool: pass ethtool_rxfh to get/set

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <20240604103359.7b7be651@kernel.org>
Date: Tue, 4 Jun 2024 10:33:59 -0700
From: Jakub Kicinski <kuba@...nel.org>
To: Edward Cree <ecree.xilinx@...il.com>
Cc: Ahmed Zaki <ahmed.zaki@...el.com>, netdev@...r.kernel.org
Subject: Re: [PATCH net-next v6 1/7] net: ethtool: pass ethtool_rxfh to
 get/set_rxfh ethtool ops

On Tue, 4 Jun 2024 15:58:02 +0100 Edward Cree wrote:
> > Can we avoid the confusion by careful wording of the related kdoc?
> > "context" is the current state, while "params" describe the intended
> > configuration. If we move the "no_change" bits over to "params", 
> > I hope it wouldn't be all that confusing.  
> 
> I think "no_change" should stay in "context", but be renamed.
> ("params" has them implicitly via setting indir_size to
>  ETH_RXFH_INDIR_NO_CHANGE or key_size to zero.)
> The bits in "context" mean that indir or key has *never* been
>  configured for this context, and therefore the driver should
>  make up a default.  In that case, if the context has to be
>  recreated (e.g. after a device reset, or maybe an ethtool -L
>  changing the number of RXQs), the driver could generate a
>  different table.  (Also, unless the driver decides to write
>  the generated default table back into "context" by hand, the
>  core won't be able to show it to userspace in netlink dumps
>  when those get added.)

Ah, great point!

> So I guess context.indir_no_change should really be called
>  something like .indir_unspecified?

/me looks at the code
We already have IFF_RXFH_CONFIGURED, and corresponding
netif_is_rxfh_configured(). Should we stick to "${field}_configured"?

> Or should the core just insist on handling default generation
>  itself (but then it can't be sure of producing defaults that
>  a device with limited resources can honour), or have yet
>  another op to populate the defaults into params when the
>  user didn't specify them?

Thinking this over during breakfast I concluded we should leave out
feeding the defaults into drivers for now.

The only useful fields we could pre-populate are indir table and
key (useful because it'd save drivers calling some ethtool_default*
helpers). But both of those are fairly complex. Key may not be
populated for dynamically created contexts at all. Indir table
may have different sizes and has to be re-calculated when queue
count changes.