Date:	Thu, 4 Sep 2014 18:31:35 +0900
From:	Simon Horman <simon.horman@...ronome.com>
To:	Jiri Pirko <jiri@...nulli.us>
Cc:	netdev@...r.kernel.org, davem@...emloft.net, nhorman@...driver.com,
	andy@...yhouse.net, tgraf@...g.ch, dborkman@...hat.com,
	ogerlitz@...lanox.com, jesse@...ira.com, pshelar@...ira.com,
	azhou@...ira.com, ben@...adent.org.uk, stephen@...workplumber.org,
	jeffrey.t.kirsher@...el.com, vyasevic@...hat.com,
	xiyou.wangcong@...il.com, john.r.fastabend@...el.com,
	edumazet@...gle.com, jhs@...atatu.com, sfeldma@...ulusnetworks.com,
	f.fainelli@...il.com, roopa@...ulusnetworks.com,
	linville@...driver.com, dev@...nvswitch.org, jasowang@...hat.com,
	ebiederm@...ssion.com, nicolas.dichtel@...nd.com,
	ryazanov.s.a@...il.com, buytenh@...tstofly.org,
	aviadr@...lanox.com, nbd@...nwrt.org, alexei.starovoitov@...il.com,
	Neil.Jerram@...aswitch.com, ronye@...lanox.com
Subject: Re: [patch net-next RFC 10/12] openvswitch: add support for datapath
 hardware offload

On Thu, Sep 04, 2014 at 11:24:58AM +0200, Jiri Pirko wrote:
> Thu, Sep 04, 2014 at 11:04:49AM CEST, simon.horman@...ronome.com wrote:
> >Hi Jiri,
> >
> >sorry for coming a little late to the party.
> >I'm very happy to see work in this area.
> >
> >On Thu, Aug 21, 2014 at 06:19:03PM +0200, 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>
> >> ---
> >>  include/linux/sw_flow.h        |  14 +++
> >>  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   | 235 +++++++++++++++++++++++++++++++++++++++++
> >>  net/openvswitch/hw_offload.h   |  22 ++++
> >>  net/openvswitch/vport-netdev.c |   3 +
> >>  net/openvswitch/vport.h        |   2 +
> >>  9 files changed, 315 insertions(+), 1 deletion(-)
> >>  create mode 100644 net/openvswitch/hw_offload.c
> >>  create mode 100644 net/openvswitch/hw_offload.h
> >> 
> >> diff --git a/include/linux/sw_flow.h b/include/linux/sw_flow.h
> >> index b622fde..079d065 100644
> >> --- a/include/linux/sw_flow.h
> >> +++ b/include/linux/sw_flow.h
> >> @@ -80,7 +80,21 @@ struct sw_flow_mask {
> >>  	struct sw_flow_key key;
> >>  };
> >>  
> >> +enum sw_flow_action_type {
> >> +	SW_FLOW_ACTION_TYPE_OUTPUT,
> >> +	SW_FLOW_ACTION_TYPE_VLAN_PUSH,
> >> +	SW_FLOW_ACTION_TYPE_VLAN_POP,
> >> +};
> >> +
> >>  struct sw_flow_action {
> >> +	enum sw_flow_action_type type;
> >> +	union {
> >> +		struct net_device *output_dev;
> >> +		struct {
> >> +			__be16 vlan_proto;
> >> +			u16 vlan_tci;
> >> +		} vlan;
> >> +	};
> >>  };
> >
> >I think it would be nice to use an more end-to-end approach for actions.
> >
> >The way things that are set up in this patch there are thee actions
> >that are allowed. This is in contrast to the Open vSwtich datapath which
> >supports a much later superset of actions. And occasionally aquires
> >support for new ones (e.g. my recent work on MPLS push, pop and set
> >actions).
> >
> >I think it would be nicer if arbitrary actions could be passed
> >down to the swdev API and for the underlying driver to return
> >an appropriate error code if an action is unsupported. Rather than having
> >datapath or swdev API code filtering actions.
> >
> >That is allow end-to-end communication between the datapath and the driver
> >rather than having a filter in the middle.
> 
> There is not an intention to filter this on swdev layer. Only couple od
> action types is supported atm. But nothing prevents to add more action
> types in the set. What you describe (driver receiving all and provides
> feedback if it can handle or not) is the goal.

Ok, good to know.

I think it would be ideal if the intermediate layers didn't need
to be taught about every new action.

> >[snip]
> >
> >In relation to ports and datapaths it seems to me that the API that
> >has been developed accommodates a model where a port may belong to a
> >switch device; that this topology is fixed before any API calls are made
> >and that all all ports belonging to the same switch belong to the same
> >datapath.
> >
> >This makes sense in the case of hardware that looks a lot like a switch.
> >But I think that other scenarios are possible. For example hardware that
> >is able to handle the same abstractions handled by the datapath: datapaths
> >may be created or destroyed; vports may be added and removed from datapaths.
> 
> Hmm. Maybe there can be added something like "datapath id" that would
> couple multiple ports?

I can see how something like that could work.

> >So one might have a piece of hardware that is configured with more than one
> >datapath configured and its different ports belong to it might be
> >associated with different datapaths.
> 
> But that could propagate 2 switch ids (datapath ids), one for each dp.

Right, I think so too. But I think it would also provide a way
to configure things dynamically, if the hardware supports such
configuration.

> I believe that switchid should say what ports are part of one silicon. I
> can imagine adding port-dp manipulation api later and that would come
> with datapath id notion.

Right, it may well make sense for this to be a separate API.
Sorry for overlooking that in my previous email.

> >Or we might have hardware that is able to offload a tunnel vport.
> >
> >In short I am thinking in terms of API callbacks to manipulate datapaths
> >and vports. Although I have not thought about it in detail I believe
> >that the current model you have implemented using such a scheme because
> >the scheme I am suggesting maps to that of the datapath and you have
> >implemented your model there.
--
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

Powered by blists - more mailing lists