* Re: [ANN] netdev call - Mar 5th
2024-03-04 18:36 ` Jakub Kicinski
@ 2024-03-11 19:27 ` Larysa Zaremba
2024-03-11 19:50 ` [ANN] netdev call - Mar 12th (note time change) Jakub Kicinski
0 siblings, 1 reply; 4+ messages in thread
From: Larysa Zaremba @ 2024-03-11 19:27 UTC (permalink / raw)
To: Jakub Kicinski; +Cc: netdev, netdev-driver-reviewers, Maciej Fijalkowski
On Mon, Mar 04, 2024 at 10:36:57AM -0800, Jakub Kicinski wrote:
> On Mon, 4 Mar 2024 10:35:38 -0800 Jakub Kicinski wrote:
> > Hi,
> >
> > The bi-weekly netdev call at https://bbb.lwn.net/b/jak-wkr-seg-hjn
> > is scheduled tomorrow at 8:30 am (PT) / 5:30 pm (~EU).
> >
> > No topics so far, please reach out if you have something to discuss,
> > otherwise we'll cancel.
> >
> > The reviewer of the week is.. Meta!
>
> Sorry, ignore that, ENOCOFFEE.
> The next call is scheduled for the 12th of March, not tomorrow 🤦️
>
We have a topic for the 12.03.2024 netdev call:
How set_channels() input should be interpreted
Mostly, implemetations of .set_channels() are tailored for only one of the
following inputs:
1. ethtool -L <ifname> combined <n>
2. ethtool -L <ifname> rx <n> tx <m>
There are also exceptions, e.g.
* bnxt: input 1 puts interface in a shared channels mode, input 2 puts device
into a dedicated channels mode
* xgbe: ethtool -L <ifname> combined <n> rx <m> tx <l>, <m> and <l> cannot both
be non-zero at the same time
* ice and idpf: accept both inputs, but try to create as many queue pairs as
possible
In idpf and ice, we have encountered a problem, where the checks (listed below)
in ethnl_set_channels() do not trigger for us, because our set_channels() operates
under an assumption that if a value in ethtool_channels stays the same, it was
not provided by user and therefore defaults to '0' [0].
The ethnl_set_channels() checks and how they fail us:
1. Ensure there is at least 1 RX and 1 TX channel [1]
ethtool -l <ifname> -> combined 40
ethtool -L <ifname> combined 40
ice triggers an internal check, which was removed from idpf after review [2],
so idpf tries to set 0 RX and
0 TX queues
2. Prevent disabling a channel with an AF_XDP socket attached [3]
ethtool -l <ifname> -> combined 40
./xdpsock -i <ifname> -q35
ethtool -L <ifname> rx 30 tx 30
ice destroys the queue the socket is attached to, despite there being a check
in the core code that must prevent this [3]
In theory, the same should be possible to do the same on e.g. mlx5, by doing
ethtool -L <ifname> combined 30 rx 10 tx 10
So, we would like to discuss, how can we solve the above problem and also
establish how should an ideal set_channels() input interpretation look.
Possible solutions:
1. Convert our drivers to whatever is a preferred way to handle the input.
2. Add .fix_channels() callback to ethtool_ops
[...]
mod |= mod_combined;
if (!mod)
return 0;
/* Here ice and idpf would change unmodified values to 0 and convert
* separate overlapping rx/tx channels into combined,
* mlx5 and bnxt would set rx_count and tx_count to 0, etc.
*/
if (dev->ethtool_ops->fix_num_channels)
dev->ethtool_ops->fix_num_channels(dev, &channels);
/* ethtool does generic checks on the modified values and calls
* set_channels() after
*/
[...]
3. Use the abovementioned assumption made by ice driver [0] in ethnl_set_channels().
We propose this, because no doubt about it was expressed during the community
review of the both affected drivers [2][4].
[0] https://elixir.bootlin.com/linux/v6.8-rc7/source/drivers/net/ethernet/intel/ice/ice_ethtool.c#L3541
[1] https://elixir.bootlin.com/linux/v6.8-rc7/source/net/ethtool/channels.c#L154
[2] https://lore.kernel.org/netdev/20230821140205.4d3bc797@kernel.org/
[3] https://elixir.bootlin.com/linux/v6.8-rc7/source/net/ethtool/channels.c#L186
[4] https://lore.kernel.org/netdev/20191121144246.04adde1a@cakuba.netronome.com/
^ permalink raw reply [flat|nested] 4+ messages in thread