> From: Thomas Monjalon <tho...@monjalon.net> on Sunday, January 17, 2021 18:16 > 16/01/2021 05:50, Alexander Kozyrev: > > Implement the generic modify flow API to allow manipulations on > > an arbitrary header field (as well as mark, metadata or tag) using > > data from another field or a user-specified value. > > This generic modify mechanism removes the necessity to implement > > a separate RTE Flow action every time we need to modify a new packet > > field in the future. > > > > Supported operation are: > > - set: copy data from source to destination. > > - add: integer addition, stores the result in destination. > > - sub: integer subtraction, stores the result in destination. > > > > The field ID is used to specify the desired source/destination packet > > field in order to simplify the API for various encapsulation models. > > Specifying the packet field ID with the needed encapsulation level > > is able to quickly get a packet field for any inner packet header. > > > > Alternatively, the special ID (ITEM_START) can be used to point to > > the very beginning of a packet. This ID in conjunction with the > > offset parameter provides great flexibility to copy/modify any part of > > a packet as needed. > > > > The number of bits to use from a source as well as the offset can be > > be specified to allow a partial copy or dividing a big packet field > > into multiple small fields (e.g. copying 128 bits of IPv6 to 4 tags). > > > > An immediate value (or pointer to it) can be specified instead of the > > level and the offset for the special FIELD_VALUE ID (or FIELD_POINTER). > > Can be used as a source only. > > > > RFC: > https://nam11.safelinks.protection.outlook.com/?url=http%3A%2F%2Fpatch > es.dpdk.org%2Fpatch%2F85384%2F&data=04%7C01%7Cakozyrev%40nv > idia.com%7Ce20d2c38372b4062420d08d8bb3dd549%7C43083d15727340c1b7d > b39efd9ccc17a%7C0%7C0%7C637465221537183554%7CUnknown%7CTWFpb > GZsb3d8eyJWIjoiMC4wLjAwMDAiLCJQIjoiV2luMzIiLCJBTiI6Ik1haWwiLCJXVCI > 6Mn0%3D%7C1000&sdata=gWEdmyAabnCCmeFyi%2FvHs0aheMmWNx > IyR%2BTC96O3Yck%3D&reserved=0 > > You don't need to refer to the RFC in the commit message. Removing link to it.
> > > Signed-off-by: Alexander Kozyrev <akozy...@nvidia.com> > > Acked-by: Ori Kam <or...@nvidia.com> > > > > --- > > +Action: ``MODIFY_FIELD`` > > +^^^^^^^^^^^^^^^^^^^^^^^^ > > + > > +Modify ``dst`` field according to ``op`` selected (move, addition, > > +subtraction) with ``width`` bits of data from ``src`` field. > > "move" is changed to "set", right? Right, thanks for catching this. > > > +Any arbitrary header field (as well as mark, metadata or tag values) > > +can be used as both source and destination fields as set by ``field``. > > +The immediate value ``RTE_FLOW_FIELD_VALUE`` (or a pointer to it > > +``RTE_FLOW_FIELD_POINTER``) is allowed as a source only. > > +``RTE_FLOW_FIELD_START`` is used to point to the beginning of a packet. > > + > > +``op`` selects the operation to perform on a destination field. > > +- ``set`` copies the data from ``src`` field to ``dst`` field. > > +- ``add`` adds together ``dst`` and ``src`` and stores the result into > > ``dst``. > > +- ``sub`` subtracts ``src`` from ``dst`` and stores the result into ``dst`` > > + > > +``width`` defines a number of bits to use from ``src`` field. > > + > > +``level`` is used to access any packet field on any encapsulation level > > +as well as any tag element in the tag array. > > +- ``0`` means the default behaviour. Depending on the packet type, it can > > +mean outermost, innermost or anything in between. > > I feel the interpretation of the level 0 is driver-dependent? It is driver-dependent, the behavior is the same as in case of RSS. > > +- ``1`` requests access to the outermost packet encapsulation level. > > +- ``2`` and subsequent values requests access to the specified packet > > +encapsulation level, from outermost to innermost (lower to higher > values). > > +For the tag array ``level`` translates directly into the array index. > > You should define what is a tag array. Ok, but it is defined already in RTE_FLOW_ACTION_TYPE_SET_TAG action. > > + > > +``offset`` specifies the number of bits to skip from a field's start. > > +That allows performing a partial copy of the needed part or to divide a big > > +packet field into multiple smaller fields. Alternatively, ``offset`` allows > > +going past the specified packet field boundary to copy a field to an > > +arbitrary place in a packet, essentially providing a way to copy any part > > of > > +a packet to any other part of it if supported by an underlying PMD driver. > > All features of this specification require PMD support, > so I think it is useless to mention here. Removing. > > + > > +``value`` sets an immediate value to be used as a source or points to a > > +location of the value in memory. It is used instead of ``level`` and > > ``offset`` > > +for ``RTE_FLOW_FIELD_VALUE`` and ``RTE_FLOW_FIELD_POINTER`` > respectively. > > + > > +.. _table_rte_flow_action_modify_field: > > + > > +.. table:: MODIFY_FIELD > > + > > + +-----------------------------------------+ > > + | Field | Value | > > + +===============+=========================+ > > + | ``op`` | operation to perform | > > + | ``dst`` | destination field | > > + | ``src`` | source field | > > + | ``width`` | number of bits to use | > > + +---------------+-------------------------+ > > + > > +.. _table_rte_flow_action_modify_data: > > + > > +.. table:: destination/source field definition > > + > > + > > +--------------------------------------------------------------------------+ > > + | Field | Value > > | > > + > +===============+========================================= > =================+ > > + | ``field`` | ID: packet field, mark, meta, tag, immediate, pointer > > | > > + | ``level`` | encapsulation level of a packet field or tag array > > index | > > + | ``offset`` | number of bits to skip at the beginning > > | > > + | ``value`` | immediate value or a pointer to this value > > | > > + > > +---------------+----------------------------------------------------------+ > > I know the whole rte_flow guide has this kind of table, > but really it looks like doxygen and can be skipped here. > > If really this redundant info is required, it should be RST definition lists, > not tables. I would keep these tables for the sake of consistency of the rte_flow guide. > [...] > > In my opinion, the most important info is in the doxygen comments. Agree, adding them for all the structures/fields mentioned below. > Please add a doxygen comment for this enum. > > > +enum rte_flow_field_id { > > + RTE_FLOW_FIELD_START = 0, > > Please add a doxygen comment for the value RTE_FLOW_FIELD_START. > > > + RTE_FLOW_FIELD_MAC_DST, > > + RTE_FLOW_FIELD_MAC_SRC, > > + RTE_FLOW_FIELD_VLAN_TYPE, > > + RTE_FLOW_FIELD_VLAN_ID, > > + RTE_FLOW_FIELD_MAC_TYPE, > > + RTE_FLOW_FIELD_IPV4_DSCP, > > + RTE_FLOW_FIELD_IPV4_TTL, > > + RTE_FLOW_FIELD_IPV4_SRC, > > + RTE_FLOW_FIELD_IPV4_DST, > > + RTE_FLOW_FIELD_IPV6_HOPLIMIT, > > + RTE_FLOW_FIELD_IPV6_SRC, > > + RTE_FLOW_FIELD_IPV6_DST, > > + RTE_FLOW_FIELD_TCP_PORT_SRC, > > + RTE_FLOW_FIELD_TCP_PORT_DST, > > + RTE_FLOW_FIELD_TCP_SEQ_NUM, > > + RTE_FLOW_FIELD_TCP_ACK_NUM, > > + RTE_FLOW_FIELD_TCP_FLAGS, > > + RTE_FLOW_FIELD_UDP_PORT_SRC, > > + RTE_FLOW_FIELD_UDP_PORT_DST, > > + RTE_FLOW_FIELD_VXLAN_VNI, > > + RTE_FLOW_FIELD_GENEVE_VNI, > > + RTE_FLOW_FIELD_GTP_TEID, > > + RTE_FLOW_FIELD_TAG, > > + RTE_FLOW_FIELD_MARK, > > + RTE_FLOW_FIELD_META, > > + RTE_FLOW_FIELD_POINTER, > > Please add a doxygen comment for the value RTE_FLOW_FIELD_POINTER. > > > + RTE_FLOW_FIELD_VALUE, > > Please add a doxygen comment for the value RTE_FLOW_FIELD_VALUE. > > > +struct rte_flow_action_modify_data { > > + enum rte_flow_field_id field; > > + RTE_STD_C11 > > + union { > > + struct { > > + uint32_t level; > > + uint32_t offset; > > + }; > > + uint64_t value; > > + }; > > +}; > > Please add doxygen for this struct and fields. > > > + > > +enum rte_flow_modify_op { > > + RTE_FLOW_MODIFY_SET = 0, > > + RTE_FLOW_MODIFY_ADD, > > + RTE_FLOW_MODIFY_SUB, > > +}; > > Please add doxygen for this enum. > > > + > > +/** > > + * @warning > > + * @b EXPERIMENTAL: this structure may change without prior notice > > + * > > + * RTE_FLOW_ACTION_TYPE_MODIFY_FIELD > > + * > > + * Modifies a destination header field according to the specified > > + * operation. Another packet field can be used as a source as well > > + * as tag, mark, metadata or an immediate value or a pointer to it. > > + * Width is the number of bits used from the source item. > > width should be documented below as other fields. > > > + */ > > +struct rte_flow_action_modify_field { > > + enum rte_flow_modify_op operation; > > + struct rte_flow_action_modify_data dst; > > + struct rte_flow_action_modify_data src; > > + uint32_t width; > > +}; > >