| Message ID | DM6PR18MB338861990B56CCA5A4384779AB540@DM6PR18MB3388.namprd18.prod.outlook.com |
|---|---|
| State | RFC |
| Delegated to: | David Miller |
| Headers | show |
| Series | LRO/HW_GRO is not disabled when native xdp is installed | expand |
On Thu, Dec 12, 2019 at 7:13 PM Manish Chopra <manishc@marvell.com> wrote: > When attaching native xdp program, device's aggregation features (i.e LRO/HW_GRO) are not getting disabled. > They seems to be getting disabled only in case of generic xdp install, not in case of native/driver mode xdp, That sounds right. For bnxt, when an xdp program is attached, the driver will run in a special paging mode that won't support LRO or hardware GRO. So they are automatically turned off. Isn't that the case for your device as well?
Hi Michael, > -----Original Message----- > From: Michael Chan <michael.chan@broadcom.com> > Sent: Friday, December 13, 2019 9:41 AM > To: Manish Chopra <manishc@marvell.com> > Cc: netdev@vger.kernel.org > Subject: [EXT] Re: LRO/HW_GRO is not disabled when native xdp is installed > > External Email > > ---------------------------------------------------------------------- > On Thu, Dec 12, 2019 at 7:13 PM Manish Chopra <manishc@marvell.com> > wrote: > > > When attaching native xdp program, device's aggregation features (i.e > LRO/HW_GRO) are not getting disabled. > > They seems to be getting disabled only in case of generic xdp install, > > not in case of native/driver mode xdp, > > That sounds right. For bnxt, when an xdp program is attached, the driver > will run in a special paging mode that won't support LRO or hardware GRO. > So they are automatically turned off. Isn't that the case for your device as > well? It used to be the case for our devices as well long back. but after the commit 18c602dee4726 ("qede: Use NETIF_F_GRO_HW") that part was removed from our driver and commit 56f5aa77cdad1 ("net: Disable GRO_HW when generic XDP is installed on a device") was added to achieve the same instead of individual driver doing it, but it wasn't caught that time why later commit only does for generic xdp, not for native xdp. So today when native xdp is attached to our device, HW GRO aggregation remains enabled on our device. Please let me know if that fix is good enough to be posted. Will test it out and post. Thanks, Manish
On Fri, Dec 13, 2019 at 1:45 AM Manish Chopra <manishc@marvell.com> wrote: > It used to be the case for our devices as well long back. but after the commit 18c602dee4726 ("qede: Use NETIF_F_GRO_HW") > that part was removed from our driver and commit 56f5aa77cdad1 ("net: Disable GRO_HW when generic XDP is installed on a device") > was added to achieve the same instead of individual driver doing it, but it wasn't caught that time why later commit only does for generic xdp, > not for native xdp. So today when native xdp is attached to our device, HW GRO aggregation remains enabled on our device. > Please let me know if that fix is good enough to be posted. Will test it out and post. > The driver knows when an xdp program is attached and can disable any features that are not compatible, so I think it is more efficient to keep it this way. Generic XDP on the other hand, does not involve the driver and so we need to disable them for the driver.
On Fri, 13 Dec 2019 13:09:31 -0800, Michael Chan wrote: > On Fri, Dec 13, 2019 at 1:45 AM Manish Chopra <manishc@marvell.com> wrote: > > > It used to be the case for our devices as well long back. but after the commit 18c602dee4726 ("qede: Use NETIF_F_GRO_HW") > > that part was removed from our driver and commit 56f5aa77cdad1 ("net: Disable GRO_HW when generic XDP is installed on a device") > > was added to achieve the same instead of individual driver doing it, but it wasn't caught that time why later commit only does for generic xdp, > > not for native xdp. So today when native xdp is attached to our device, HW GRO aggregation remains enabled on our device. > > Please let me know if that fix is good enough to be posted. Will test it out and post. > > > > The driver knows when an xdp program is attached and can disable any > features that are not compatible, so I think it is more efficient to > keep it this way. Generic XDP on the other hand, does not involve the > driver and so we need to disable them for the driver. The only question is should the driver refuse the XDP prog installation (with a nice extack message) or should it silently disable the feature? IIRC ixgbe opts for returning -EINVAL if RSC/LRO is enabled. Micheal says that bnxt disables automatically. My preference is for the former, because it's simpler - we all keep the MTU intact and don't disable queues to make room for XDP, IOW don't automatically change user controlled configuration is a simpler policy. But I don't feel too strongly, we should just make sure we document what's the expected behaviour (even better make netdevsim behave that way and write a test). Manish, if you were to go ahead with your patch please make sure you don't disable the features when program is installed in offload mode, though.
> -----Original Message----- > From: Jakub Kicinski <jakub.kicinski@netronome.com> > Sent: Sunday, December 15, 2019 1:12 AM > To: Michael Chan <michael.chan@broadcom.com> > Cc: Manish Chopra <manishc@marvell.com>; netdev@vger.kernel.org > Subject: Re: [EXT] Re: LRO/HW_GRO is not disabled when native xdp is > installed > > On Fri, 13 Dec 2019 13:09:31 -0800, Michael Chan wrote: > > On Fri, Dec 13, 2019 at 1:45 AM Manish Chopra <manishc@marvell.com> > wrote: > > > > > It used to be the case for our devices as well long back. but after > > > the commit 18c602dee4726 ("qede: Use NETIF_F_GRO_HW") that part > was > > > removed from our driver and commit 56f5aa77cdad1 ("net: Disable > > > GRO_HW when generic XDP is installed on a device") was added to > achieve the same instead of individual driver doing it, but it wasn't caught > that time why later commit only does for generic xdp, not for native xdp. So > today when native xdp is attached to our device, HW GRO aggregation > remains enabled on our device. > > > Please let me know if that fix is good enough to be posted. Will test it > out and post. > > > > > > > The driver knows when an xdp program is attached and can disable any > > features that are not compatible, so I think it is more efficient to > > keep it this way. Generic XDP on the other hand, does not involve the > > driver and so we need to disable them for the driver. > > The only question is should the driver refuse the XDP prog installation (with > a nice extack message) or should it silently disable the feature? > IIRC ixgbe opts for returning -EINVAL if RSC/LRO is enabled. Micheal says that > bnxt disables automatically. My preference is for the former, because it's > simpler - we all keep the MTU intact and don't disable queues to make room > for XDP, IOW don't automatically change user controlled configuration is a > simpler policy. But I don't feel too strongly, we should just make sure we > document what's the expected behaviour (even better make netdevsim > behave that way and write a test). > > Manish, if you were to go ahead with your patch please make sure you don't > disable the features when program is installed in offload mode, though. Thanks Michael and Jakub, I will rather opt to fix this in qede driver - qede will implicitly disable the HW gro and allow the xdp prog installation instead of failing it (just the way bridging disables LRO implicitly on underlined NIC). I could also choose to fail xdp installation if HW gro is enabled, but that will require some user intervention to disable HW gro explicitly by user and I am not sure if such failure from qede can lead to unexpected issues in some deployment environment. Regards, Manish
diff --git a/net/core/dev.c b/net/core/dev.c index c7db39926769..8a128a34378f 100644 --- a/net/core/dev.c +++ b/net/core/dev.c @@ -4580,8 +4580,6 @@ static int generic_xdp_install(struct net_device *dev, struct netdev_bpf *xdp) static_key_slow_dec(&generic_xdp_needed); } else if (new && !old) { static_key_slow_inc(&generic_xdp_needed); - dev_disable_lro(dev); - dev_disable_gro_hw(dev); } break; @@ -7216,8 +7214,12 @@ int dev_change_xdp_fd(struct net_device *dev, struct netlink_ext_ack *extack, } err = dev_xdp_install(dev, bpf_op, extack, flags, prog); - if (err < 0 && prog) + if (err < 0 && prog) { bpf_prog_put(prog); + } else { + dev_disable_lro(dev); + dev_disable_gro_hw(dev); + } return err; }