On Fri, Nov 15, 2024 at 1:15 PM Jakub Kicinski <kuba@xxxxxxxxxx> 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
