| Message ID | 20241218144530.2963326-4-ap420073@gmail.com (mailing list archive) |
|---|---|
| State | Changes Requested |
| Delegated to: | Netdev Maintainers |
| Headers | show |
| Series | bnxt_en: implement tcp-data-split and thresh option | expand |
On Wed, 18 Dec 2024 14:45:24 +0000 Taehee Yoo wrote: > + if (tcp_data_split == ETHTOOL_TCP_DATA_SPLIT_DISABLED && hds_config_mod) > + return -EOPNOTSUPP; I think ethtool ops generally return -EINVAL when param not supported. EOPNOTSUPP means entire op is not supported (again, that's just how ethtool ops generally work, not a kernel-wide rule). > + if (tcp_data_split == ETHTOOL_TCP_DATA_SPLIT_ENABLED && > + hds_config_mod && BNXT_RX_PAGE_MODE(bp)) { Looks like patch 4 adds this check in the core. I think adding the check in the core can be a separate patch. If you put it before this patch in the series this bnxt check can be removed? I mean this chunk in the core: + hds_config_mod = old_hds_config != kernel_ringparam.tcp_data_split; + if (kernel_ringparam.tcp_data_split == ETHTOOL_TCP_DATA_SPLIT_ENABLED && + hds_config_mod && dev_xdp_sb_prog_count(dev)) { + NL_SET_ERR_MSG(info->extack, + "tcp-data-split can not be enabled with single buffer XDP"); + return -EINVAL; + } It's currently in the hds-thresh patch but really it's unrelated to the threshold.. > + NL_SET_ERR_MSG_MOD(extack, "tcp-data-split is disallowed when XDP is attached"); > + return -EOPNOTSUPP; > + } > + > if (netif_running(dev)) > bnxt_close_nic(bp, false, false); > > + if (hds_config_mod) { > + if (tcp_data_split == ETHTOOL_TCP_DATA_SPLIT_ENABLED) > + bp->flags |= BNXT_FLAG_HDS; > + else if (tcp_data_split == ETHTOOL_TCP_DATA_SPLIT_UNKNOWN) > + bp->flags &= ~BNXT_FLAG_HDS; > + } > + > bp->rx_ring_size = ering->rx_pending; > bp->tx_ring_size = ering->tx_pending; > bnxt_set_ring_params(bp); > @@ -5354,6 +5374,7 @@ const struct ethtool_ops bnxt_ethtool_ops = { > ETHTOOL_COALESCE_STATS_BLOCK_USECS | > ETHTOOL_COALESCE_USE_ADAPTIVE_RX | > ETHTOOL_COALESCE_USE_CQE, > + .supported_ring_params = ETHTOOL_RING_USE_TCP_DATA_SPLIT, > .get_link_ksettings = bnxt_get_link_ksettings, > .set_link_ksettings = bnxt_set_link_ksettings, > .get_fec_stats = bnxt_get_fec_stats, > diff --git a/drivers/net/ethernet/broadcom/bnxt/bnxt_xdp.c b/drivers/net/ethernet/broadcom/bnxt/bnxt_xdp.c > index f88b641533fc..1bfff7f29310 100644 > --- a/drivers/net/ethernet/broadcom/bnxt/bnxt_xdp.c > +++ b/drivers/net/ethernet/broadcom/bnxt/bnxt_xdp.c > @@ -395,6 +395,10 @@ static int bnxt_xdp_set(struct bnxt *bp, struct bpf_prog *prog) > bp->dev->mtu, BNXT_MAX_PAGE_MODE_MTU); > return -EOPNOTSUPP; > } > + if (prog && bp->flags & BNXT_FLAG_HDS) { > + netdev_warn(dev, "XDP is disallowed when HDS is enabled.\n"); > + return -EOPNOTSUPP; > + } And this check should also live in the core, now that core has access to dev->ethtool->hds_config ? I think you can add this check to the core in the same patch as the chunk referred to above.
On Wed, 18 Dec 2024 18:25:47 -0800 Jakub Kicinski wrote: > > + if (prog && bp->flags & BNXT_FLAG_HDS) { > > + netdev_warn(dev, "XDP is disallowed when HDS is enabled.\n"); > > + return -EOPNOTSUPP; > > + } > > And this check should also live in the core, now that core has access > to dev->ethtool->hds_config ? I think you can add this check to the > core in the same patch as the chunk referred to above. Oh, you also already have this logic in patch 7? So it just needs to be reordered in the series, and then the driver doesn't need to check?
On Thu, Dec 19, 2024 at 11:25 AM Jakub Kicinski <kuba@kernel.org> wrote: > > On Wed, 18 Dec 2024 14:45:24 +0000 Taehee Yoo wrote: > > + if (tcp_data_split == ETHTOOL_TCP_DATA_SPLIT_DISABLED && hds_config_mod) > > + return -EOPNOTSUPP; > > I think ethtool ops generally return -EINVAL when param not supported. > EOPNOTSUPP means entire op is not supported (again, that's just how > ethtool ops generally work, not a kernel-wide rule). Thanks! I will use -EINVAL instead of EOPNOTSUPP. > > > + if (tcp_data_split == ETHTOOL_TCP_DATA_SPLIT_ENABLED && > > + hds_config_mod && BNXT_RX_PAGE_MODE(bp)) { > > Looks like patch 4 adds this check in the core. I think adding the > check in the core can be a separate patch. If you put it before this > patch in the series this bnxt check can be removed? > > I mean this chunk in the core: > > + hds_config_mod = old_hds_config != kernel_ringparam.tcp_data_split; > + if (kernel_ringparam.tcp_data_split == ETHTOOL_TCP_DATA_SPLIT_ENABLED && > + hds_config_mod && dev_xdp_sb_prog_count(dev)) { > + NL_SET_ERR_MSG(info->extack, > + "tcp-data-split can not be enabled with single buffer XDP"); > + return -EINVAL; > + } > Right, The core checks single buffer XDP. But there was a review that bnxt_en driver doesn't support both single and multi buffer XDP if HDS is in use. So, this is the reason why logic exists. > It's currently in the hds-thresh patch but really it's unrelated > to the threshold.. Thanks, I will move this code to a HDS related patch, not hds-threshold, or create new patch for this. > > > + NL_SET_ERR_MSG_MOD(extack, "tcp-data-split is disallowed when XDP is attached"); > > + return -EOPNOTSUPP; > > + } > > + > > if (netif_running(dev)) > > bnxt_close_nic(bp, false, false); > > > > + if (hds_config_mod) { > > + if (tcp_data_split == ETHTOOL_TCP_DATA_SPLIT_ENABLED) > > + bp->flags |= BNXT_FLAG_HDS; > > + else if (tcp_data_split == ETHTOOL_TCP_DATA_SPLIT_UNKNOWN) > > + bp->flags &= ~BNXT_FLAG_HDS; > > + } > > + > > bp->rx_ring_size = ering->rx_pending; > > bp->tx_ring_size = ering->tx_pending; > > bnxt_set_ring_params(bp); > > @@ -5354,6 +5374,7 @@ const struct ethtool_ops bnxt_ethtool_ops = { > > ETHTOOL_COALESCE_STATS_BLOCK_USECS | > > ETHTOOL_COALESCE_USE_ADAPTIVE_RX | > > ETHTOOL_COALESCE_USE_CQE, > > + .supported_ring_params = ETHTOOL_RING_USE_TCP_DATA_SPLIT, > > .get_link_ksettings = bnxt_get_link_ksettings, > > .set_link_ksettings = bnxt_set_link_ksettings, > > .get_fec_stats = bnxt_get_fec_stats, > > diff --git a/drivers/net/ethernet/broadcom/bnxt/bnxt_xdp.c b/drivers/net/ethernet/broadcom/bnxt/bnxt_xdp.c > > index f88b641533fc..1bfff7f29310 100644 > > --- a/drivers/net/ethernet/broadcom/bnxt/bnxt_xdp.c > > +++ b/drivers/net/ethernet/broadcom/bnxt/bnxt_xdp.c > > @@ -395,6 +395,10 @@ static int bnxt_xdp_set(struct bnxt *bp, struct bpf_prog *prog) > > bp->dev->mtu, BNXT_MAX_PAGE_MODE_MTU); > > return -EOPNOTSUPP; > > } > > + if (prog && bp->flags & BNXT_FLAG_HDS) { > > + netdev_warn(dev, "XDP is disallowed when HDS is enabled.\n"); > > + return -EOPNOTSUPP; > > + } > > And this check should also live in the core, now that core has access > to dev->ethtool->hds_config ? I think you can add this check to the > core in the same patch as the chunk referred to above. The bnxt_en disallows setting up both single and multi buffer XDP, but core checks only single buffer XDP. So, if multi buffer XDP is attaching to the bnxt_en driver when HDS is enabled, the core can't filter it. Thanks a lot! Taehee Yoo
On Thu, 19 Dec 2024 23:05:30 +0900 Taehee Yoo wrote: > > > diff --git a/drivers/net/ethernet/broadcom/bnxt/bnxt_xdp.c b/drivers/net/ethernet/broadcom/bnxt/bnxt_xdp.c > > > index f88b641533fc..1bfff7f29310 100644 > > > --- a/drivers/net/ethernet/broadcom/bnxt/bnxt_xdp.c > > > +++ b/drivers/net/ethernet/broadcom/bnxt/bnxt_xdp.c > > > @@ -395,6 +395,10 @@ static int bnxt_xdp_set(struct bnxt *bp, struct bpf_prog *prog) > > > bp->dev->mtu, BNXT_MAX_PAGE_MODE_MTU); > > > return -EOPNOTSUPP; > > > } > > > + if (prog && bp->flags & BNXT_FLAG_HDS) { > > > + netdev_warn(dev, "XDP is disallowed when HDS is enabled.\n"); > > > + return -EOPNOTSUPP; > > > + } > > > > And this check should also live in the core, now that core has access > > to dev->ethtool->hds_config ? I think you can add this check to the > > core in the same patch as the chunk referred to above. > > The bnxt_en disallows setting up both single and multi buffer XDP, but core > checks only single buffer XDP. So, if multi buffer XDP is attaching to > the bnxt_en driver when HDS is enabled, the core can't filter it. Hm. Did you find this in the code, or did Broadcom folks suggest it? AFAICT bnxt supports multi-buf XDP. Is there something in the code that special-cases aggregation but doesn't work for pure HDS?
On Thu, Dec 19, 2024 at 11:29 PM Jakub Kicinski <kuba@kernel.org> wrote: > > On Thu, 19 Dec 2024 23:05:30 +0900 Taehee Yoo wrote: > > > > diff --git a/drivers/net/ethernet/broadcom/bnxt/bnxt_xdp.c b/drivers/net/ethernet/broadcom/bnxt/bnxt_xdp.c > > > > index f88b641533fc..1bfff7f29310 100644 > > > > --- a/drivers/net/ethernet/broadcom/bnxt/bnxt_xdp.c > > > > +++ b/drivers/net/ethernet/broadcom/bnxt/bnxt_xdp.c > > > > @@ -395,6 +395,10 @@ static int bnxt_xdp_set(struct bnxt *bp, struct bpf_prog *prog) > > > > bp->dev->mtu, BNXT_MAX_PAGE_MODE_MTU); > > > > return -EOPNOTSUPP; > > > > } > > > > + if (prog && bp->flags & BNXT_FLAG_HDS) { > > > > + netdev_warn(dev, "XDP is disallowed when HDS is enabled.\n"); > > > > + return -EOPNOTSUPP; > > > > + } > > > > > > And this check should also live in the core, now that core has access > > > to dev->ethtool->hds_config ? I think you can add this check to the > > > core in the same patch as the chunk referred to above. > > > > The bnxt_en disallows setting up both single and multi buffer XDP, but core > > checks only single buffer XDP. So, if multi buffer XDP is attaching to > > the bnxt_en driver when HDS is enabled, the core can't filter it. > > Hm. Did you find this in the code, or did Broadcom folks suggest it? > AFAICT bnxt supports multi-buf XDP. Is there something in the code > that special-cases aggregation but doesn't work for pure HDS? There were some comments about HDS with XDP in the following thread. https://lore.kernel.org/netdev/20241022162359.2713094-1-ap420073@gmail.com/T/ I may misunderstand reviews from Broadcom folks.
On Fri, 20 Dec 2024 00:14:01 +0900 Taehee Yoo wrote: > > > The bnxt_en disallows setting up both single and multi buffer XDP, but core > > > checks only single buffer XDP. So, if multi buffer XDP is attaching to > > > the bnxt_en driver when HDS is enabled, the core can't filter it. > > > > Hm. Did you find this in the code, or did Broadcom folks suggest it? > > AFAICT bnxt supports multi-buf XDP. Is there something in the code > > that special-cases aggregation but doesn't work for pure HDS? > > There were some comments about HDS with XDP in the following thread. > https://lore.kernel.org/netdev/20241022162359.2713094-1-ap420073@gmail.com/T/ > I may misunderstand reviews from Broadcom folks. I see it now in bnxt_set_rx_skb_mode. I guess with high MTU the device splits in some "dumb" way, at a fixed offset.. You're right, we have to keep the check in the driver, at least for now.
On Thu, Dec 19, 2024 at 07:25:19AM -0800, Jakub Kicinski wrote: > On Fri, 20 Dec 2024 00:14:01 +0900 Taehee Yoo wrote: > > > > The bnxt_en disallows setting up both single and multi buffer XDP, but core > > > > checks only single buffer XDP. So, if multi buffer XDP is attaching to > > > > the bnxt_en driver when HDS is enabled, the core can't filter it. > > > > > > Hm. Did you find this in the code, or did Broadcom folks suggest it? > > > AFAICT bnxt supports multi-buf XDP. Is there something in the code > > > that special-cases aggregation but doesn't work for pure HDS? > > > > There were some comments about HDS with XDP in the following thread. > > https://lore.kernel.org/netdev/20241022162359.2713094-1-ap420073@gmail.com/T/ > > I may misunderstand reviews from Broadcom folks. > > I see it now in bnxt_set_rx_skb_mode. I guess with high MTU > the device splits in some "dumb" way, at a fixed offset.. > You're right, we have to keep the check in the driver, > at least for now. The mutlti-buffer implementation followed what was done at the time in other drivers. Is the 'dumb way' you mention this check? 4717 if (dev->mtu > BNXT_MAX_PAGE_MODE_MTU) { 4718 bp->flags |= BNXT_FLAG_JUMBO; 4719 bp->rx_skb_func = bnxt_rx_multi_page_skb; 4720 } else { 4721 bp->flags |= BNXT_FLAG_NO_AGG_RINGS; 4722 bp->rx_skb_func = bnxt_rx_page_skb; 4723 }
On Thu, 19 Dec 2024 14:33:44 -0500 Andy Gospodarek wrote: > > I see it now in bnxt_set_rx_skb_mode. I guess with high MTU > > the device splits in some "dumb" way, at a fixed offset.. > > You're right, we have to keep the check in the driver, > > at least for now. > > The mutlti-buffer implementation followed what was done at the time in > other drivers. Is the 'dumb way' you mention this check? > > 4717 if (dev->mtu > BNXT_MAX_PAGE_MODE_MTU) { > 4718 bp->flags |= BNXT_FLAG_JUMBO; > 4719 bp->rx_skb_func = bnxt_rx_multi_page_skb; > 4720 } else { > 4721 bp->flags |= BNXT_FLAG_NO_AGG_RINGS; > 4722 bp->rx_skb_func = bnxt_rx_page_skb; > 4723 } Yes, that and my interpretation of the previous discussion let me to believe that the BNXT_FLAG_JUMBO does not enable header-data split. And speculating further I thought that perhaps the buffer split with jumbo > 4k is to fill first buffer completely, header+however much data fits. I could have misread the previous conversation (perhaps Michael meant XDP SB / PAGE_MODE when he was referring to XDP limitations?) Or maybe the HDS does happen with XDP MB but there is another limitation in the code? I'm not sure. At this stage we just need to know if the check in the driver is really needed or XDP MB + HDS are fine, and we can remove the driver check, as core already prevents XDP SB + HDS. Could you clarify?
On Thu, Dec 19, 2024 at 12:18 PM Jakub Kicinski <kuba@kernel.org> wrote: > > On Thu, 19 Dec 2024 14:33:44 -0500 Andy Gospodarek wrote: > > > I see it now in bnxt_set_rx_skb_mode. I guess with high MTU > > > the device splits in some "dumb" way, at a fixed offset.. > > > You're right, we have to keep the check in the driver, > > > at least for now. > > > > The mutlti-buffer implementation followed what was done at the time in > > other drivers. Is the 'dumb way' you mention this check? > > > > 4717 if (dev->mtu > BNXT_MAX_PAGE_MODE_MTU) { > > 4718 bp->flags |= BNXT_FLAG_JUMBO; > > 4719 bp->rx_skb_func = bnxt_rx_multi_page_skb; > > 4720 } else { > > 4721 bp->flags |= BNXT_FLAG_NO_AGG_RINGS; > > 4722 bp->rx_skb_func = bnxt_rx_page_skb; > > 4723 } > > Yes, that and my interpretation of the previous discussion let me to > believe that the BNXT_FLAG_JUMBO does not enable header-data split. > And speculating further I thought that perhaps the buffer split with > jumbo > 4k is to fill first buffer completely, header+however much > data fits. > > I could have misread the previous conversation (perhaps Michael meant > XDP SB / PAGE_MODE when he was referring to XDP limitations?) To clarify, my review comment applied to XDP SB and MB modes. Andy's MB implementation from 2022 disables HWGRO/LRO and HDS in XDP MB mode. My comment was to preserve this implementation. > > Or maybe the HDS does happen with XDP MB but there is another > limitation in the code? HW doesn't know whether we're in XDP mode or not and can definitely do HDS. But again, HDS is disabled currently in any XDP mode. Andy will respond to discuss this further. Long term, we may be able to enable HDS in XDP MB mode, but for now I think we should disable it just to keep it unchanged.
On Thu, 19 Dec 2024 15:41:17 -0800 Michael Chan wrote: > > Or maybe the HDS does happen with XDP MB but there is another > > limitation in the code? > > HW doesn't know whether we're in XDP mode or not and can definitely do > HDS. But again, HDS is disabled currently in any XDP mode. Andy will > respond to discuss this further. Long term, we may be able to enable > HDS in XDP MB mode, but for now I think we should disable it just to > keep it unchanged. SGTM, this series accomplishes enough things already :)
diff --git a/drivers/net/ethernet/broadcom/bnxt/bnxt.c b/drivers/net/ethernet/broadcom/bnxt/bnxt.c index c31894b9187e..42ffaa88ae4e 100644 --- a/drivers/net/ethernet/broadcom/bnxt/bnxt.c +++ b/drivers/net/ethernet/broadcom/bnxt/bnxt.c @@ -4623,7 +4623,7 @@ void bnxt_set_ring_params(struct bnxt *bp) bp->rx_agg_ring_size = 0; bp->rx_agg_nr_pages = 0; - if (bp->flags & BNXT_FLAG_TPA) + if (bp->flags & BNXT_FLAG_TPA || bp->flags & BNXT_FLAG_HDS) agg_factor = min_t(u32, 4, 65536 / BNXT_RX_PAGE_SIZE); bp->flags &= ~BNXT_FLAG_JUMBO; diff --git a/drivers/net/ethernet/broadcom/bnxt/bnxt.h b/drivers/net/ethernet/broadcom/bnxt/bnxt.h index b73de5683063..847dedf61a9e 100644 --- a/drivers/net/ethernet/broadcom/bnxt/bnxt.h +++ b/drivers/net/ethernet/broadcom/bnxt/bnxt.h @@ -2244,8 +2244,6 @@ struct bnxt { #define BNXT_FLAG_TPA (BNXT_FLAG_LRO | BNXT_FLAG_GRO) #define BNXT_FLAG_JUMBO 0x10 #define BNXT_FLAG_STRIP_VLAN 0x20 - #define BNXT_FLAG_AGG_RINGS (BNXT_FLAG_JUMBO | BNXT_FLAG_GRO | \ - BNXT_FLAG_LRO) #define BNXT_FLAG_RFS 0x100 #define BNXT_FLAG_SHARED_RINGS 0x200 #define BNXT_FLAG_PORT_STATS 0x400 @@ -2266,6 +2264,9 @@ struct bnxt { #define BNXT_FLAG_ROCE_MIRROR_CAP 0x4000000 #define BNXT_FLAG_TX_COAL_CMPL 0x8000000 #define BNXT_FLAG_PORT_STATS_EXT 0x10000000 + #define BNXT_FLAG_HDS 0x20000000 + #define BNXT_FLAG_AGG_RINGS (BNXT_FLAG_JUMBO | BNXT_FLAG_GRO | \ + BNXT_FLAG_LRO | BNXT_FLAG_HDS) #define BNXT_FLAG_ALL_CONFIG_FEATS (BNXT_FLAG_TPA | \ BNXT_FLAG_RFS | \ diff --git a/drivers/net/ethernet/broadcom/bnxt/bnxt_ethtool.c b/drivers/net/ethernet/broadcom/bnxt/bnxt_ethtool.c index 4cdfff5d531c..25eb5931aea9 100644 --- a/drivers/net/ethernet/broadcom/bnxt/bnxt_ethtool.c +++ b/drivers/net/ethernet/broadcom/bnxt/bnxt_ethtool.c @@ -840,16 +840,36 @@ static int bnxt_set_ringparam(struct net_device *dev, struct kernel_ethtool_ringparam *kernel_ering, struct netlink_ext_ack *extack) { + u8 tcp_data_split = kernel_ering->tcp_data_split; struct bnxt *bp = netdev_priv(dev); + u8 hds_config_mod; + + hds_config_mod = tcp_data_split != dev->ethtool->hds_config; if ((ering->rx_pending > BNXT_MAX_RX_DESC_CNT) || (ering->tx_pending > BNXT_MAX_TX_DESC_CNT) || (ering->tx_pending < BNXT_MIN_TX_DESC_CNT)) return -EINVAL; + if (tcp_data_split == ETHTOOL_TCP_DATA_SPLIT_DISABLED && hds_config_mod) + return -EOPNOTSUPP; + + if (tcp_data_split == ETHTOOL_TCP_DATA_SPLIT_ENABLED && + hds_config_mod && BNXT_RX_PAGE_MODE(bp)) { + NL_SET_ERR_MSG_MOD(extack, "tcp-data-split is disallowed when XDP is attached"); + return -EOPNOTSUPP; + } + if (netif_running(dev)) bnxt_close_nic(bp, false, false); + if (hds_config_mod) { + if (tcp_data_split == ETHTOOL_TCP_DATA_SPLIT_ENABLED) + bp->flags |= BNXT_FLAG_HDS; + else if (tcp_data_split == ETHTOOL_TCP_DATA_SPLIT_UNKNOWN) + bp->flags &= ~BNXT_FLAG_HDS; + } + bp->rx_ring_size = ering->rx_pending; bp->tx_ring_size = ering->tx_pending; bnxt_set_ring_params(bp); @@ -5354,6 +5374,7 @@ const struct ethtool_ops bnxt_ethtool_ops = { ETHTOOL_COALESCE_STATS_BLOCK_USECS | ETHTOOL_COALESCE_USE_ADAPTIVE_RX | ETHTOOL_COALESCE_USE_CQE, + .supported_ring_params = ETHTOOL_RING_USE_TCP_DATA_SPLIT, .get_link_ksettings = bnxt_get_link_ksettings, .set_link_ksettings = bnxt_set_link_ksettings, .get_fec_stats = bnxt_get_fec_stats, diff --git a/drivers/net/ethernet/broadcom/bnxt/bnxt_xdp.c b/drivers/net/ethernet/broadcom/bnxt/bnxt_xdp.c index f88b641533fc..1bfff7f29310 100644 --- a/drivers/net/ethernet/broadcom/bnxt/bnxt_xdp.c +++ b/drivers/net/ethernet/broadcom/bnxt/bnxt_xdp.c @@ -395,6 +395,10 @@ static int bnxt_xdp_set(struct bnxt *bp, struct bpf_prog *prog) bp->dev->mtu, BNXT_MAX_PAGE_MODE_MTU); return -EOPNOTSUPP; } + if (prog && bp->flags & BNXT_FLAG_HDS) { + netdev_warn(dev, "XDP is disallowed when HDS is enabled.\n"); + return -EOPNOTSUPP; + } if (!(bp->flags & BNXT_FLAG_SHARED_RINGS)) { netdev_warn(dev, "ethtool rx/tx channels must be combined to support XDP.\n"); return -EOPNOTSUPP;