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

[Thomas Monjalon <thomas@monjalon.net>]

18/01/2021 17:18, Alexander Kozyrev:
> +/**
> + * Field description for MODIFY_FIELD action.
> + */
> +struct rte_flow_action_modify_data {
> +	enum rte_flow_field_id field; /**< Field ID */

more accurate:
	Field or memory type

> +	RTE_STD_C11
> +	union {
> +		struct {
> +			uint32_t level; /**< Encapsulation level or tag index */
> +			uint32_t offset; /**< Number of bits to skip from src */

"from src" only?
I think we could use it for dst as well.
I would remove "from src".

> +		};
> +		uint64_t value; /**< Immediate value or memory address of it */

You should specify for RTE_FLOW_FIELD_POINTER and RTE_FLOW_FIELD_VALUE.

Please add a dot at the end of each comment.

[...]
> +/**
> + * @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

s/Modifies/Modify/  (imperative form is preferred)

> + * operation. Another packet field can be used as a source as well
> + * as tag, mark, metadata, immediate value or a pointer to it.
> + */
> +struct rte_flow_action_modify_field {
> +	enum rte_flow_modify_op operation; /**< Operation to perform on dst*/
> +	struct rte_flow_action_modify_data dst; /**< Destination field */
> +	struct rte_flow_action_modify_data src; /**< Source field */
> +	uint32_t width; /**< Number of bits to use from a source field */
> +};


With above changes,
Acked-by: Thomas Monjalon <thomas@monjalon.net>