LRO/HW_GRO is not disabled when native xdp is installed

LRO/HW_GRO is not disabled when native xdp is installed

[Manish Chopra]

Hello,

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,
Shouldn't it be done something like below ?? if so, please let me know if I can post a patch like below.

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;
}

Thanks,
Manish

Re: LRO/HW_GRO is not disabled when native xdp is installed

[Michael Chan]

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?

RE: [EXT] Re: LRO/HW_GRO is not disabled when native xdp is installed

[Manish Chopra]

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

Re: [EXT] Re: LRO/HW_GRO is not disabled when native xdp is installed

[Michael Chan]

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.

Re: [EXT] Re: LRO/HW_GRO is not disabled when native xdp is installed

[Jakub Kicinski]

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.

RE: [EXT] Re: LRO/HW_GRO is not disabled when native xdp is installed

[Manish Chopra]

> -----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