Re: [PATCH net-next v5 3/7] bnxt_en: add support for tcp-data-split ethtool command

Taehee Yoo
2024-11-15T17:12:31Z

On Fri, Nov 15, 2024 at 1:15 PM Jakub Kicinski <kuba@kernel.org> wrote:
>

Hi Jakub,
Thank you so much for the review!

> On Wed, 13 Nov 2024 17:32:17 +0000 Taehee Yoo wrote:
> > NICs that uses bnxt_en driver supports tcp-data-split feature by the
> > name of HDS(header-data-split).
> > But there is no implementation for the HDS to enable by ethtool.
> > Only getting the current HDS status is implemented and The HDS is just
> > automatically enabled only when either LRO, HW-GRO, or JUMBO is enabled.
> > The hds_threshold follows rx-copybreak value. and it was unchangeable.
> >
> > This implements `ethtool -G <interface name> tcp-data-split <value>`
> > command option.
> > The value can be <on> and <auto>.
> > The value is <auto> and one of LRO/GRO/JUMBO is enabled, HDS is
> > automatically enabled and all LRO/GRO/JUMBO are disabled, HDS is
> > automatically disabled.
> >
> > HDS feature relies on the aggregation ring.
> > So, if HDS is enabled, the bnxt_en driver initializes the aggregation ring.
> > This is the reason why BNXT_FLAG_AGG_RINGS contains HDS condition.
>
> I may be missing some existing check but doesn't enabling XDP force
> page_mode which in turn clears BNXT_FLAG_AGG_RINGS, including HDS ?
> If user specifically requested HDS we should refuse to install XDP
> in non-multibuf mode.

Sorry, I missed adding this check.
I added a check to reject setting HDS when XDP is attached.
But, I didn't add a check to reject attaching XDP when HDS is enabled.

bnxt driver doesn't allow setting HDS if XDP is attached even if it's
multibuffer XDP. So, I will reject installing singlebuffer and
multibuffer XDP if HDS is enabled.

>
> TBH a selftest under tools/testing/drivers/net would go a long way
> to make it clear we caught all cases. You can add a dummy netdevsim
> implementation for testing without bnxt present (some of the existing
> python tests can work with real drivers and netdevsim):
> https://github.com/linux-netdev/nipa/wiki/Running-driver-tests

Thanks for that, I will try to use this selftest.
I will add a dummy HDS feature for testing on the netdevsim.

Thanks a lot!
Taehee Yoo