[PATCH net] openvswitch: disable LRO unless stated otherwise

[PATCH net] openvswitch: disable LRO unless stated otherwise

[Jiri Benc]

Currently, openvswitch tries to disable LRO from the user space. This does
not work correctly when the device added is a vlan interface, though.
Instead of dealing with possibly complex stacked cross name space relations
in the user space, do the same as bridging does and call dev_disable_lro in
the kernel.

As there are use cases of openvswitch setup that can keep LRO enabled and
there's a planned feature to optimize such use cases (and stop disabling LRO
unconditionally from the user space daemon), allow the user space to
override this when adding the interface.

Signed-off-by: Jiri Benc <jbenc@redhat.com>
---
 include/uapi/linux/openvswitch.h | 9 +++++++++
 net/openvswitch/vport-netdev.c   | 3 +++
 2 files changed, 12 insertions(+)

diff --git a/include/uapi/linux/openvswitch.h b/include/uapi/linux/openvswitch.h
index bbd49a0c46c7..3832953a4f27 100644
--- a/include/uapi/linux/openvswitch.h
+++ b/include/uapi/linux/openvswitch.h
@@ -252,6 +252,15 @@ enum ovs_vport_attr {
 
 #define OVS_VPORT_ATTR_MAX (__OVS_VPORT_ATTR_MAX - 1)
 
+/* OVS_VPORT_ATTR_OPTIONS attributes for netdev vports.
+ */
+enum {
+	OVS_NETDEV_ATTR_KEEP_LRO, /* flag */
+	__OVS_NETDEV_ATTR_MAX
+};
+
+#define OVS_NETDEV_ATTR_MAX (__OVS_NETDEV_ATTR_MAX - 1)
+
 enum {
 	OVS_VXLAN_EXT_UNSPEC,
 	OVS_VXLAN_EXT_GBP,	/* Flag or __u32 */
diff --git a/net/openvswitch/vport-netdev.c b/net/openvswitch/vport-netdev.c
index 4776282c6417..04391b6cfb9a 100644
--- a/net/openvswitch/vport-netdev.c
+++ b/net/openvswitch/vport-netdev.c
@@ -125,6 +125,9 @@ static struct vport *netdev_create(const struct vport_parms *parms)
 	if (err)
 		goto error_master_upper_dev_unlink;
 
+	if (!parms->options ||
+	    !nla_find_nested(parms->options, OVS_NETDEV_ATTR_KEEP_LRO))
+		dev_disable_lro(netdev_vport->dev);
 	dev_set_promiscuity(netdev_vport->dev, 1);
 	netdev_vport->dev->priv_flags |= IFF_OVS_DATAPATH;
 	rtnl_unlock();
-- 
1.8.3.1

Re: [PATCH net] openvswitch: disable LRO unless stated otherwise

[Pravin Shelar]

On Tue, May 26, 2015 at 10:38 AM, Jiri Benc <jbenc@redhat.com> wrote:
> Currently, openvswitch tries to disable LRO from the user space. This does
> not work correctly when the device added is a vlan interface, though.
> Instead of dealing with possibly complex stacked cross name space relations
> in the user space, do the same as bridging does and call dev_disable_lro in
> the kernel.
>
> As there are use cases of openvswitch setup that can keep LRO enabled and
> there's a planned feature to optimize such use cases (and stop disabling LRO
> unconditionally from the user space daemon), allow the user space to
> override this when adding the interface.
>

OVS interface for generic networking device operation looks odd. have
you considered adding new device ioctl to do this?

> Signed-off-by: Jiri Benc <jbenc@redhat.com>
> ---
>  include/uapi/linux/openvswitch.h | 9 +++++++++
>  net/openvswitch/vport-netdev.c   | 3 +++
>  2 files changed, 12 insertions(+)
>
> diff --git a/include/uapi/linux/openvswitch.h b/include/uapi/linux/openvswitch.h
> index bbd49a0c46c7..3832953a4f27 100644
> --- a/include/uapi/linux/openvswitch.h
> +++ b/include/uapi/linux/openvswitch.h
> @@ -252,6 +252,15 @@ enum ovs_vport_attr {
>
>  #define OVS_VPORT_ATTR_MAX (__OVS_VPORT_ATTR_MAX - 1)
>
> +/* OVS_VPORT_ATTR_OPTIONS attributes for netdev vports.
> + */
> +enum {
> +       OVS_NETDEV_ATTR_KEEP_LRO, /* flag */
> +       __OVS_NETDEV_ATTR_MAX
> +};
> +
> +#define OVS_NETDEV_ATTR_MAX (__OVS_NETDEV_ATTR_MAX - 1)
> +
>  enum {
>         OVS_VXLAN_EXT_UNSPEC,
>         OVS_VXLAN_EXT_GBP,      /* Flag or __u32 */
_______________________________________________
dev mailing list
dev@openvswitch.org
http://openvswitch.org/mailman/listinfo/dev

Re: [PATCH net] openvswitch: disable LRO unless stated otherwise

[Jiri Benc]

On Tue, 26 May 2015 15:03:56 -0700, Pravin Shelar wrote:
> OVS interface for generic networking device operation looks odd. have
> you considered adding new device ioctl to do this?

New ioctls for networking configuration are generally not allowed. The
preferred way to configure networking is netlink. And as this is very
much ovs specific (all other users of dev_disable_lro such as bridging
want to do this unconditionally), ovs netlink is the correct place to
put this to.

 Jiri

-- 
Jiri Benc
_______________________________________________
dev mailing list
dev@openvswitch.org
http://openvswitch.org/mailman/listinfo/dev

Re: [ovs-dev] [PATCH net] openvswitch: disable LRO unless stated otherwise

[Flavio Leitner]

On Wed, May 27, 2015 at 10:50:21AM +0200, Jiri Benc wrote:
> On Tue, 26 May 2015 15:03:56 -0700, Pravin Shelar wrote:
> > OVS interface for generic networking device operation looks odd. have
> > you considered adding new device ioctl to do this?
> 
> New ioctls for networking configuration are generally not allowed. The
> preferred way to configure networking is netlink. And as this is very
> much ovs specific (all other users of dev_disable_lro such as bridging
> want to do this unconditionally), ovs netlink is the correct place to
> put this to.

Exactly. Team, Bonding, bridge, and when you enable forwarding on a
networking device gets LRO automatically disabled in the kernel.

I suggest to always disable LRO in the kernel as the other examples
do until there is a real need in OVS to benefit from LRO to implement
such API change.

fbl

Re: [ovs-dev] [PATCH net] openvswitch: disable LRO unless stated otherwise

[David Miller]

From: Flavio Leitner <fbl@sysclose.org>
Date: Wed, 27 May 2015 10:24:14 -0300

> On Wed, May 27, 2015 at 10:50:21AM +0200, Jiri Benc wrote:
>> On Tue, 26 May 2015 15:03:56 -0700, Pravin Shelar wrote:
>> > OVS interface for generic networking device operation looks odd. have
>> > you considered adding new device ioctl to do this?
>> 
>> New ioctls for networking configuration are generally not allowed. The
>> preferred way to configure networking is netlink. And as this is very
>> much ovs specific (all other users of dev_disable_lro such as bridging
>> want to do this unconditionally), ovs netlink is the correct place to
>> put this to.
> 
> Exactly. Team, Bonding, bridge, and when you enable forwarding on a
> networking device gets LRO automatically disabled in the kernel.
> 
> I suggest to always disable LRO in the kernel as the other examples
> do until there is a real need in OVS to benefit from LRO to implement
> such API change.

Agreed, just turn off LRO by default and don't add a new interface to
turn it back on until we actually need it.