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

[John Fastabend <john.fastabend-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org>]

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-rHqAuBHg3fBzbRFIqnYvSA@public.gmane.org>
> ---
>   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.

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