Re: [PATCHv2 net-next 5/5] selftests/net: add offload checking test for virtual interface

2025-09-08, 10:14:57 +0000, Hangbin Liu wrote:
> On Mon, Sep 08, 2025 at 11:36:21AM +0200, Sabrina Dubroca wrote:
> > > The esp-hw-offload is fixed on netdevsim
> > > 
> > > # ethtool -k eni0np1 | grep -i esp-hw-offload
> > > esp-hw-offload: on [fixed]
> > > 
> > > There is no way to disable it.
> > 
> > I don't think this is intentional. nsim_ipsec_init only adds
> > NSIM_ESP_FEATURES to ->features but not to ->hw_features, but I think
> > it was just forgotten. I added a few in 494bd83bb519 ("netdevsim: add
> > more hw_features"), extending nsim_ipsec_init (and nsim_macsec_init
> > since I made the same mistake) to also add features to ->hw_features
> > would make sense to me.
> 
> This could be done in another patch.

If it's not needed for this series, sure.


> > > After we add the netdevsim to bond,
> > > the bond also shows "esp-hw-offload off" as the flag is inherit
> > > in dev->hw_enc_features, not dev->features.
> > 
> > Did you mean dev->hw_features?
> 
> No, the xfrm_features in patch 01 updates dev->hw_enc_features, not
> dev->hw_features.

Ok. But hw_enc_features is not the reason ethtool shows
"esp-hw-offload off". This line is:

	bond_dev->hw_features |= BOND_XFRM_FEATURES;

(from bond_setup)

> Do you think if we should update dev->hw_features in the
> patch?

For dev->hw_features (and dev->features) maybe not, since that depends
on the upper device's features and implementation. I'm not sure we can
have a common function without changing the behavior on at least one
type of device.

But maybe ndo_fix_features could use a common
netdev_fix_features_from_lowers? bond/team/bridge have very similar
implementations.

> > > It looks the only way to check if bond dev->hw_enc_features has NETIF_F_HW_ESP
> > > is try set xfrm offload. As
> > 
> > Was this test meant to check hw_enc_features?
> > 
> > To check hw_enc_features, I think the only way would be sending GSO
> > packets, since it's only used in those situations.
> 
> Oh.. That would make the test complex. Can we ignore this test first?

Ok for me.

> BTW, I'm a bit lost in the callbacks.gso_segment. e.g.
> 
> esp4_gso_segment
>  - xfrm4_outer_mode_gso_segment
>    - xfrm4_transport_gso_segment
>      - ops->callbacks.gso_segment
> 
> But who calls esp4_gso_segment? I can't find where the features is assigned.

inet_gso_segment via inet_offloads[] (ESP is a L4 proto like UDP etc).

-- 
Sabrina