On 11/25/2020 11:27 AM, Eric Dumazet wrote:
On Wed, Nov 25, 2020 at 10:06 AM Tariq Toukan <ttoukan.li...@gmail.com> wrote:
On 11/25/2020 5:25 AM, Herbert Xu wrote:
On Tue, Nov 24, 2020 at 11:48:35AM +0100, Eric Dumazet wrote:
Well, the 'increment' part was suggesting the function was adding
flags, not removing them.
The idea of the increment part is that we're adding a constituent
device, not that we're adding features. There have always been
features which were conjunctions, i.e., they must be supported by
all underlying devices for them to be enabled on the virtual device.
Your use of the increment function is unusual, as you're not adding
features that belong to one underlying device, but rather you're
trying to enable a feature on the virtual device unconditionally.
This was not the intent.
We can still disable TSO on the bonding device if desired.
pk51:~# for i in bond0 eth1 eth2; do ethtool -k $i|grep
tcp-segmentation-offload; done
tcp-segmentation-offload: on
tcp-segmentation-offload: on
tcp-segmentation-offload: on
lpk51:~# ethtool -K bond0 tso off
Actual changes:
tcp-segmentation-offload: off
tx-tcp-segmentation: off
tx-tcp-ecn-segmentation: off
tx-tcp-mangleid-segmentation: off
tx-tcp6-segmentation: off
large-receive-offload: off [requested on]
lpk51:~# for i in bond0 eth1 eth2; do ethtool -k $i|grep
tcp-segmentation-offload; done
tcp-segmentation-offload: off
tcp-segmentation-offload: on
tcp-segmentation-offload: on
The intent was that we could have :
lpk51:~# ethtool -K bond0 tso on
Actual changes:
tcp-segmentation-offload: on
tx-tcp-segmentation: on
tx-tcp-ecn-segmentation: on
tx-tcp-mangleid-segmentation: on
tx-tcp6-segmentation: on
lpk51:~# ethtool -K eth1 tso off
lpk51:~# ethtool -K eth2 tso off
lpk51:~# for i in bond0 eth1 eth2; do ethtool -k $i|grep
tcp-segmentation-offload; done
tcp-segmentation-offload: on
tcp-segmentation-offload: off
tcp-segmentation-offload: off
lpk51:~#
IIUC, we want to let the bond TSO feature bit be totally independent,
not affected by slaves.
If so, I think that:
First we should take NETIF_F_GSO_SOFTWARE (or just NETIF_F_ALL_TSO) out
of NETIF_F_ONE_FOR_ALL.
Then, make sure it is set in bond_setup (it is already done, as part of
BOND_VLAN_FEATURES).
I think this new logic is good for all other upper devices, which will
be affected by the change in NETIF_F_ONE_FOR_ALL.
We might ask Herbert Xu if we :
1) Need to comment the function, or change its name to be more descriptive.
2) Change the behavior (as you suggested)
3) Other choice.
I think Tariq's patch is fine, although a comment should be added
to netdev_add_tso_features as this use of the increment function
is nonstandard.
Thanks Herbert, I'll add a comment and re-spin.
I think we should remove the use of netdev_increment_features() and
replace it with something else,
because there is too much confusion.
I think it would be best.
I can prepare the patch I described above if you agree with it.
