From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from mails.dpdk.org (mails.dpdk.org [217.70.189.124]) by inbox.dpdk.org (Postfix) with ESMTP id EC9F342F93; Wed, 2 Aug 2023 16:54:14 +0200 (CEST) Received: from mails.dpdk.org (localhost [127.0.0.1]) by mails.dpdk.org (Postfix) with ESMTP id 7D07440DDB; Wed, 2 Aug 2023 16:54:14 +0200 (CEST) Received: from dkmailrelay1.smartsharesystems.com (smartserver.smartsharesystems.com [77.243.40.215]) by mails.dpdk.org (Postfix) with ESMTP id 3A6124021D; Wed, 2 Aug 2023 16:54:13 +0200 (CEST) Received: from smartserver.smartsharesystems.com (smartserver.smartsharesys.local [192.168.4.10]) by dkmailrelay1.smartsharesystems.com (Postfix) with ESMTP id 0249E20634; Wed, 2 Aug 2023 16:54:13 +0200 (CEST) Content-class: urn:content-classes:message MIME-Version: 1.0 Content-Type: text/plain; charset="iso-8859-1" Content-Transfer-Encoding: quoted-printable Subject: RE: [PATCH] ethdev: introduce generic flow item and action Date: Wed, 2 Aug 2023 16:54:11 +0200 X-MimeOLE: Produced By Microsoft Exchange V6.5 Message-ID: <98CBD80474FA8B44BF855DF32C47DC35D87ABE@smartserver.smartshare.dk> In-Reply-To: X-MS-Has-Attach: X-MS-TNEF-Correlator: Thread-Topic: [PATCH] ethdev: introduce generic flow item and action Thread-Index: AQHZxSHaq6JEAAy13km0+kJQgl6M8q/WzPOAgAAjTbCAABzR0A== References: <20230802173451.3151646-1-qi.z.zhang@intel.com> <98CBD80474FA8B44BF855DF32C47DC35D87AB8@smartserver.smartshare.dk> From: =?iso-8859-1?Q?Morten_Br=F8rup?= To: "Dumitrescu, Cristian" , "Zhang, Qi Z" , , , , "Richardson, Bruce" , , Cc: , , "Mcnamara, John" , "Zhang, Helin" , X-BeenThere: dev@dpdk.org X-Mailman-Version: 2.1.29 Precedence: list List-Id: DPDK patches and discussions List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Errors-To: dev-bounces@dpdk.org > From: Dumitrescu, Cristian [mailto:cristian.dumitrescu@intel.com] > Sent: Wednesday, 2 August 2023 16.06 >=20 > Hi Morten, >=20 > Thanks for your reply, comments inline below. >=20 > > From: Morten Br=F8rup > > Sent: Wednesday, August 2, 2023 11:25 AM > > > > > From: Qi Zhang [mailto:qi.z.zhang@intel.com] > > > Sent: Wednesday, 2 August 2023 19.35 > > > > > > From: Cristian Dumitrescu > > > > > > For network devices that are programmable through languages such = as > > > the P4 language, there are no pre-defined flow items and actions. > > > > > > The format of the protocol header and metadata fields that are = used to > > > specify the flow items that make up the flow pattern, as well as = the > > > flow actions, are all defined by the program, with an infinity of > > > possible combinations, as opposed to being selected from a finite > > > pre-defined list. > > > > > > It is virtually impossible to pre-define all the flow items and = the > > > flow actions that programs might ever use, as these are only = limited > > > by the set of HW resources and the program developer's = imagination. > > > > > > To support the programmable network devices, we are introducing: > > > > > > * A generic flow item: The flow item is expressed as an array of = bytes > > > of a given length, whose meaning is defined by the program loaded = by > > > the network device. > > > > The flow item is not "generic", it is "opaque": Only the application = knows > what > this flow item does. >=20 > This item is definitely not opaque, as it does not contain any > pointers/handles. > It actually contains the exact bytes of the field in clear text, so = how is > this opaque? The flow item is opaque in the sense that the DPDK library has no way to = interpret it. E.g., unlike the RAW item, it cannot be debug-dumped to = some human readable text format by a future DPDK library function. >=20 > We already have flow items in RTE_FLOW such as RAW, FUZZY, META, TAG, = FLEX > whose meaning is fully defined by the application, as you state, with = some of > them > (like FLEX) containing pointers or indices that are completely = "opaque". We > could > use one of these existing items, but we think it would be cleaner to = define a > new > one that is more aligned to this specific purpose. >=20 > The way I would read your comment is that the _format_ of the flow = item is > defined by the application. But this is how the P4-programmable HW = devices > from > Intel and other vendors work: the P4 program loaded by the device = (i.e. the > data > plane application) defines the _format_ of the flow items. Yes, the _format_ is opaque to DPDK, and only understood by the = application. >=20 > > > > I hate the concept for two reasons: > > 1. The inability for applications to detect which flow items the = underlying > > hardware supports. >=20 > Does RTE_FLOW have any capability to detect what flows are supported = by the > underlying HW support? I don't know. But a capability query API could be implemented. > How is this a problem introduced by this proposal? With this proposal, querying for capabilities seems impossible. Maybe not. >=20 > > 2. The risk that vendors will use this instead of introducing new = flow item > > types, available for anyone to implement. >=20 > As stated above, there are already many existing flow items in = RTE_FLOW that > do just > thisc(and even worse, as some of them contain opaque pointers or = indices), > such as > RAW, FUZZY, META, TAG, FLEX. >=20 > > > > If you proceed anyway, there's a few comments inline below. > > > > > The device is still expected to accept the > > > existing pre-defined flow items such as Ethernet, IPv4/IPv6 = headers, > > > etc, as long as the program currently loaded on the device is = defining > > > and using flow items with identical format, but the device is not > > > limited to the current set of pre-defined RTE_FLOW flow items. > > > > > > * A generic flow action: The flow action exact processing is = defined > > > by the program loaded by the network device, with the user = expected to > > > know the set of program actions for the purpose of assigning = actions > > > to flows. The device is still expected to accept the existing > > > pre-defined flow actions such as packet drop, packet redirection = to > > > output queues, packet modifications, etc, as long as the program > > > currently loaded on the device is defining and using flow actions = that > > > perform identical processing, but the device is not limited to the > > > current set of pre-defined RTE_FLOW flow actions. > > > > > > Signed-off-by: Cristian Dumitrescu > > > Signed-off-by: Qi Zhang > > > --- > > > lib/ethdev/rte_flow.h | 82 > > +++++++++++++++++++++++++++++++++++++++++++ > > > 1 file changed, 82 insertions(+) > > > > > > diff --git a/lib/ethdev/rte_flow.h b/lib/ethdev/rte_flow.h > > > index 3fe57140f9..f7889d7dd0 100644 > > > --- a/lib/ethdev/rte_flow.h > > > +++ b/lib/ethdev/rte_flow.h > > > @@ -688,6 +688,15 @@ enum rte_flow_item_type { > > > * @see struct rte_flow_item_ib_bth. > > > */ > > > RTE_FLOW_ITEM_TYPE_IB_BTH, > > > + > > > + /** > > > + * Matches a custom protocol header or metadata field = represented > > > + * as a byte string of a given length, whose meaning is = typically > > > + * defined by the data plane program. > > > + * > > > + * @see struct rte_flow_item_generic. > > > + */ > > > + RTE_FLOW_ITEM_TYPE_GENERIC, > > > > Rename: _GENERIC -> _OPAQUE, everywhere. > > > > > }; > > > > > > /** > > > @@ -2311,6 +2320,32 @@ static const struct rte_flow_item_tx_queue > > > rte_flow_item_tx_queue_mask =3D { > > > }; > > > #endif > > > > > > +/** > > > + * @warning > > > + * @b EXPERIMENTAL: this structure may change without prior = notice > > > + * > > > + * RTE_FLOW_ITEM_TYPE_GENERIC > > > + * > > > + * Matches a custom protocol header or metadata field represented = as a > > byte > > > + * array of a given length, whose meaning is typically defined by = the > data > > > + * plane program. > > > > "byte array" -> "opaque data type" >=20 > This flow item does not contain any pointer or index with hidden = meaning, > this flow item contains the exact byte array in clear text, so it is = not > opaque. The meaning of the byte array is opaque. It doesn't matter that the byte = array is in clear text. If you look at the other flow items (e.g. RAW, FLEX), anyone can parse = the byte array and tell what it means. > The only difference is that you don't have a predefined name for its = protocol, > as the field format is defined by the P4 program loaded by the HW = device. >=20 >=20 > > > > > + * > > > + * The field length must be non-zero. The field value must be = non-NULL, > > with > > > the > > > + * value bytes specified in network byte order. > > > + */ > > > +struct rte_flow_item_generic { > > > + uint32_t length; /**< Field length. */ > > > + const uint8_t *value; /**< Field value. */ > > > > Suggestion: Change the value type from "const uint8_t *" to "const = void *". > It > > makes it easier for the application to use a hierarchy of structures > internally > > for this. >=20 > The "value" should be an array of exactly "length" bytes. Changing the = data > type to > "void *" is making this field opaque for no good reason IMO. We are = simply > following > the same pattern used by other flow items such as RAW and FLEX, which = are also > specified as array of bytes, right? OK. It was only a suggestion. It seems you are doing the same as FLEX = here. NB: I wonder why the flow item points to the value (in RAW and FLEX), = instead of directly holding the value in a variable length array... but = it's not important. >=20 > > > > > +}; > > > + > > > +/** Default mask for RTE_FLOW_ITEM_TYPE_GENERIC. */ > > > +#ifndef __cplusplus > > > +static const struct rte_flow_item_generic = rte_flow_item_generic_mask =3D { > > > + .length =3D 0xffffffff, > > > + .value =3D NULL, > > > +}; > > > +#endif > > > + > > > /** > > > * Action types. > > > * > > > @@ -2989,6 +3024,14 @@ enum rte_flow_action_type { > > > * @see struct rte_flow_action_indirect_list > > > */ > > > RTE_FLOW_ACTION_TYPE_INDIRECT_LIST, > > > + > > > + /** > > > + * Custom action whose processing is typically defined by the = data > > plane > > > + * program. > > > + * > > > + * @see struct rte_flow_action_generic. > > > + */ > > > + RTE_FLOW_ACTION_TYPE_GENERIC, > > > }; > > > > > > /** > > > @@ -4064,6 +4107,45 @@ struct > > rte_flow_indirect_update_flow_meter_mark { > > > enum rte_color init_color; > > > }; > > > > > > +/** > > > + * @warning > > > + * @b EXPERIMENTAL: this structure may change without prior = notice. > > > + * > > > + * Generic action argument configuration parameters. > > > + * > > > + * The action argument field length must be non-zero. The action = argument > > > field > > > + * value must be non-NULL, with the value bytes specified in = network byte > > > order. > > > + * > > > + * @see struct rte_flow_action_generic > > > + */ > > > +struct rte_flow_action_generic_argument { > > > + /** Argument field length. */ > > > + uint32_t length; > > > + /** Argument field value. */ > > > + const uint8_t *value; > > > +}; > > > + > > > +/** > > > + * @warning > > > + * @b EXPERIMENTAL: this structure may change without prior = notice. > > > + * > > > + * RTE_FLOW_ACTION_TYPE_GENERIC > > > + * > > > + * Generic action configuration parameters. > > > + * > > > + * Each action can have zero or more arguments. > > > + * > > > + * @see RTE_FLOW_ACTION_TYPE_GENERIC > > > + */ > > > +struct rte_flow_action_generic { > > > + /** Action ID. */ > > > + uint32_t action_id; > > > + /** Number of action arguments. */ > > > + uint32_t action_args_num; > > > + /** Action arguments array. */ > > > + const struct rte_flow_action_generic_argument *action_args; > > > +}; > > > > This is a list of arguments, not one argument. You should append = "_array" > > somewhere in the name, and add a normal (non-list) action, e.g.: > > > > struct rte_flow_action_opaque { > > //Or: "struct rte_flow_action_generic", if you want to keep that = name. > > /** Action ID. */ > > uint32_t action_id; > > /** Argument length. */ > > uint32_t length; > > /** Argument value. */ > > const uint8_t *value; > > // Or: "const void *value;" for the same reason as for the flow = item. > > }; > > >=20 > Did you somehow overlooked the "struct rte_flow_action_generic" from = just > above? >=20 > An action can have zero, one or more arguments, which are expressed > in the "action_args" array; we are requiring the user to provide the = action > arguments as an array of "struct rte_flow_action_generic" as opposed = to > a single raw buffer with all the arguments concatenated there, makes = sense? Makes sense. I was thinking that you should have two different flow = items, one item with a single argument, and one item with an array of = arguments. However, it seems that FLEX also does what you do here, i.e. = it has one flow item to cover zero, one or more arguments. >=20 >=20 > > > > > + > > > /* Mbuf dynamic field offset for metadata. */ > > > extern int32_t rte_flow_dynf_metadata_offs; > > > > > > -- > > > 2.31.1 >=20 > Regards, > Cristian