| Message ID | 20240906080750.1068983-1-ap420073@gmail.com (mailing list archive) |
|---|---|
| Headers | show |
| Series | bnxt_en: implement tcp-data-split ethtool command | expand |
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. 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..
On Sat, Sep 7, 2024 at 10:38 AM Jakub Kicinski <kuba@kernel.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
NICs that use the bnxt_en driver support tcp-data-split feature named HDS(header-data-split). But there is no implementation for the HDS to enable/disable 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 the rx-copybreak value but it wasn't changeable. To implement, it requires decoupling rx-copybreak and hds_threshold value. The first patch implements .{set, get}_tunable() in the bnxt_en. The bnxt_en driver has been supporting the rx-copybreak feature but is not configurable, Only the default rx-copybreak value has been working. So, it changes the bnxt_en driver to be able to configure the rx-copybreak value. The second patch adds an implementation of tcp-data-split ethtool command. The HDS relies on the Aggregation ring, which is automatically enabled when either LRO, GRO, or large mtu is configured. So, if the Aggregation ring is enabled, HDS is automatically enabled by it. 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. Taehee Yoo (2): bnxt_en: add support for rx-copybreak ethtool command bnxt_en: add support for tcp-data-split ethtool command drivers/net/ethernet/broadcom/bnxt/bnxt.c | 30 ++++---- drivers/net/ethernet/broadcom/bnxt/bnxt.h | 10 ++- .../net/ethernet/broadcom/bnxt/bnxt_ethtool.c | 70 ++++++++++++++++++- 3 files changed, 91 insertions(+), 19 deletions(-)