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

[Ori Kam <orika@mellanox.com>]

Hi,
PSB

> -----Original Message-----
> From: Yongseok Koh
> Sent: Monday, October 29, 2018 8:04 PM
> To: Ori Kam <orika@mellanox.com>
> Cc: Shahaf Shuler <shahafs@mellanox.com>; dev@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@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.

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