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

[Ori Kam <orika@mellanox.com>]

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.



Ori

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