[dpdk-dev] [PATCH v2 3/7] ethdev: add flow action type update as an offload

[Andrew Rybchenko]

On 8/21/19 11:47 PM, pbhagavatula at marvell.com wrote:
> From: Pavan Nikhilesh <pbhagavatula at marvell.com>
>
> Add new Rx offload flag `DEV_RX_OFFLOAD_FLOW_MARK` that can be used to
> enable/disable PMDs write to `rte_mbuf::hash::fdir::hi` and
> `rte_mbuf::ol_flags` when flow actions `RTE_FLOW_ACTION_MARK` and
> `RTE_FLOW_ACTION_FLAG` are enabled.
>
> PMDs notify the validity of `rte_mbuf::hash:fdir::hi` to the applcation
> by enabling `PKT_RX_FDIR_ID` flag in `rte_mbuf::ol_flags`.
>
> Signed-off-by: Pavan Nikhilesh <pbhagavatula at marvell.com>

LGTM, few minor notes below, otherwise

Reviewed-by: Andrew Rybchenko <arybchenko at solarflare.com>

However, it is a long discussion of it in v1 which should be taken into 
account.

<snip>

> diff --git a/lib/librte_ethdev/rte_flow.h b/lib/librte_ethdev/rte_flow.h
> index b66bf1495..5d9d88d76 100644
> --- a/lib/librte_ethdev/rte_flow.h
> +++ b/lib/librte_ethdev/rte_flow.h
> @@ -1316,7 +1316,8 @@ enum rte_flow_action_type {
>   
>   	/**
>   	 * Attaches an integer value to packets and sets PKT_RX_FDIR and
> -	 * PKT_RX_FDIR_ID mbuf flags.
> +	 * PKT_RX_FDIR_ID mbuf flags when
> +	 * `rx_mode:offloads:DEV_RX_OFFLOAD_FLOW_MARK` is set.

I'd not concentrate on rx_mode offloads here. I'd say:
when Rx offload DEV_RX_OFFLOAD_FLOW_MARK is enabled.

>   	 *
>   	 * See struct rte_flow_action_mark.
>   	 */
> @@ -1324,7 +1325,8 @@ enum rte_flow_action_type {
>   
>   	/**
>   	 * Flags packets. Similar to MARK without a specific value; only
> -	 * sets the PKT_RX_FDIR mbuf flag.
> +	 * sets the PKT_RX_FDIR mbuf flag when
> +	 * `rx_mode:offloads:DEV_RX_OFFLOAD_FLOW_MARK` is set

Same as above.

>   	 *
>   	 * No associated configuration structure.
>   	 */