| Message ID | 20250220005318.560733-1-kuba@kernel.org (mailing list archive) |
|---|---|
| State | Changes Requested |
| Delegated to: | Netdev Maintainers |
| Headers | show |
| Series | [net,1/2] bnxt: don't reject XDP installation when HDS isn't forced on | expand |
On Wed, Feb 19, 2025 at 04:53:17PM -0800, Jakub Kicinski wrote: > HDS flag is often set because GRO-HW is enabled. But we call > bnxt_set_rx_skb_mode() later, which will clear it. So make > sure we reject XDP when user asked for HDS, not when it's > enabled for other reasons. > > Fixes: 87c8f8496a05 ("bnxt_en: add support for tcp-data-split ethtool command") > Signed-off-by: Jakub Kicinski <kuba@kernel.org> > --- > CC: daniel@iogearbox.net > CC: hawk@kernel.org > CC: john.fastabend@gmail.com > CC: michael.chan@broadcom.com > CC: pavan.chebbi@broadcom.com > CC: ap420073@gmail.com > --- > drivers/net/ethernet/broadcom/bnxt/bnxt_xdp.c | 4 +++- > 1 file changed, 3 insertions(+), 1 deletion(-) > > diff --git a/drivers/net/ethernet/broadcom/bnxt/bnxt_xdp.c b/drivers/net/ethernet/broadcom/bnxt/bnxt_xdp.c > index e6c64e4bd66c..ff208c4b7d70 100644 > --- a/drivers/net/ethernet/broadcom/bnxt/bnxt_xdp.c > +++ b/drivers/net/ethernet/broadcom/bnxt/bnxt_xdp.c > @@ -11,10 +11,12 @@ > #include <linux/pci.h> > #include <linux/netdevice.h> > #include <linux/etherdevice.h> > +#include <linux/ethtool_netlink.h> > #include <linux/if_vlan.h> > #include <linux/bpf.h> > #include <linux/bpf_trace.h> > #include <linux/filter.h> > +#include <net/netdev_queues.h> > #include <net/page_pool/helpers.h> > #include "bnxt_hsi.h" > #include "bnxt.h" > @@ -395,7 +397,7 @@ 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) { > + if (prog && dev->cfg->hds_config == ETHTOOL_TCP_DATA_SPLIT_ENABLED) { > netdev_warn(dev, "XDP is disallowed when HDS is enabled.\n"); > return -EOPNOTSUPP; > } > -- > 2.48.1 > Nice, that fixed it. Tested-by: Daniel Xu <dxu@dxuuu.xyz>
On Wed, 19 Feb 2025 18:58:02 -0700 Daniel Xu wrote: > > @@ -395,7 +397,7 @@ 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) { > > + if (prog && dev->cfg->hds_config == ETHTOOL_TCP_DATA_SPLIT_ENABLED) { > > netdev_warn(dev, "XDP is disallowed when HDS is enabled.\n"); > > return -EOPNOTSUPP; > > } > > -- > > 2.48.1 > > > > Nice, that fixed it. > > Tested-by: Daniel Xu <dxu@dxuuu.xyz> I looked again after sending because it wasn't sitting 100% well with me. As the commit message says this will work, because it forces all flags to off. But the driver is also only setting its internal flag when user requested. So why does it get set in the first place.. I think the real fix may be: @@ -2071,6 +2072,8 @@ static int ethtool_set_ringparam(struct net_device *dev, void __user *useraddr) dev->ethtool_ops->get_ringparam(dev, &max, &kernel_ringparam, NULL); + kernel_ringparam.tcp_data_split = dev->cfg->hds_config; + /* ensure new ring parameters are within the maximums */ if (ringparam.rx_pending > max.rx_max_pending || ringparam.rx_mini_pending > max.rx_mini_max_pending || This is the legacy / ioctl path. We don't hit it in testing, but you probably hit it via systemd. At least that's my current theory, waiting for the test kernel to deploy. Sorry for the flip flop..
On Thu, Feb 20, 2025 at 11:14 AM Jakub Kicinski <kuba@kernel.org> wrote: Hi Jakub, Thank you so much for this fix! > > On Wed, 19 Feb 2025 18:58:02 -0700 Daniel Xu wrote: > > > @@ -395,7 +397,7 @@ 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) { > > > + if (prog && dev->cfg->hds_config == ETHTOOL_TCP_DATA_SPLIT_ENABLED) { > > > netdev_warn(dev, "XDP is disallowed when HDS is enabled.\n"); > > > return -EOPNOTSUPP; > > > } > > > -- > > > 2.48.1 > > > > > > > Nice, that fixed it. > > > > Tested-by: Daniel Xu <dxu@dxuuu.xyz> > > I looked again after sending because it wasn't sitting 100% well with > me. As the commit message says this will work, because it forces all > flags to off. But the driver is also only setting its internal flag > when user requested. So why does it get set in the first place.. > > I think the real fix may be: > > @@ -2071,6 +2072,8 @@ static int ethtool_set_ringparam(struct net_device *dev, void __user *useraddr) > > dev->ethtool_ops->get_ringparam(dev, &max, &kernel_ringparam, NULL); > > + kernel_ringparam.tcp_data_split = dev->cfg->hds_config; > + > /* ensure new ring parameters are within the maximums */ > if (ringparam.rx_pending > max.rx_max_pending || > ringparam.rx_mini_pending > max.rx_mini_max_pending || > > This is the legacy / ioctl path. We don't hit it in testing, but you > probably hit it via systemd. > > At least that's my current theory, waiting for the test kernel > to deploy. Sorry for the flip flop.. As you mentioned, I tested it with legacy/ioctl path. How to reproduce: ethtool -K eth0 lro on gro on ethtool --disable-netlink -G eth0 rx 512 ip link set eth0 xdp obj xdp.o With this change, I can't see this bug anymore. Thanks a lot! Taehee Yoo
diff --git a/drivers/net/ethernet/broadcom/bnxt/bnxt_xdp.c b/drivers/net/ethernet/broadcom/bnxt/bnxt_xdp.c index e6c64e4bd66c..ff208c4b7d70 100644 --- a/drivers/net/ethernet/broadcom/bnxt/bnxt_xdp.c +++ b/drivers/net/ethernet/broadcom/bnxt/bnxt_xdp.c @@ -11,10 +11,12 @@ #include <linux/pci.h> #include <linux/netdevice.h> #include <linux/etherdevice.h> +#include <linux/ethtool_netlink.h> #include <linux/if_vlan.h> #include <linux/bpf.h> #include <linux/bpf_trace.h> #include <linux/filter.h> +#include <net/netdev_queues.h> #include <net/page_pool/helpers.h> #include "bnxt_hsi.h" #include "bnxt.h" @@ -395,7 +397,7 @@ 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) { + if (prog && dev->cfg->hds_config == ETHTOOL_TCP_DATA_SPLIT_ENABLED) { netdev_warn(dev, "XDP is disallowed when HDS is enabled.\n"); return -EOPNOTSUPP; }
HDS flag is often set because GRO-HW is enabled. But we call bnxt_set_rx_skb_mode() later, which will clear it. So make sure we reject XDP when user asked for HDS, not when it's enabled for other reasons. Fixes: 87c8f8496a05 ("bnxt_en: add support for tcp-data-split ethtool command") Signed-off-by: Jakub Kicinski <kuba@kernel.org> --- CC: daniel@iogearbox.net CC: hawk@kernel.org CC: john.fastabend@gmail.com CC: michael.chan@broadcom.com CC: pavan.chebbi@broadcom.com CC: ap420073@gmail.com --- drivers/net/ethernet/broadcom/bnxt/bnxt_xdp.c | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-)