From mboxrd@z Thu Jan 1 00:00:00 1970 From: Adrien Mazarguil Subject: Re: [PATCH v2 4/4] ether: add packet modification aciton in flow API Date: Thu, 12 Apr 2018 09:03:43 +0200 Message-ID: <20180412070343.GO4957@6wind.com> References: <1522279780-34842-1-git-send-email-qi.z.zhang@intel.com> <1522617562-25940-1-git-send-email-qi.z.zhang@intel.com> <1522617562-25940-5-git-send-email-qi.z.zhang@intel.com> Mime-Version: 1.0 Content-Type: text/plain; charset=us-ascii Cc: dev@dpdk.org, declan.doherty@intel.com, sugesh.chandran@intel.com, michael.j.glynn@intel.com, yu.y.liu@intel.com, konstantin.ananyev@intel.com, bruce.richardson@intel.com To: Qi Zhang Return-path: Received: from mail-wm0-f67.google.com (mail-wm0-f67.google.com [74.125.82.67]) by dpdk.org (Postfix) with ESMTP id E798B1BADF for ; Thu, 12 Apr 2018 09:03:57 +0200 (CEST) Received: by mail-wm0-f67.google.com with SMTP id b21so7767694wme.4 for ; Thu, 12 Apr 2018 00:03:57 -0700 (PDT) Content-Disposition: inline In-Reply-To: <1522617562-25940-5-git-send-email-qi.z.zhang@intel.com> List-Id: DPDK patches and discussions List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Errors-To: dev-bounces@dpdk.org Sender: "dev" On Sun, Apr 01, 2018 at 05:19:22PM -0400, Qi Zhang wrote: > Add new actions that be used to modify packet content with > generic semantic: > > RTE_FLOW_ACTION_TYPE_FIELD_UPDATE: > - update specific field of packet > RTE_FLWO_ACTION_TYPE_FIELD_INCREMENT: > - increament specific field of packet > RTE_FLWO_ACTION_TYPE_FIELD_DECREMENT: > - decreament specific field of packet > RTE_FLWO_ACTION_TYPE_FIELD_COPY: > - copy data from one field to another in packet. > > All action use struct rte_flow_item parameter to match the pattern > that going to be modified, if no pattern match, the action just be > skipped. That's not good. It must result in undefined behavior, more about that below. > These action are non-terminating action. they will not > impact the fate of the packets. > > Signed-off-by: Qi Zhang Noticed a few typos above and in subject line ("aciton", "FLWO", "increament", "decreament"). Note that I'm usually against using rte_flow_item structures and associated enum values inside action lists because it could be seen as inconsistent from an API standpoint. On the other hand, reusing existing types is a good thing so let's go with that for now. Please see inline comments. > --- > doc/guides/prog_guide/rte_flow.rst | 89 ++++++++++++++++++++++++++++++++++++++ > lib/librte_ether/rte_flow.h | 57 ++++++++++++++++++++++++ > 2 files changed, 146 insertions(+) > > diff --git a/doc/guides/prog_guide/rte_flow.rst b/doc/guides/prog_guide/rte_flow.rst > index aa5c818..6628964 100644 > --- a/doc/guides/prog_guide/rte_flow.rst > +++ b/doc/guides/prog_guide/rte_flow.rst > @@ -1508,6 +1508,95 @@ Representor. > | ``port_id`` | identification of the destination | > +--------------+-----------------------------------+ > > +Action: ``FILED_UPDATE`` > +^^^^^^^^^^^^^^^^^^^^^^^ FILED => FIELD Underline is also shorter than title and might cause documentation warnings. > + > +Update specific field of the packet. > + > +- Non-terminating by default. These statements are not needed since "ethdev: alter behavior of flow API actions" [1]. [1] http://dpdk.org/ml/archives/dev/2018-April/096527.html > + > +.. _table_rte_flow_action_field_update: > + > +.. table:: FIELD_UPDATE > + > + +-----------+---------------------------------------------------------+ > + | Field | Value | > + +===========+=========================================================+ > + | ``item`` | item->type: specify the pattern to modify | > + | | item->spec: specify the new value to update | > + | | item->mask: specify which part of the pattern to update | > + | | item->last: ignored | This table needs to be divided a bit more with one cell per field for better clarity. See other pattern item definitions such as "Item: ``RAW``" for an example. > + +-----------+---------------------------------------------------------+ > + | ``layer`` | 0 means outermost matched pattern, | > + | | 1 means next-to-outermost and so on ... | > + +-----------+---------------------------------------------------------+ What does "layer" refer to by the way? The layer described on the pattern side of the flow rule, the actual protocol layer matched inside traffic, or is "item" actually an END-terminated list of items (as suggested by "pattern" in above documentation)? I suspect the intent is for layer to have the same definition as RSS encapulation level ("ethdev: add encap level to RSS flow API action" [2]), and item points to a single item, correct? In that case, it's misleading, please rename it "level". Also keep in mind you can't make an action rely on anything found on the pattern side of a flow rule. What happens when this action is attempted on non-matching traffic must be documented here as well. Refer to discussion re "ethdev: Add tunnel encap/decap actions" [3]. To be on the safe side, it must be documented as resulting in undefined behavior. Based the same thread, I also suggest here to define "last" as reserved and therefore an error if set to anything other than NULL, however it might prove useful, see below. [2] http://dpdk.org/ml/archives/dev/2018-April/096531.html [3] http://dpdk.org/ml/archives/dev/2018-April/096418.html > + > +Action: ``FILED_INCREMENT`` > +^^^^^^^^^^^^^^^^^^^^^^^^^^^ FILED => FIELD > + > +Increment 1 on specific field of the packet. All right, but what for? FIELD_UPDATE overwrites a specific value at some specific place after matching something rather specific. In my opinion to get predictable results with FIELD_INCREMENT, applications also need to have a pretty good idea of what's about to be incremented. That's because you can't put conditionals in flow rules (yet). So if you need to match an exact IPv4 address in order to increment it, why wouldn't you just provide its replacement directly? I'm not saying there are no use cases for this, but you know, a couple of real world example scenarios can't hurt. This should answer what an application could possibly want to unconditionally increment in ingress/egress traffic and for what purpose. > + > +- Non-terminating by default. Same comment as in FIELD_UPDATE. > + > +.. _table_rte_flow_action_field_update: > + > +.. table:: FIELD_INCREMENT > + > + +-----------+---------------------------------------------------------+ > + | Field | Value | > + +===========+=========================================================+ > + | ``item`` | item->type: specify the pattern to modify | > + | | item->spec: ignored | > + | | item->mask: specify which part of the pattern to update | > + | | item->last: ignored | > + +-----------+---------------------------------------------------------+ > + | ``layer`` | 0 means outermost matched pattern, | > + | | 1 means next-to-outermost and so on ... | > + +-----------+---------------------------------------------------------+ Ditto. With another comment regarding "last". When specified it could be used to provide a stride, i.e. the amount to increment instead of 1 (increment = last - spec). > + > +Action: ``FIELD_DECREMENT`` > +^^^^^^^^^^^^^^^^^^^^^^^^^^^ > + > +Decrement 1 on specific field of the packet. Same comment as in FIELD_INCREMENT. > + > +Paramenter is same is FIELD_INCREMENT. Paramenter => Parameter > + > +-Non-terminating by default. Same comment as in FIELD_UPDATE. A table is missing in this section and must be included. Keep in mind section titles are used as anchors in the documentation and readers may reach this point without going through the previous sections. > + > +ACTION: ``FIELD_COPY`` > +^^^^^^^^^^^^^^^^^^^^^^ > + > +Copy data of one field to another of the packet. > + > +-Non-terminating by default. Same comment as in FIELD_UPDATE. > + > +.. _table_rte_flow_action_field_copy: > + > +.. table:: FIELD_COPY > + > + +-----------------+-----------------------------------------------------------------+ > + | Field | Value | > + +=================+=================================================================+ > + | ``src_item`` | src_item->type: match the pattern that data will be copy from | > + | | src_item->spec: ignored | > + | | src_item->mask: specify which part of the pattern to copy | > + | | src_item->last: ignored | > + +-----------------+-----------------------------------------------------------------+ > + | ``src_layer`` | layer of src_item | > + | | 0 means outermost matched pattern, | > + | | 1 means next-to-outermost and so on ... | > + +-----------------+-----------------------------------------------------------------+ > + | ``dst_item`` | dst_item->type: match the pattern that data will be copy to | > + | | dst_item->spec: ignored | > + | | dst_item->mask: specify which part of the pattern to be update | > + | | it must match src_item->mask. | > + | | dst_item->last: ignored | > + +-----------------+-----------------------------------------------------------------+ > + | ``dst_layer`` | layer of dst_item | > + | | 0 means outermost matched pattern, | > + | | 1 means next-to-outermost and so on ... | > + +-----------------+-----------------------------------------------------------------+ Same comments as in FIELD_UPDATE regarding table format, "last", etc. A couple of real world use cases would be a nice addition here as well. > + > Negative types > ~~~~~~~~~~~~~~ > > diff --git a/lib/librte_ether/rte_flow.h b/lib/librte_ether/rte_flow.h > index a8ec780..d81ac4c 100644 > --- a/lib/librte_ether/rte_flow.h > +++ b/lib/librte_ether/rte_flow.h > @@ -1178,6 +1178,34 @@ enum rte_flow_action_type { > * See struct rte_flow_action_port. > */ > RTE_FLOW_ACTION_TYPE_PORT, > + > + /** > + * Update specific field of the packet. Update => Updates (to keep the style) > + * > + * See struct rte_flow_item_field_update. > + */ > + RTE_FLOW_ACTION_TYPE_FILED_UPDATE, FILED => FIELD > + > + /** > + * Increment specific field of the packet. Increment => Increments > + * > + * See struct rte_flow_item_field_update. > + */ > + RTE_FLOW_ACTION_TYPE_FIELD_INCREMENT, > + > + /** > + * Decrement specific field of the packet. Decrement => Decrements > + * > + * See struct rte_flow_item_field_update. > + */ > + RTE_FLOW_ACTION_TYPE_FIELD_DECREMENT, > + > + /** > + * Copy data of one field to another of the packet. Copy => Copies > + * > + * See struct rte_flow_item_type_field_copy. > + */ > + RTE_FLOW_ACTION_TYPE_FIELD_COPY, > }; > > /** > @@ -1327,6 +1355,35 @@ struct rte_flow_action_port { > uint16_t port_id; /**< identification of the forward destination. */ > }; > > +/** RTE_FLOW_ACTION_TYPE_FIELD_UPDATE Here "/**" should be on its own line like the rest of the file. You can safely ignore checkpatch nonsense (if any). > + * RTE_FLOW_ACTION_TYPE_FIELD_INCREMENT > + * RTE_FLOW_ACTION_TYPE_FIELD_DECREMENT One shared structure for everything? How about a single UPDATE action to cover everything instead? - mask && last is NULL or last - spec is 0 => update - mask && last - spec is positive => increment by that amount - mask && last - spec is negative => decrement by that amount This would be easier to document, would result in a smaller patch, implicitly gives meaning to "last" and provides the ability to simultaneously increment, decrement and update several fields at once. > + * > + * Update specific field of the packet. > + * > + * Typical usage: update mac/ip address. Documentation is a bit weak and needs improvement. In any case, except for tables and examples, whatever is written in this comment should be word for word the same as what is written in rte_flow.rst. > + */ > +struct rte_flow_action_field_update { > + const struct rte_flow_item *item; /**< specify the data to modify. */ Since there is a single item that contains its own pointers to spec/last/mask, "item" shouldn't be a pointer. It's unnecessary and misleading. Documentation needs improvement. > + uint8_t layer; > + /**< 0 means outermost matched pattern, 1 means next-to-outermost... */ uint8_t layer => uint32_t level (we're not constrained by space) Ditto re documentation. See RSS encap level patch [2] for ideas. > +}; > + > +/** RTE_FLOW_ACTION_TYPE_FIELD_COPY Ditto for "/**". > + * > + * Copy data from one field to another of the packet. > + * > + * Typical usage: TTL copy-in / copy-out This typical usage doesn't look that obvious to me. Copy TTL from what part of the packet to where? What happens to the overwritten TTL value? Why should they be synchronized? > + */ > +struct rte_flow_action_field_copy { > + const struct rte_flow_item *src_item; /**< Specify the data copy from */ > + uint8_t src_layer; > + /**< 0 means outermost matched pattern, 1 means next-to-outermost... */ > + const struct rte_flow_item *dst_item; /**< Specify the data copy to */ > + uint8_t dst_layer; > + /**< 0 means outermost matched pattern, 1 means next-to-outermost... */ > +}; Same comments as for struct rte_flow_action_field_update. > + > /** > * Definition of a single action. > * > -- > 2.7.4 > -- Adrien Mazarguil 6WIND