| Message ID | 20240618215313.29631-3-michael.chan@broadcom.com (mailing list archive) |
|---|---|
| State | Accepted |
| Commit | b7bfcb4c7ce44fd0070ce8bccbc91c56341f05c1 |
| Delegated to: | Netdev Maintainers |
| Headers | show |
| Series | bnxt_en: Bug fixes for net | expand |
On Tue, 18 Jun 2024 14:53:12 -0700 Michael Chan wrote: > Firmware will now advertise a non-zero TSO max segments if the > device has a limit. 0 means no limit. The latest 5760X chip > (early revs) has a limit of 2047 that cannot be exceeded. If > exceeded, the chip will send out just a small number of segments. If we're only going to see 0 or 2047 pulling in the FW interface update and depending on newer FW version isn't a great way to fix this, IMHO. TCP has min MSS of 500+ bytes, so 2k segments gives us 1MB LSO at min legitimate segment size, right? So this is really just a protection against bugs in the TCP stack, letting MSS slide below 100. For a fix I'd just hardcode this to 2047 or even just 1k, and pull in the new FW interface to make it configurable in net-next.
On Wed, Jun 19, 2024 at 5:13 PM Jakub Kicinski <kuba@kernel.org> wrote: > > On Tue, 18 Jun 2024 14:53:12 -0700 Michael Chan wrote: > > Firmware will now advertise a non-zero TSO max segments if the > > device has a limit. 0 means no limit. The latest 5760X chip > > (early revs) has a limit of 2047 that cannot be exceeded. If > > exceeded, the chip will send out just a small number of segments. > > If we're only going to see 0 or 2047 pulling in the FW interface update > and depending on newer FW version isn't a great way to fix this, IMHO. > This issue only affects the newest chip which just started production and the production FW has this updated interface. > TCP has min MSS of 500+ bytes, so 2k segments gives us 1MB LSO at min > legitimate segment size, right? So this is really just a protection > against bugs in the TCP stack, letting MSS slide below 100. That's true for Big TCP, but what about UDP GSO? Can UDP GSO exceed 2k segments? > > For a fix I'd just hardcode this to 2047 or even just 1k, and pull in > the new FW interface to make it configurable in net-next. > -- > pw-bot: cr
On Thu, Jun 20, 2024 at 2:13 AM Jakub Kicinski <kuba@kernel.org> wrote: > > On Tue, 18 Jun 2024 14:53:12 -0700 Michael Chan wrote: > > Firmware will now advertise a non-zero TSO max segments if the > > device has a limit. 0 means no limit. The latest 5760X chip > > (early revs) has a limit of 2047 that cannot be exceeded. If > > exceeded, the chip will send out just a small number of segments. > > If we're only going to see 0 or 2047 pulling in the FW interface update > and depending on newer FW version isn't a great way to fix this, IMHO. > > TCP has min MSS of 500+ bytes, so 2k segments gives us 1MB LSO at min > legitimate segment size, right? So this is really just a protection > against bugs in the TCP stack, letting MSS slide below 100. > TCP default MSS is 536 (if no MSS attribute is present in SYN or SYNACK packets) But the minimal payload size per segment is 8 #define TCP_MIN_SND_MSS 48 #define TCP_MIN_GSO_SIZE (TCP_MIN_SND_MSS - MAX_TCP_OPTION_SPACE) > For a fix I'd just hardcode this to 2047 or even just 1k, and pull in > the new FW interface to make it configurable in net-next. > -- > pw-bot: cr
On Wed, 19 Jun 2024 23:50:42 -0700 Michael Chan wrote: > > If we're only going to see 0 or 2047 pulling in the FW interface update > > and depending on newer FW version isn't a great way to fix this, IMHO. > > This issue only affects the newest chip which just started production > and the production FW has this updated interface. Hm, okay, I see this only needs to go back to 6.8. > > TCP has min MSS of 500+ bytes, so 2k segments gives us 1MB LSO at min > > legitimate segment size, right? So this is really just a protection > > against bugs in the TCP stack, letting MSS slide below 100. > > That's true for Big TCP, but what about UDP GSO? Can UDP GSO exceed > 2k segments? I really don't think 2k segment limit is of any practical concern. What I have seen for UDP GSO, since its used mostly for QUIC and mostly on edge / in CDN - the speeds are much lower, so the QUIC stacks only use sends of 16k or so. Anyway, the "this is only needed in 6.8" convinced me, I'll apply.
diff --git a/drivers/net/ethernet/broadcom/bnxt/bnxt.c b/drivers/net/ethernet/broadcom/bnxt/bnxt.c index c437ca1c0fd3..89d29d6d7517 100644 --- a/drivers/net/ethernet/broadcom/bnxt/bnxt.c +++ b/drivers/net/ethernet/broadcom/bnxt/bnxt.c @@ -8996,6 +8996,7 @@ static int __bnxt_hwrm_func_qcaps(struct bnxt *bp) memcpy(vf->mac_addr, resp->mac_address, ETH_ALEN); #endif } + bp->tso_max_segs = le16_to_cpu(resp->max_tso_segs); hwrm_func_qcaps_exit: hwrm_req_drop(bp, req); @@ -15363,6 +15364,8 @@ static int bnxt_init_one(struct pci_dev *pdev, const struct pci_device_id *ent) dev->priv_flags |= IFF_UNICAST_FLT; netif_set_tso_max_size(dev, GSO_MAX_SIZE); + if (bp->tso_max_segs) + netif_set_tso_max_segs(dev, bp->tso_max_segs); dev->xdp_features = NETDEV_XDP_ACT_BASIC | NETDEV_XDP_ACT_REDIRECT | NETDEV_XDP_ACT_RX_SG; diff --git a/drivers/net/ethernet/broadcom/bnxt/bnxt.h b/drivers/net/ethernet/broadcom/bnxt/bnxt.h index bbc7edccd5a4..9cf0acfa04e5 100644 --- a/drivers/net/ethernet/broadcom/bnxt/bnxt.h +++ b/drivers/net/ethernet/broadcom/bnxt/bnxt.h @@ -2318,6 +2318,7 @@ struct bnxt { u8 rss_hash_key_updated:1; u16 max_mtu; + u16 tso_max_segs; u8 max_tc; u8 max_lltc; /* lossless TCs */ struct bnxt_queue_info q_info[BNXT_MAX_QUEUE];
Firmware will now advertise a non-zero TSO max segments if the device has a limit. 0 means no limit. The latest 5760X chip (early revs) has a limit of 2047 that cannot be exceeded. If exceeded, the chip will send out just a small number of segments. Call netif_set_tso_max_segs() if the device has a limit. Fixes: 2012a6abc876 ("bnxt_en: Add 5760X (P7) PCI IDs") Reviewed-by: Ajit Khaparde <ajit.khaparde@broadcom.com> Reviewed-by: Somnath Kotur <somnath.kotur@broadcom.com> Signed-off-by: Michael Chan <michael.chan@broadcom.com> --- drivers/net/ethernet/broadcom/bnxt/bnxt.c | 3 +++ drivers/net/ethernet/broadcom/bnxt/bnxt.h | 1 + 2 files changed, 4 insertions(+)