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

[Thomas Monjalon <thomas@monjalon.net>]

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: http://patches.dpdk.org/patch/85384/

You don't need to refer to the RFC in the commit message.

> 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?

> +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?

> +- ``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.

> +
> +``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.

> +
> +``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.

[...]

In my opinion, the most important info is in the doxygen comments.

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;
> +};