Re: [dpdk-dev] [PATCH] net/mlx5: fix Direct Verbs getting item and action flags

[Yongseok Koh <yskoh@mellanox.com>]

> On Oct 28, 2018, at 11:03 PM, Ori Kam <orika@mellanox.com> wrote:
> 
> Why should DV prepare function return the list of actions?
> 
> The only reason I can think of, is if you want to remove the for loop in
> dv_translate. And then in flow_dv_create_action change the switch case
> to ifs.

Then, I should ask you a question. Why did you put actions/layers in rte_flow
struct in the first place? And _prepare() API even takes pointers of item_flags
and actions_flags in order to fill in the structs for nothing. Verbs and TCF
fills in the structs but only DV doesn't. I think you just wanted to use
flow->actions for _apply() and it is filled in _translate(). But, in case of
TCF, _apply() doesn't need to know the action_flags and its _translate doesn't
even fill in the struct while its _prepare() does in vain.
Where's the consistency? What is the definition of the APIs?

If people started to use the fields (because it is there), it won't work as
expected. Slava wants to use the flags in _translate() for his vxlan patch.

So, I think _prepare is the right place to fill in the flags.

Let me know how you want to change it.


Thanks,
Yongseok

> 
>> -----Original Message-----
>> From: Yongseok Koh
>> Sent: Sunday, October 28, 2018 7:35 PM
>> To: Shahaf Shuler <shahafs@mellanox.com>
>> Cc: dev@dpdk.org; Yongseok Koh <yskoh@mellanox.com>; Ori Kam
>> <orika@mellanox.com>
>> Subject: [PATCH] net/mlx5: fix Direct Verbs getting item and action flags
>> 
>> Flow driver has to provide detected item flags and action flags via
>> flow_drv_prepare(). DV doesn't return flags at all.
>> 
>> Fixes: 865a0c15672c ("net/mlx5: add Direct Verbs prepare function")
>> Cc: orika@mellanox.com
>> 
>> Signed-off-by: Yongseok Koh <yskoh@mellanox.com>
>> ---
>> drivers/net/mlx5/mlx5_flow_dv.c | 115
>> ++++++++++++++++++++++++++++++++++++----
>> 1 file changed, 106 insertions(+), 9 deletions(-)
>> 
>> diff --git a/drivers/net/mlx5/mlx5_flow_dv.c
>> b/drivers/net/mlx5/mlx5_flow_dv.c
>> index 8f729f44f8..67c133c2fb 100644
>> --- a/drivers/net/mlx5/mlx5_flow_dv.c
>> +++ b/drivers/net/mlx5/mlx5_flow_dv.c
>> @@ -354,6 +354,103 @@ flow_dv_validate(struct rte_eth_dev *dev, const
>> struct rte_flow_attr *attr,
>> }
>> 
>> /**
>> + * Extract item flags and action flags.
>> + *
>> + * @param[in] items
>> + *   Pointer to the list of items.
>> + * @param[in] actions
>> + *   Pointer to the list of actions.
>> + * @param[out] item_flags
>> + *   Pointer to bit mask of all items detected.
>> + * @param[out] action_flags
>> + *   Pointer to bit mask of all actions detected.
>> + */
>> +static void
>> +flow_dv_get_item_action_flags(const struct rte_flow_item items[],
>> +			      const struct rte_flow_action actions[],
>> +			      uint64_t *item_flags, uint64_t *action_flags)
>> +{
>> +	uint64_t detected_items = 0;
>> +	uint64_t detected_actions = 0;
>> +	int tunnel;
>> +
>> +	for (; items->type != RTE_FLOW_ITEM_TYPE_END; items++) {
>> +		tunnel = !!(detected_items & MLX5_FLOW_LAYER_TUNNEL);
>> +		switch (items->type) {
>> +		case RTE_FLOW_ITEM_TYPE_ETH:
>> +			detected_items |= tunnel ?
>> MLX5_FLOW_LAYER_INNER_L2 :
>> +
>> MLX5_FLOW_LAYER_OUTER_L2;
>> +			break;
>> +		case RTE_FLOW_ITEM_TYPE_VLAN:
>> +			detected_items |= tunnel ?
>> MLX5_FLOW_LAYER_INNER_VLAN :
>> +
>> MLX5_FLOW_LAYER_OUTER_VLAN;
>> +			break;
>> +		case RTE_FLOW_ITEM_TYPE_IPV4:
>> +			detected_items |= tunnel ?
>> +					  MLX5_FLOW_LAYER_INNER_L3_IPV4
>> :
>> +
>> MLX5_FLOW_LAYER_OUTER_L3_IPV4;
>> +			break;
>> +		case RTE_FLOW_ITEM_TYPE_IPV6:
>> +			detected_items |= tunnel ?
>> +					  MLX5_FLOW_LAYER_INNER_L3_IPV6
>> :
>> +
>> MLX5_FLOW_LAYER_OUTER_L3_IPV6;
>> +			break;
>> +		case RTE_FLOW_ITEM_TYPE_TCP:
>> +			detected_items |= tunnel ?
>> +					  MLX5_FLOW_LAYER_INNER_L4_TCP :
>> +
>> MLX5_FLOW_LAYER_OUTER_L4_TCP;
>> +			break;
>> +		case RTE_FLOW_ITEM_TYPE_UDP:
>> +			detected_items |= tunnel ?
>> +					  MLX5_FLOW_LAYER_INNER_L4_UDP
>> :
>> +
>> MLX5_FLOW_LAYER_OUTER_L4_UDP;
>> +			break;
>> +		case RTE_FLOW_ITEM_TYPE_GRE:
>> +		case RTE_FLOW_ITEM_TYPE_NVGRE:
>> +			detected_items |= MLX5_FLOW_LAYER_GRE;
>> +			break;
>> +		case RTE_FLOW_ITEM_TYPE_VXLAN:
>> +			detected_items |= MLX5_FLOW_LAYER_VXLAN;
>> +			break;
>> +		case RTE_FLOW_ITEM_TYPE_VXLAN_GPE:
>> +			detected_items |= MLX5_FLOW_LAYER_VXLAN_GPE;
>> +			break;
>> +		case RTE_FLOW_ITEM_TYPE_META:
>> +			detected_items |= MLX5_FLOW_ITEM_METADATA;
>> +			break;
>> +		default:
>> +			break;
>> +		}
>> +	}
>> +	for (; actions->type != RTE_FLOW_ACTION_TYPE_END; actions++) {
>> +		switch (actions->type) {
>> +		case RTE_FLOW_ACTION_TYPE_FLAG:
>> +			detected_actions |= MLX5_FLOW_ACTION_FLAG;
>> +			break;
>> +		case RTE_FLOW_ACTION_TYPE_MARK:
>> +			detected_actions |= MLX5_FLOW_ACTION_MARK;
>> +			break;
>> +		case RTE_FLOW_ACTION_TYPE_DROP:
>> +			detected_actions |= MLX5_FLOW_ACTION_DROP;
>> +			break;
>> +		case RTE_FLOW_ACTION_TYPE_QUEUE:
>> +			detected_actions |= MLX5_FLOW_ACTION_QUEUE;
>> +			break;
>> +		case RTE_FLOW_ACTION_TYPE_RSS:
>> +			detected_actions |= MLX5_FLOW_ACTION_RSS;
>> +			break;
>> +		case RTE_FLOW_ACTION_TYPE_COUNT:
>> +			detected_actions |= MLX5_FLOW_ACTION_COUNT;
>> +			break;
>> +		default:
>> +			break;
>> +		}
>> +	}
>> +	*item_flags = detected_items;
>> +	*action_flags = detected_actions;
>> +}
>> +
>> +/**
>>  * Internal preparation function. Allocates the DV flow size,
>>  * this size is constant.
>>  *
>> @@ -376,15 +473,15 @@ flow_dv_validate(struct rte_eth_dev *dev, const
>> struct rte_flow_attr *attr,
>>  */
>> static struct mlx5_flow *
>> flow_dv_prepare(const struct rte_flow_attr *attr __rte_unused,
>> -		const struct rte_flow_item items[] __rte_unused,
>> -		const struct rte_flow_action actions[] __rte_unused,
>> -		uint64_t *item_flags __rte_unused,
>> -		uint64_t *action_flags __rte_unused,
>> +		const struct rte_flow_item items[],
>> +		const struct rte_flow_action actions[],
>> +		uint64_t *item_flags, uint64_t *action_flags,
>> 		struct rte_flow_error *error)
>> {
>> 	uint32_t size = sizeof(struct mlx5_flow);
>> 	struct mlx5_flow *flow;
>> 
>> +	flow_dv_get_item_action_flags(items, actions, item_flags,
>> action_flags);
>> 	flow = rte_calloc(__func__, 1, size, 0);
>> 	if (!flow) {
>> 		rte_flow_error_set(error, ENOMEM,
>> @@ -1067,7 +1164,7 @@ flow_dv_create_action(const struct rte_flow_action
>> *action,
>> 		dev_flow->dv.actions[actions_n].tag_value =
>> 			mlx5_flow_mark_set(MLX5_FLOW_MARK_DEFAULT);
>> 		actions_n++;
>> -		flow->actions |= MLX5_FLOW_ACTION_FLAG;
>> +		assert(flow->actions & MLX5_FLOW_ACTION_FLAG);
>> 		break;
>> 	case RTE_FLOW_ACTION_TYPE_MARK:
>> 		dev_flow->dv.actions[actions_n].type =
>> MLX5DV_FLOW_ACTION_TAG;
>> @@ -1075,18 +1172,18 @@ flow_dv_create_action(const struct
>> rte_flow_action *action,
>> 			mlx5_flow_mark_set
>> 			(((const struct rte_flow_action_mark *)
>> 			  (action->conf))->id);
>> -		flow->actions |= MLX5_FLOW_ACTION_MARK;
>> +		assert(flow->actions & MLX5_FLOW_ACTION_MARK);
>> 		actions_n++;
>> 		break;
>> 	case RTE_FLOW_ACTION_TYPE_DROP:
>> 		dev_flow->dv.actions[actions_n].type =
>> MLX5DV_FLOW_ACTION_DROP;
>> -		flow->actions |= MLX5_FLOW_ACTION_DROP;
>> +		assert(flow->actions & MLX5_FLOW_ACTION_DROP);
>> 		break;
>> 	case RTE_FLOW_ACTION_TYPE_QUEUE:
>> 		queue = action->conf;
>> 		flow->rss.queue_num = 1;
>> 		(*flow->queue)[0] = queue->index;
>> -		flow->actions |= MLX5_FLOW_ACTION_QUEUE;
>> +		assert(flow->actions & MLX5_FLOW_ACTION_QUEUE);
>> 		break;
>> 	case RTE_FLOW_ACTION_TYPE_RSS:
>> 		rss = action->conf;
>> @@ -1098,7 +1195,7 @@ flow_dv_create_action(const struct rte_flow_action
>> *action,
>> 		flow->rss.types = rss->types;
>> 		flow->rss.level = rss->level;
>> 		/* Added to array only in apply since we need the QP */
>> -		flow->actions |= MLX5_FLOW_ACTION_RSS;
>> +		assert(flow->actions & MLX5_FLOW_ACTION_RSS);
>> 		break;
>> 	default:
>> 		break;
>> --
>> 2.11.0
>