Re: [dpdk-dev] [PATCH v7 1/2] ethdev: introduce generic modify rte flow action

[Alexander Kozyrev <akozyrev@nvidia.com>]

> From: Thomas Monjalon <thomas@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&amp;data=04%7C01%7Cakozyrev%40nv
> idia.com%7Ce20d2c38372b4062420d08d8bb3dd549%7C43083d15727340c1b7d
> b39efd9ccc17a%7C0%7C0%7C637465221537183554%7CUnknown%7CTWFpb
> GZsb3d8eyJWIjoiMC4wLjAwMDAiLCJQIjoiV2luMzIiLCJBTiI6Ik1haWwiLCJXVCI
> 6Mn0%3D%7C1000&amp;sdata=gWEdmyAabnCCmeFyi%2FvHs0aheMmWNx
> IyR%2BTC96O3Yck%3D&amp;reserved=0
> 
> You don't need to refer to the RFC in the commit message.
Removing link to it.

> 
> > Signed-off-by: Alexander Kozyrev <akozyrev@nvidia.com>
> > Acked-by: Ori Kam <orika@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;
> > +};
> 
>