From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from EUR01-HE1-obe.outbound.protection.outlook.com (mail-he1eur01on0064.outbound.protection.outlook.com [104.47.0.64]) by dpdk.org (Postfix) with ESMTP id 3F9111B6FE for ; Tue, 10 Apr 2018 13:06:46 +0200 (CEST) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=Mellanox.com; s=selector1; h=From:Date:Subject:Message-ID:Content-Type:MIME-Version; bh=GHKoQkqwCNMk9h1fZr3nbV9d1wU5B57qKx5iVNVDouw=; b=oETAVLQeVEpVqigYeDTj0Ee9rLRFtDNrseAGW9AlPZgOEQRqluCqhIxxpdEEGzsXtYiN/0e/YLgWlazTr9sSJ77r8qQ9iGGykvY9lQkMjaLJTdHpRQDqZ5rWpfUPsfGF/hmrgZmNmOso9JNkIYRTB0/apoygeFL7b7XK12ov4HQ= Received: from DB7PR05MB4426.eurprd05.prod.outlook.com (52.134.109.15) by DB7PR05MB4203.eurprd05.prod.outlook.com (52.134.107.160) with Microsoft SMTP Server (version=TLS1_2, cipher=TLS_ECDHE_RSA_WITH_AES_256_CBC_SHA384_P256) id 15.20.653.12; Tue, 10 Apr 2018 11:06:43 +0000 Received: from DB7PR05MB4426.eurprd05.prod.outlook.com ([fe80::808d:386e:26f3:859f]) by DB7PR05MB4426.eurprd05.prod.outlook.com ([fe80::808d:386e:26f3:859f%13]) with mapi id 15.20.0653.014; Tue, 10 Apr 2018 11:06:43 +0000 From: Shahaf Shuler To: Adrien Mazarguil , Mohammad Abdul Awal CC: Declan Doherty , "dev@dpdk.org" , Alex Rosenbaum Thread-Topic: [dpdk-dev] [PATCH v3 2/4] ethdev: Add tunnel encap/decap actions Thread-Index: AQHTzaJAd7/G4mqq/0CqlnoYqkiTz6P0L+CAgARvcICAATBfAIAACwfg Date: Tue, 10 Apr 2018 11:06:43 +0000 Message-ID: References: <1523017443-12414-1-git-send-email-declan.doherty@intel.com> <1523017443-12414-3-git-send-email-declan.doherty@intel.com> <20180406202641.GS4957@6wind.com> <20180410101958.GF4957@6wind.com> In-Reply-To: <20180410101958.GF4957@6wind.com> Accept-Language: en-US Content-Language: en-US X-MS-Has-Attach: X-MS-TNEF-Correlator: x-originating-ip: [31.154.10.107] x-ms-publictraffictype: Email x-microsoft-exchange-diagnostics: 1; DB7PR05MB4203; 7:7IXatF4ob+oevgQpiDpteRWAOli9nfPw8hQBsl2zu6ltakMVFSdrKQILXxCedgFYbSy/w5T6fOJonYgK/aHH+LKp19HoLa9ySzJlmXXocTriSOluqsI6modk3UjwtPZXv0i1HjK77/J4+E5dspNxCr81C5I+DsXTYByUulAMTpCdIt6jpUOW9Pfiahi3nUiRMg+Y2m3zqkNJ6gGLrNyxITD4JRZR7J0oaOAgQePrHsq4VCERA/yJuWA7o1O0EfFY x-ms-exchange-antispam-srfa-diagnostics: SOS; x-ms-office365-filtering-ht: Tenant x-microsoft-antispam: UriScan:; BCL:0; PCL:0; RULEID:(7020095)(4652020)(48565401081)(5600026)(3008032)(4534165)(4627221)(201703031133081)(201702281549075)(2017052603328)(7153060)(7193020); SRVR:DB7PR05MB4203; x-ms-traffictypediagnostic: DB7PR05MB4203: x-ld-processed: a652971c-7d2e-4d9b-a6a4-d149256f461b,ExtAddr x-microsoft-antispam-prvs: x-exchange-antispam-report-test: UriScan:(278428928389397)(192374486261705)(211171220733660)(228905959029699)(17755550239193); x-exchange-antispam-report-cfa-test: BCL:0; PCL:0; RULEID:(8211001083)(6040522)(2401047)(5005006)(8121501046)(93006095)(93001095)(3231221)(944501327)(52105095)(3002001)(10201501046)(6055026)(6041310)(20161123560045)(20161123564045)(20161123558120)(20161123562045)(201703131423095)(201702281528075)(20161123555045)(201703061421075)(201703061406153)(6072148)(201708071742011); SRVR:DB7PR05MB4203; BCL:0; PCL:0; RULEID:; SRVR:DB7PR05MB4203; x-forefront-prvs: 0638FD5066 x-forefront-antispam-report: SFV:NSPM; SFS:(10009020)(39380400002)(396003)(376002)(366004)(39860400002)(346002)(199004)(189003)(43544003)(26005)(7696005)(76176011)(305945005)(6116002)(6346003)(54906003)(102836004)(3846002)(99286004)(106356001)(6506007)(110136005)(186003)(86362001)(107886003)(316002)(5660300001)(6246003)(478600001)(7736002)(74316002)(33656002)(561944003)(66066001)(4326008)(2906002)(53546011)(25786009)(81156014)(8936002)(9686003)(59450400001)(3280700002)(53936002)(105586002)(5250100002)(486006)(3660700001)(81166006)(55016002)(93886005)(6436002)(8676002)(11346002)(14454004)(2900100001)(476003)(97736004)(446003)(229853002)(68736007); DIR:OUT; SFP:1101; SCL:1; SRVR:DB7PR05MB4203; H:DB7PR05MB4426.eurprd05.prod.outlook.com; FPR:; SPF:None; LANG:en; PTR:InfoNoRecords; A:1; MX:1; received-spf: None (protection.outlook.com: mellanox.com does not designate permitted sender hosts) authentication-results: spf=none (sender IP is ) smtp.mailfrom=shahafs@mellanox.com; x-microsoft-antispam-message-info: tkHewmXZngErQA2BKw2W+26CFXy3A+ZB1AoR4/vTF7YbDdweyGawPTcDIMMpvPz160j68D4x4z1VGAPV/mKS8QCsNGqO2rP9oPxp3Nryt/9kbRPWFi7h4YnvxHZdGLjMHwOZ5C6wzlRPmQ6GudZsmxIBkcQ419W2bH214swPhs0v9+blxLI7EfSYUNWoNRp2l/g1DTSM+268KnlsW2kNY0ifGglYr2kNcXI+YLZZDgZ3tTyaIseA3EAvPhfQLApiiPYNsnq/9BtOxNoib5HbGOd6Fp825AA6Np68Ob5RJSMck3yLsUT2MuSy+WvCxHeQlhHxMaI+/VwIrj8zc+r8Gx/NGhv4sD9Dy0h6eVQjQocYern6kk879c0VWm8rg+96L25WWPscybwqfDb9x+lgBnpJZ0+H7kXXoqOSLQBMvNA= spamdiagnosticoutput: 1:99 spamdiagnosticmetadata: NSPM Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: quoted-printable MIME-Version: 1.0 X-MS-Office365-Filtering-Correlation-Id: 5637ce53-ea80-42c2-8c88-08d59ed324e0 X-OriginatorOrg: Mellanox.com X-MS-Exchange-CrossTenant-Network-Message-Id: 5637ce53-ea80-42c2-8c88-08d59ed324e0 X-MS-Exchange-CrossTenant-originalarrivaltime: 10 Apr 2018 11:06:43.5527 (UTC) X-MS-Exchange-CrossTenant-fromentityheader: Hosted X-MS-Exchange-CrossTenant-id: a652971c-7d2e-4d9b-a6a4-d149256f461b X-MS-Exchange-Transport-CrossTenantHeadersStamped: DB7PR05MB4203 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: Tue, 10 Apr 2018 11:06:46 -0000 Hi, Adding small comment on top of Adrien's Tuesday, April 10, 2018 1:20 PM, Adrien Mazarguil: > On Mon, Apr 09, 2018 at 05:10:35PM +0100, Mohammad Abdul Awal wrote: > > On 06/04/2018 21:26, Adrien Mazarguil wrote: > > > 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 furt= her 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_f= low > 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. > > We can go this way and this will increase the action for more and more > > tunneling protocols being added. Current proposal is already a generic > > approach which specifies as a tunnel for all the tunneling protocols. >=20 > Right, on the other hand there are not that many standard encapsulations > offloaded by existing devices. rte_flow could easily handle dedicated act= ions > for each of them without problem. >=20 > My point is that many of those (will eventually) have their own quirks to > manage when doing encap/decap, it's not just a matter of prepending or > removing a bunch of header definitions, otherwise we could as well let > applications simply provide an arbitrary buffer to prepend. >=20 > Consider that the "generic" part is already built into rte_flow as the wa= y > patterns and action are handled. Adding another generic layer on top of t= hat > could make things more inconvenient than necessary to applications (my > main concern). >=20 > You'd need another layer of validation/error reporting machinery to prope= rly > let applications know they cannot encap VXLAN on top of TCP on top of > QinQinQinQinQ for instance. Either a single bounded encapsulation definit= ion > or a combination at the action list level is needed to avoid that. >=20 > > > Now we can start with the generic approach, see how it fares and add > > > dedicated encap/decap later as needed. > > > > > > More comments below. > > > > > +Action: ``TUNNEL_ENCAP`` > > > > +^^^^^^^^^^^^^^^^^^^^^^ The ENCAP/DECAP doesn't have to be in the context of tunnel. For example - let's take GRE - application may want to decap the GRE and en= cap it with L2. The L2 encapsulation is not related to any tunnel.=20 Same for the other direction - VM sends Eth frame, and we want to decap the= Eth and encap with GRE. I think those action should be free from the tunnel association and just pr= ovide flow items we want to encap/decap or in a more generic way offset to = the packet headers and buffer to encap (not sure how many devices supports = that, may be overkill at this point).=20 > > > > + > > > > +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. > > We understand that it is implementation specifics. If we do not go for > > another inner loop, all the bundling need to be handled in the same > > function, which seems more clumsy to me. This also breaks the tunnel > > endpoint concept. > > > > > > How about making each encountered > RTE_FLOW_ACTION_TYPE_TUNNEL_ENCAP > > > provide exactly one item instead (in encap, i.e. reverse order)? > > Again, if we have tunnel action, security action, and other actions, > > all the processing and tracking need to be done in one function. Now > > we will need ETH_ENCAP/DECAP, UDP_ENCAP/DECAP, > NVGRE_ENCAP/DECAP, etc. >=20 > Well, the number of DECAP actions doesn't need to perfectly reflect that = of > ENCAP since it implies all preceding layers. No problem with that. >=20 > Regarding multiple dedicated actions, my suggestion was for a single gene= ric > one as in this patch, but each instance on the ENCAP side would deal with= a > single protocol layer, instead of having a single ENCAP action with multi= ple > inner layers (and thus an inner loop). >=20 > PMDs also gain the ability to precisely report which encap step fails by = making > rte_flow_error point to the problematic object to ease debugging of flow > rules on the application side. >=20 > Why would that break the tunnel idea and more importantly, how would it > prevent PMD developers from splitting their processing into multiple > functions? >=20 > > > > > > In which case perhaps "GENERIC" would be a better fit than "TUNNEL". > > > > > > > > + > > > > + +-------+--------------------------+------------+ > > > > + | Index | Flow Item Type | Flow Item | > > > > + +=3D=3D=3D=3D=3D=3D=3D+=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D= =3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D+=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D= =3D+ > > > > + | 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. > > Please note that the "void *item" in the > > "rte_flow_action_tunnel_encap.pattern" points to the data structure > > defined for the corresponding rte_flow_item_type instead of a > > rte_flow_item structure. As an example, for the rte_flow_item_eth type, > the "void *item" > > will point to a "struct rte_flow_item_eth" instance. Thats why we have > > defined struct rte_flow_action_item inside struct > > rte_flow_action_tunnel_encap. So, no question of spec, mask, last > anymore. >=20 > Right, I noticed that after commenting its structure definition below. >=20 > I think I won't be the only one confused by this approach, also because a > mask is needed in addition to a specification structure, otherwise how do= you > plan to tell what fields are relevant in application-provided protocol he= aders? >=20 > An application might set unusual IPv4/UDP/VXLAN fields and expect them to > be part of the encapsulated traffic. Without a mask, a PMD must take > headers verbatim, and I don't think many devices are ready for that yet. >=20 > Hence my other suggestion: defining inflexible $PROTOCOL_(ENCAP|DECAP) > actions that do not allow more than what's defined by official RFCs for > $PROTOCOL. >=20 > > > > > + */ > > > > +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 IP= v4 > 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. > > Please see the comment above regarding this field. >=20 > Point still stands, if you need to distinguish spec and mask, a more comp= lete > structure is needed. Instead of adding a new (confusing) type, you should > use rte_flow_item and define what happens when "last" is set. >=20 > You should define it as reserved for now, any non-NULL value is an error.= This > field might also be useful later. >=20 > > > > > +}; > > > > + > > > > +/** > > > > + * 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 fi= ne. > > To clarify, the the decap is upto the PMD to remove all the header for > > a specified type. For example, for > > > > rte_flow_item_type type=3DRTE_FLOW_ITEM_TYPE_VXLAN, the PMD will > peel off (ETH, IPV4, UDP, VXLAN) header all together. >=20 > Yep, that's fine, whether we use multiple actions or a single one doing > multiple things, a single DECAP can peel them off all at once :) >=20 > > > > > > > + > > > > +/** > > > > * Definition of a single action. > > > > * > > > > * A list of actions is terminated by a END action. > > > > -- > > > > 2.7.4 > > > > >=20 > If the reasons I gave did not manage to convince you about choosing > between > either fixed (VLAN|VXLAN)_(ENCAP|DECAP) actions or generic encap/decap > actions that deal with a single protocol layer at once instead of the > proposed approach, I'm ready to try it out as an experimental API (all ne= w > objects tagged as experimental) *if* you address the lack of mask, which > remains an open issue. >=20 > -- > Adrien Mazarguil > 6WIND