netdev - Re: [ovs-dev] [patch net-next 10/13] openvswitch: add support for datapath hardware offload

Open Source and information security mailing list archives

Message-ID: <20140905035937.GA32481@vergenet.net>
Date:	Fri, 5 Sep 2014 12:59:38 +0900
From:	Simon Horman <simon.horman@...ronome.com>
To:	Jiri Pirko <jiri@...nulli.us>
Cc:	John Fastabend <john.fastabend@...il.com>, ryazanov.s.a@...il.com,
	jasowang@...hat.com, john.r.fastabend@...el.com,
	Neil.Jerram@...aswitch.com, edumazet@...gle.com,
	andy@...yhouse.net, dev@...nvswitch.org, nbd@...nwrt.org,
	f.fainelli@...il.com, ronye@...lanox.com,
	jeffrey.t.kirsher@...el.com, ogerlitz@...lanox.com,
	ben@...adent.org.uk, buytenh@...tstofly.org,
	roopa@...ulusnetworks.com, jhs@...atatu.com, aviadr@...lanox.com,
	nicolas.dichtel@...nd.com, vyasevic@...hat.com,
	nhorman@...driver.com, netdev@...r.kernel.org,
	stephen@...workplumber.org, dborkman@...hat.com,
	ebiederm@...ssion.com, davem@...emloft.net
Subject: Re: [ovs-dev] [patch net-next 10/13] openvswitch: add support for
 datapath hardware offload

On Thu, Sep 04, 2014 at 02:48:37PM +0200, Jiri Pirko wrote:
> Wed, Sep 03, 2014 at 06:37:08PM CEST, john.fastabend@...il.com wrote:
> >On 09/03/2014 02:24 AM, Jiri Pirko wrote:
> >>Benefit from the possibility to work with flows in switch devices and
> >>use the swdev api to offload flow datapath.
> >>
> >>Signed-off-by: Jiri Pirko <jiri@...nulli.us>
> >>---
> >>  net/openvswitch/Makefile       |   3 +-
> >>  net/openvswitch/datapath.c     |  33 ++++++
> >>  net/openvswitch/datapath.h     |   3 +
> >>  net/openvswitch/flow_table.c   |   1 +
> >>  net/openvswitch/hw_offload.c   | 245 +++++++++++++++++++++++++++++++++++++++++
> >>  net/openvswitch/hw_offload.h   |  22 ++++
> >>  net/openvswitch/vport-netdev.c |   3 +
> >>  net/openvswitch/vport.h        |   2 +
> >>  8 files changed, 311 insertions(+), 1 deletion(-)
> >>  create mode 100644 net/openvswitch/hw_offload.c
> >>  create mode 100644 net/openvswitch/hw_offload.h
> >>
> >>diff --git a/net/openvswitch/Makefile b/net/openvswitch/Makefile
> >>index 3591cb5..5152437 100644
> >>--- a/net/openvswitch/Makefile
> >>+++ b/net/openvswitch/Makefile
> >>@@ -13,7 +13,8 @@ openvswitch-y := \
> >>  	flow_table.o \
> >>  	vport.o \
> >>  	vport-internal_dev.o \
> >>-	vport-netdev.o
> >>+	vport-netdev.o \
> >>+	hw_offload.o
> >>
> >>  ifneq ($(CONFIG_OPENVSWITCH_VXLAN),)
> >>  openvswitch-y += vport-vxlan.o
> >>diff --git a/net/openvswitch/datapath.c b/net/openvswitch/datapath.c
> >>index 75bb07f..3e43e1d 100644
> >>--- a/net/openvswitch/datapath.c
> >>+++ b/net/openvswitch/datapath.c
> >>@@ -57,6 +57,7 @@
> >>  #include "flow_netlink.h"
> >>  #include "vport-internal_dev.h"
> >>  #include "vport-netdev.h"
> >>+#include "hw_offload.h"
> >>
> >>  int ovs_net_id __read_mostly;
> >>
> >>@@ -864,6 +865,9 @@ static int ovs_flow_cmd_new(struct sk_buff *skb, struct genl_info *info)
> >>  			acts = NULL;
> >>  			goto err_unlock_ovs;
> >>  		}
> >>+		error = ovs_hw_flow_insert(dp, new_flow);
> >>+		if (error)
> >>+			pr_warn("failed to insert flow into hw\n");
> >
> >This is really close to silently failing. I think we need to
> >hard fail here somehow and push it back to userspace as part of
> >the reply and ovs_notify.
> 
> Yes, I agree. My plan was to handle this in ovs hw/sw/both netlink attr
> implementation.

FWIW I agree that handling it in that way makes sense.
In particular I think that "both", where the datapath is allowed
to fall back to software is a useful mode to have (its the current
implementation, right?). But that it is also good to allow user-space
more control.

> >Otherwise I don't know how to manage the hardware correctly. Consider
> >the hardware table is full. In this case user space will continue to
> >add rules and they will be silently discarded. Similarly if user space
> >adds a flow/action that can not be supported by the hardware it will
> >be silently ignored.
> >
> >Even if we do careful accounting on resources in user space we could
> >still get an ENOMEM error from sw_flow_action_create.
> >
> >Same comment for the other hw commands flush/remove.
> >
> >>  		if (unlikely(reply)) {
> >>  			error = ovs_flow_cmd_fill_info(new_flow,
> >>@@ -896,10 +900,18 @@ static int ovs_flow_cmd_new(struct sk_buff *skb, struct genl_info *info)
> >>  				goto err_unlock_ovs;
> >>  			}
> >>  		}
> >
> >
> >[...]
> >
> >
> >Thanks,
> >John
> >
> >-- 
> >John Fastabend         Intel Corporation
> _______________________________________________
> dev mailing list
> dev@...nvswitch.org
> http://openvswitch.org/mailman/listinfo/dev
> 
--
To unsubscribe from this list: send the line "unsubscribe netdev" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives