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

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <CAMArcTXP0PWw0DEXzW5u7to8S9rUbcyc5dLOR_ZSE1H2VzBXFA@mail.gmail.com>
Date: Sat, 16 Nov 2024 02:12:18 +0900
From: Taehee Yoo <ap420073@...il.com>
To: Jakub Kicinski <kuba@...nel.org>
Cc: davem@...emloft.net, pabeni@...hat.com, edumazet@...gle.com, 
	almasrymina@...gle.com, donald.hunter@...il.com, corbet@....net, 
	michael.chan@...adcom.com, andrew+netdev@...n.ch, hawk@...nel.org, 
	ilias.apalodimas@...aro.org, ast@...nel.org, daniel@...earbox.net, 
	john.fastabend@...il.com, dw@...idwei.uk, sdf@...ichev.me, 
	asml.silence@...il.com, brett.creeley@....com, linux-doc@...r.kernel.org, 
	netdev@...r.kernel.org, kory.maincent@...tlin.com, 
	maxime.chevallier@...tlin.com, danieller@...dia.com, hengqi@...ux.alibaba.com, 
	ecree.xilinx@...il.com, przemyslaw.kitszel@...el.com, hkallweit1@...il.com, 
	ahmed.zaki@...el.com, rrameshbabu@...dia.com, idosch@...dia.com, 
	jiri@...nulli.us, bigeasy@...utronix.de, lorenzo@...nel.org, 
	jdamato@...tly.com, aleksander.lobakin@...el.com, kaiyuanz@...gle.com, 
	willemb@...gle.com, daniel.zahka@...il.com
Subject: Re: [PATCH net-next v5 3/7] bnxt_en: add support for tcp-data-split
 ethtool command

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