netdev - Re: [PATCH net-next 0/2] bnxt_en: implement 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 for Android: free password hash cracker in your pocket

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

Message-ID: <CAMArcTUiRJHj+u3DMjf+SGXgk57z-uDmXNycsfXku4=MKrngVA@mail.gmail.com>
Date: Sun, 8 Sep 2024 02:06:38 +0900
From: Taehee Yoo <ap420073@...il.com>
To: Jakub Kicinski <kuba@...nel.org>
Cc: davem@...emloft.net, pabeni@...hat.com, edumazet@...gle.com, 
	michael.chan@...adcom.com, netdev@...r.kernel.org
Subject: Re: [PATCH net-next 0/2] bnxt_en: implement tcp-data-split ethtool command

On Sat, Sep 7, 2024 at 10:38 AM Jakub Kicinski <kuba@...nel.org> wrote:
>

Hi Jakub,
Thanks a lot for your review!

> On Fri,  6 Sep 2024 08:07:48 +0000 Taehee Yoo wrote:
> > The approach of this patch is to support the bnxt_en driver setting up
> > enable/disable HDS explicitly, not rely on LRO/GRO, JUMBO.
> > In addition, hds_threshold no longer follows rx-copybreak.
> > By this patch, hds_threshold always be 0.
>
> That may make sense for zero-copy use cases, where you want to make
> sure that all of the data lands in target page pool. But in general
> using the data buffers  may waste quite a bit of memory, and PCIe bus
> bandwidth (two small transfers instead of one medium size).
>
> I think we should add a user-controlled setting in ethtool -g for
> hds-threshold.

Thanks, I understand your concern.
So, I will implement the tcp-data-split-threshold option in the v2 patch.

>
> Also please make sure you describe the level of testing you have done
> in the commit message. I remember discussing this a few years back
> and at that time HDS was tied to GRO for bnxt at the FW level.
> A lot has changed since but please describe what you tested..

Sorry about the lack of describing how I tested this patch.

I'm using BCM57412 and the latest firmware(230.0.157.0/pkg 230.1.116.0).
I tested if the HDS had any dependencies such as TPA (HW-GRO, LRO) or jumbo.
When I tested it I checked out skb->data size and skb->data_len size.
And HDS and TPA were worked independently.

HDS disabled + TPA disabled:
It receives normal packets.
`ethtool -S <interface name> | grep tpa` doesn't show an increment of
tpa statistics.

HDS disabled + TPA enabled:
It receives gro packets.
`ethtool -S <interface name> | grep tpa` shows an increment of tpa statistics.

HDS enabled + TPA disabled:
It receives header-data split packets.
`ethtool -S <interface name> | grep tpa` doesn't show an increment of
tpa statistics.

HDS enabled + TPA enabled:
It receives header-data split gro packets.
`ethtool -S <interface name> | grep tpa` shows an increment of tpa statistics.

I tested the above cases and they worked expectedly.
But I tested again after your review, I found a bug that sometimes
couldn't reset jumbo_thresh properly.
I will fix that bug too in the v2 patch.

So, the v2 patch will contain the following.
1. fix jumbo_thresh reset logic.
2. add a description of how I tested this patch.
3. implement `ethtool -G <interface name> tcp-data-split-threshold
<value> option.

If you think the above description is still not enough, please let me know!

Thanks a lot!
Taehee Yoo