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

[Yongseok Koh]

> On Oct 31, 2018, at 1:25 AM, Ori Kam <orika at mellanox.com> wrote:
> 
> Hi,
> PSB
> 
>> -----Original Message-----
>> From: Yongseok Koh
>> Sent: Monday, October 29, 2018 8:04 PM
>> To: Ori Kam <orika at mellanox.com>
>> Cc: Shahaf Shuler <shahafs at mellanox.com>; dev at dpdk.org
>> Subject: Re: [PATCH] net/mlx5: fix Direct Verbs getting item and action flags
>> 
>>> On Oct 28, 2018, at 11:03 PM, Ori Kam <orika at 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?
> 
> According to design prepare function is responsible for allocating the flow.
> In case of Verbs the allocation size is dependent on the number of actions,
> while in Direct Verbs the size is fixed. This is the reason for the difference.
> If it helps one can add the scan for items and actions. Currently is was not needed
> in case of Direct Verbs maybe it can help for example for Dekel patches were to commands
> (encap and encap) should sometime be combined in to one command.
> Also maybe in future moving to Direct Steering it will be useful to save the actions based on the
> real number of actions.

I knew that.
That doesn't explain why _prepare() gets item_flags and action_flags to fill in, because
it isn't used anyway after _prepare() call. And I want to see more consistency here.
"Someone will add it later if needed" sounds bad.

Anyway, I'll push more fixes together related to this flag handling because it breaks
tunnel flow badly. I'll also refactor many of code lines in DV and Verbs.

I have changed this patch as 'superseded'.


Thanks,
Yongseok


>> 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 at mellanox.com>
>>>> Cc: dev at dpdk.org; Yongseok Koh <yskoh at mellanox.com>; Ori Kam
>>>> <orika at 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 at mellanox.com
>>>> 
>>>> Signed-off-by: Yongseok Koh <yskoh at 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