From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from mail-wm0-f44.google.com (mail-wm0-f44.google.com [74.125.82.44]) by dpdk.org (Postfix) with ESMTP id 1D6881CC8F for ; Fri, 6 Apr 2018 22:33:21 +0200 (CEST) Received: by mail-wm0-f44.google.com with SMTP id x82so5408440wmg.1 for ; Fri, 06 Apr 2018 13:33:21 -0700 (PDT) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=6wind-com.20150623.gappssmtp.com; s=20150623; h=date:from:to:cc:subject:message-id:references:mime-version :content-disposition:in-reply-to; bh=d6lfVHqHVVpPueGUTY2lFXOUn4ZI1Qa1CAmqE+y357c=; b=KeMRKCvdQW6PudbiFvb3CpTOYijukGtJzqmzKWfkLmnogGXwKUmgpuMAquiCmW+c/q 1voE012/+uP5g5oiJWceuDef4jCdiNTpdpBz+CbMtTDdVM6uB/FiK9r4L0f3RhENegXt QODKvVmd0ohud/SegKaHAD1q2M4g2aELgN8kBDGq3bjRoasE28Xz0tfKe9S6oXDun7Qq Qx9PYKbtyHLwrmS320PaFhNvByoe0Hxm3ZNohFp9V43dz+S0W0EefDVXN8HMtIUpPCih kHFIYWKrs2YIvVDfChAf5BC86yyjSU3M4NmBNch+jmmQrAz2VFOrm+lNHMm+BrzP4G8C 4qTA== X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20161025; h=x-gm-message-state:date:from:to:cc:subject:message-id:references :mime-version:content-disposition:in-reply-to; bh=d6lfVHqHVVpPueGUTY2lFXOUn4ZI1Qa1CAmqE+y357c=; b=BUrZL6gyRTbS9nefU1na07ecJ+9X/Sh3Eom/aVCLGLwEjNDmg/JZ32NVYcbh6JtV4d 0+WQfsiFa5fJFe4/BCsioquK2TXbA+dtjbLy/dnQfvpOBtmr12gKrhZYENOAEXNHwQFO Ak66EtnG+H18K4/NArbwFrSi193ZPgBINlJZikewC8pwX4Rpw878BGC6q0jmTsbd85D1 TYp2zCuUoWjQVMfuA9BEvp3baAj0+ho0iMVY/oz1ySloeVvZ8BAaa2H/SynrPNCVAaEr y0ehJhXKfP7XuLvvq8POEcMr/ojHvi5WDD9H8irUrrNlEftdJxKYaC8tu9BdciBGmAoQ JUfQ== X-Gm-Message-State: ALQs6tCKsgUsGSKXde6UtqQU4p3/u/HViAvi0MfPSzEN1G7n38ek2jOw vn26ByaynRdMZGiqfQeXhtSYiA== X-Google-Smtp-Source: AIpwx49zVnus6xGLfPT8BE/xX/HzYQ5zXGFDATysACXAwKfqvhpSo9UPqxTjcwS1sH++vYjhZt5J2A== X-Received: by 10.28.167.80 with SMTP id q77mr2748986wme.98.1523046414997; Fri, 06 Apr 2018 13:26:54 -0700 (PDT) Received: from 6wind.com (host.78.145.23.62.rev.coltfrance.com. [62.23.145.78]) by smtp.gmail.com with ESMTPSA id 25sm3666357wrv.51.2018.04.06.13.26.54 (version=TLS1_2 cipher=ECDHE-RSA-AES128-GCM-SHA256 bits=128/128); Fri, 06 Apr 2018 13:26:54 -0700 (PDT) Date: Fri, 6 Apr 2018 22:26:41 +0200 From: Adrien Mazarguil To: Declan Doherty Cc: dev@dpdk.org Message-ID: <20180406202641.GS4957@6wind.com> References: <1523017443-12414-1-git-send-email-declan.doherty@intel.com> <1523017443-12414-3-git-send-email-declan.doherty@intel.com> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: <1523017443-12414-3-git-send-email-declan.doherty@intel.com> Subject: Re: [dpdk-dev] [PATCH v3 2/4] ethdev: Add tunnel encap/decap actions X-BeenThere: dev@dpdk.org X-Mailman-Version: 2.1.15 Precedence: list List-Id: DPDK patches and discussions List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Fri, 06 Apr 2018 20:33:21 -0000 On Fri, Apr 06, 2018 at 01:24:01PM +0100, Declan Doherty wrote: > Add new flow action types and associated action data structures to > support the encapsulation and decapsulation of the virtual tunnel > endpoints. > > The RTE_FLOW_ACTION_TYPE_TUNNEL_ENCAP action will cause the matching > flow to be encapsulated in the virtual tunnel endpoint overlay > defined in the tunnel_encap action data. > > The RTE_FLOW_ACTION_TYPE_TUNNEL_DECAP action will cause all virtual > tunnel endpoint overlays up to and including the first instance of > the flow item type defined in the tunnel_decap action data for the > matching flows. > > Signed-off-by: Declan Doherty This generic approach looks flexible enough to cover the use cases that immediately come to mind (VLAN, VXLAN), its design is sound. However, while I'm aware it's not a concern at this point, it won't be able to deal with stateful tunnel or encapsulation types (e.g. IPsec or TCP) which will require additional meta data or some run-time assistance from the application. Eventually for more complex use cases, dedicated encap/decap actions will have to appear, so the issue I wanted to raise before going further is this: Going generic inevitably trades some of the usability; flat structures dedicated to VXLAN encap/decap with only the needed info to get the job done would likely be easier to implement in PMDs and use in applications. Any number of such actions can be added to rte_flow without ABI impact. If VXLAN is the only use case at this point, my suggestion would be to go with simpler RTE_FLOW_ACTION_TYPE_VXLAN_(ENCAP|DECAP) actions, with fixed L2/L3/L4/L5 header definitions to prepend according to RFC 7348. Now we can start with the generic approach, see how it fares and add dedicated encap/decap later as needed. More comments below. > --- > doc/guides/prog_guide/rte_flow.rst | 77 ++++++++++++++++++++++++++++++++-- > lib/librte_ether/rte_flow.h | 84 ++++++++++++++++++++++++++++++++++++-- > 2 files changed, 155 insertions(+), 6 deletions(-) > > diff --git a/doc/guides/prog_guide/rte_flow.rst b/doc/guides/prog_guide/rte_flow.rst > index fd33d19..106fb93 100644 > --- a/doc/guides/prog_guide/rte_flow.rst > +++ b/doc/guides/prog_guide/rte_flow.rst > @@ -997,9 +997,11 @@ Actions > > Each possible action is represented by a type. Some have associated > configuration structures. Several actions combined in a list can be assigned > -to a flow rule. That list is not ordered. > +to a flow rule. That list is not ordered, with the exception of actions which > +modify the packet itself, these packet modification actions must be specified > +in the explicit order in which they are to be executed. > > -They fall in three categories: > +They fall in four categories: > > - Terminating actions (such as QUEUE, DROP, RSS, PF, VF) that prevent > processing matched packets by subsequent flow rules, unless overridden > @@ -1008,8 +1010,11 @@ They fall in three categories: > - Non-terminating actions (PASSTHRU, DUP) that leave matched packets up for > additional processing by subsequent flow rules. > > +- Non-terminating meta actions that do not affect the fate of packets but result > + in modification of the packet itself (SECURITY, TUNNEL_ENCAP, TUNNEL_DECAP). > + > - Other non-terminating meta actions that do not affect the fate of packets > - (END, VOID, MARK, FLAG, COUNT, SECURITY). > + (END, VOID, MARK, FLAG, COUNT). The above changes are not necessary anymore [1][2]. [1] "ethdev: clarify flow API pattern items and actions" https://dpdk.org/ml/archives/dev/2018-April/095776.html [2] "ethdev: alter behavior of flow API actions" https://dpdk.org/ml/archives/dev/2018-April/095779.html > When several actions are combined in a flow rule, they should all have > different types (e.g. dropping a packet twice is not possible). > @@ -1486,6 +1491,72 @@ fields in the pattern items. > | 1 | END | > +-------+----------+ > > + Nit: titles in this file are separated by a single empty line. > +Action: ``TUNNEL_ENCAP`` > +^^^^^^^^^^^^^^^^^^^^^^ > + > +Performs an encapsulation action by encapsulating the flows matched by the > +pattern items according to the network overlay defined in the > +``rte_flow_action_tunnel_encap`` pattern items. > + > +This action modifies the payload of matched flows. The pattern items specified > +in the ``rte_flow_action_tunnel_encap`` action structure must defined a valid > +set of overlay headers, from the Ethernet header up to the overlay header. The > +pattern must be terminated with the RTE_FLOW_ITEM_TYPE_END item type. Regarding the use of a pattern list, if you consider PMDs are already iterating on a list of actions when encountering RTE_FLOW_ACTION_TYPE_TUNNEL_ENCAP, it adds yet another inner loop. How about making each encountered RTE_FLOW_ACTION_TYPE_TUNNEL_ENCAP provide exactly one item instead (in encap, i.e. reverse order)? In which case perhaps "GENERIC" would be a better fit than "TUNNEL". > + > +- Non-terminating by default. There's no such property anymore [2]. > + > +.. _table_rte_flow_action_tunnel_encap: > + > +.. table:: TUNNEL_ENCAP > + > + +-------------+---------------------------------------------+ > + | Field | Value | > + +=============+=============================================+ > + | ``pattern`` | Virtual tunnel end-point pattern definition | > + +-------------+---------------------------------------------+ > + > + > +.. _table_rte_flow_action_tunnel_encap_example: > + > +.. table:: IPv4 VxLAN flow pattern example. VxLAN => VXLAN > + > + +-------+--------------------------+------------+ > + | Index | Flow Item Type | Flow Item | > + +=======+==========================+============+ > + | 0 | RTE_FLOW_ITEM_TYPE_ETH | eth item | > + +-------+--------------------------+------------+ > + | 1 | RTE_FLOW_ITEM_TYPE_IPV4 | ipv4 item | > + +-------+--------------------------+------------+ > + | 2 | RTE_FLOW_ITEM_TYPE_UDP | udp item | > + +-------+--------------------------+------------+ > + | 3 | RTE_FLOW_ITEM_TYPE_VXLAN | vxlan item | > + +-------+--------------------------+------------+ > + | 4 | RTE_FLOW_ITEM_TYPE_END | NULL | > + +-------+--------------------------+------------+ One possible issue is that it relies on objects normally found on the pattern side of flow rules. Those are supposed to match something, they are not intended for packet header generation. While their "spec" and "mask" fields might make sense in this context, the "last" field is odd. You must define them without leaving anything open for interpretation by PMDs and users alike. Defining things as "undefined" is fine as long as it's covered. > + > + Nit: only one empty line necessary here. > +Action: ``TUNNEL_DECAP`` > +^^^^^^^^^^^^^^^^^^^^^^ > + > +Performs a decapsulation action by stripping all headers of the virtual tunnel > +end-point overlay up to the header defined by the flow item type of flows > +matched by the pattern items. Not necessarily, for instance if one guarantees that flowing traffic only consists of decap'able packets. You must avoid mandatory dependencies between patterns and actions since they are normally unrelated. What you can document on the other hand is that the behavior is undefined when processing traffic on which the action can't be applied. This is how RSS level is documented [3]. [3] https://dpdk.org/ml/archives/dev/2018-April/095783.html > + > +This action modifies the payload of matched flows. The flow item type specified > +in the ``rte_flow_action_tunnel_decap`` action structure must defined a valid > +set of overlay header type. > + > +- Non-terminating by default. See [2]. > + > +.. _table_rte_flow_action_tunnel_decap: > + > + +---------------+----------------------------------------------+ > + | Field | Value | > + +===============+==============================================+ > + | ``item type`` | Item type of tunnel end-point to decapsulate | > + +---------------+----------------------------------------------+ "item type" should be the exact name used in the structure. > + > Negative types > ~~~~~~~~~~~~~~ > > diff --git a/lib/librte_ether/rte_flow.h b/lib/librte_ether/rte_flow.h > index 7d1f89d..6d94423 100644 > --- a/lib/librte_ether/rte_flow.h > +++ b/lib/librte_ether/rte_flow.h > @@ -854,14 +854,17 @@ struct rte_flow_item { > const void *mask; /**< Bit-mask applied to spec and last. */ > }; > > + Unnecessary empty line. > /** > * Action types. > * > * Each possible action is represented by a type. Some have associated > * configuration structures. Several actions combined in a list can be > - * affected to a flow rule. That list is not ordered. > + * affected to a flow rule. That list is not ordered, with the exception of > + * actions which modify the packet itself, these packet modification actions > + * must be specified in the explicit order in which they are to be executed. > * > - * They fall in three categories: > + * They fall in four categories: > * > * - Terminating actions (such as QUEUE, DROP, RSS, PF, VF) that prevent > * processing matched packets by subsequent flow rules, unless overridden > @@ -870,6 +873,10 @@ struct rte_flow_item { > * - Non terminating actions (PASSTHRU, DUP) that leave matched packets up > * for additional processing by subsequent flow rules. > * > + * - Non terminating meta actions that do not affect the fate of > + * packets but result in modification of the packet itself (SECURITY, > + * TUNNEL_ENCAP, TUNNEL_DECAP). > + * Same comment as above [1][2]. > * - Other non terminating meta actions that do not affect the fate of > * packets (END, VOID, MARK, FLAG, COUNT). > * > @@ -1022,7 +1029,42 @@ enum rte_flow_action_type { > * > * See struct rte_flow_action_group_count. > */ > - RTE_FLOW_ACTION_TYPE_GROUP_COUNT > + RTE_FLOW_ACTION_TYPE_GROUP_COUNT, An empty line would have been needed here (if we agree about no more GROUP_COUNT.) > + /** > + * Encapsulate flow with tunnel defined in > + * rte_flow_action_tunnel_encap structure. > + * > + * See struct rte_flow_action_tunnel_encap. > + */ > + RTE_FLOW_ACTION_TYPE_TUNNEL_ENCAP, > + > + /** > + * Decapsulate all the headers of the tunnel > + * > + * See struct rte_flow_action_tunnel_decap. > + */ > + RTE_FLOW_ACTION_TYPE_TUNNEL_DECAP, > + > + /** > + * Redirects packets to the logical group of the current device. > + * > + * In a logical hierarchy of groups, which can be used to represent a > + * physical of logical chaining of flow tables, this action allows the > + * terminating action to be a logical group of the same device. > + * > + * See struct rte_flow_action_group. > + */ > + RTE_FLOW_ACTION_TYPE_GROUP, > + > + /** > + * [META] > + * > + * Set specific metadata field associated with packet which is then > + * available to further pipeline stages. > + * > + * See struct rte_flow_action_metadata. > + */ > + RTE_FLOW_ACTION_TYPE_METADATA These two actions should be part of the next patch, I won't comment them here. > }; > > /** > @@ -1173,6 +1215,42 @@ struct rte_flow_action_group_count { > }; > > /** > + * RTE_FLOW_ACTION_TYPE_TUNNEL_ENCAP > + * > + * Virtual tunnel end-point encapsulation action data. > + * > + * Non-terminating action by default. See [2]. > + */ > +struct rte_flow_action_tunnel_encap { > + struct rte_flow_action_item { > + enum rte_flow_item_type type; > + /**< Flow item type. */ > + const void *item; > + /**< Flow item definition which points to the data of > + * corresponding rte_flow_item_type. > + */ I see it's a new action type, albeit a bit confusing (there is no RTE_FLOW_ACTION_TYPE_ITEM). I suggest the standard pattern item type since you're going with enum rte_flow_item_type anyway. Keep in mind you need some kind of mask to tell what fields are relevant. An application might otherwise want to encap with unsupported properties (e.g. specific IPv4 ToS field and whatnot). How about a single "struct rte_flow_pattern_item item", neither const and neither a pointer. It's generic enough, enclosed spec/last/mask pointers take care of the specifics. You just need to define what's supposed to happen when "last" is set. > + } *pattern; > + /**< > + * Tunnel pattern specification (list terminated by the END pattern > + * item). > + */ As previously suggested, how about a single item per encap? > +}; > + > +/** > + * RTE_FLOW_ACTION_TYP_TUNNEL_DECAP > + * > + * Virtual tunnel end-point decapsulation action data. > + * > + * Non-terminating action by default. > + */ > +struct rte_flow_action_tunnel_decap { > + enum rte_flow_item_type type; > + /**< > + * Flow item type of virtual tunnel end-point to be decapsulated > + */ > +}; Note that contrary to ENCAP, DECAP wouldn't necessarily need repeated actions to peel each layer off. The current definition is fine. > + > +/** > * Definition of a single action. > * > * A list of actions is terminated by a END action. > -- > 2.7.4 > -- Adrien Mazarguil 6WIND