Re: [dpdk-dev] [PATCH 5/8] net/mlx5: split sample flow into two sub flows

[Ori Kam <orika@mellanox.com>]

Hi Jiawei,

Please fix the small comment below and send with my ack
Acked-by: Ori Kam <orika@mellanox.com>

Best,
Ori

> -----Original Message-----
> From: Jiawei Wang <jiaweiw@mellanox.com>
> Sent: Thursday, June 25, 2020 7:26 PM
> To: Ori Kam <orika@mellanox.com>; Slava Ovsiienko
> <viacheslavo@mellanox.com>; Matan Azrad <matan@mellanox.com>
> Cc: dev@dpdk.org; Thomas Monjalon <thomas@monjalon.net>; Raslan
> Darawsheh <rasland@mellanox.com>; ian.stokes@intel.com; fbl@redhat.com;
> Jiawei(Jonny) Wang <jiaweiw@mellanox.com>
> Subject: [PATCH 5/8] net/mlx5: split sample flow into two sub flows
> 
> Add the sampler action resource structs definition.
> 
> The flow with sample action will be splited into two sub flows,
> the prefix flow with sample action, the suffix flow with the left
> actions.
> 
> For the prefix flow, add the extra the tag action with unique id
> to metadata register, and suffix flow will add the extra tag item
> to match that unique id.
> 
> Signed-off-by: Jiawei Wang <jiaweiw@mellanox.com>
> ---
>  drivers/net/mlx5/mlx5.c      |  11 ++
>  drivers/net/mlx5/mlx5.h      |   3 +
>  drivers/net/mlx5/mlx5_flow.c | 254
> ++++++++++++++++++++++++++++++++++++++++++-
>  drivers/net/mlx5/mlx5_flow.h |  37 +++++++
>  4 files changed, 301 insertions(+), 4 deletions(-)
> 
> diff --git a/drivers/net/mlx5/mlx5.c b/drivers/net/mlx5/mlx5.c
> index ddbe29d..4a52462 100644
> --- a/drivers/net/mlx5/mlx5.c
> +++ b/drivers/net/mlx5/mlx5.c
> @@ -238,6 +238,17 @@ static LIST_HEAD(, mlx5_dev_ctx_shared)
> mlx5_dev_ctx_list =
>  		.free = rte_free,
>  		.type = "mlx5_jump_ipool",
>  	},
> +	{
> +		.size = sizeof(struct mlx5_flow_dv_sample_resource),
> +		.trunk_size = 64,
> +		.grow_trunk = 3,
> +		.grow_shift = 2,
> +		.need_lock = 0,
> +		.release_mem_en = 1,
> +		.malloc = rte_malloc_socket,
> +		.free = rte_free,
> +		.type = "mlx5_sample_ipool",
> +	},
>  #endif
>  	{
>  		.size = sizeof(struct mlx5_flow_meter),
> diff --git a/drivers/net/mlx5/mlx5.h b/drivers/net/mlx5/mlx5.h
> index c2a875c..7394753 100644
> --- a/drivers/net/mlx5/mlx5.h
> +++ b/drivers/net/mlx5/mlx5.h
> @@ -51,6 +51,7 @@ enum mlx5_ipool_index {
>  	MLX5_IPOOL_TAG, /* Pool for tag resource. */
>  	MLX5_IPOOL_PORT_ID, /* Pool for port id resource. */
>  	MLX5_IPOOL_JUMP, /* Pool for jump resource. */
> +	MLX5_IPOOL_SAMPLE, /* Pool for sample resource. */
>  #endif
>  	MLX5_IPOOL_MTR, /* Pool for meter resource. */
>  	MLX5_IPOOL_MCP, /* Pool for metadata resource. */
> @@ -510,6 +511,7 @@ struct mlx5_flow_tbl_resource {
>  /* Tables for metering splits should be added here. */
>  #define MLX5_MAX_TABLES_EXTERNAL (MLX5_MAX_TABLES - 3)
>  #define MLX5_MAX_TABLES_FDB UINT16_MAX
> +#define MLX5_FLOW_TABLE_FACTOR 10
> 
>  /* ID generation structure. */
>  struct mlx5_flow_id_pool {
> @@ -558,6 +560,7 @@ struct mlx5_dev_ctx_shared {
>  	struct mlx5_hlist *tag_table;
>  	uint32_t port_id_action_list; /* List of port ID actions. */
>  	uint32_t push_vlan_action_list; /* List of push VLAN actions. */
> +	uint32_t sample_action_list; /* List of sample actions. */
>  	struct mlx5_flow_counter_mng cmng; /* Counters management
> structure. */
>  	struct mlx5_indexed_pool *ipool[MLX5_IPOOL_MAX];
>  	/* Memory Pool for mlx5 flow resources. */
> diff --git a/drivers/net/mlx5/mlx5_flow.c b/drivers/net/mlx5/mlx5_flow.c
> index 3a48b89..7c65a9a 100644
> --- a/drivers/net/mlx5/mlx5_flow.c
> +++ b/drivers/net/mlx5/mlx5_flow.c
> @@ -360,6 +360,8 @@ struct mlx5_flow_tunnel_info {
>  		return REG_B;
>  	case MLX5_HAIRPIN_TX:
>  		return REG_A;
> +	case MLX5_SAMPLE_FDB:
> +		return REG_C_0;
>  	case MLX5_METADATA_RX:
>  		switch (config->dv_xmeta_en) {
>  		case MLX5_XMETA_MODE_LEGACY:
> @@ -3878,6 +3880,137 @@ uint32_t mlx5_flow_adjust_priority(struct
> rte_eth_dev *dev, int32_t priority,
>  	return 0;
>  }
> 
> +
> +/**
> + * Check the match action from the action list.
> + *
> + * @param[in] actions
> + *   Pointer to the list of actions.
> + * @param[in] action
> + *   The action to be check if exist.
> + *
> + * @return
> + *   > 0 the total number of actions.
> + *   0 if not found match action in action list.
> + */
> +static int
> +flow_check_match_action(const struct rte_flow_action actions[],
> +					enum rte_flow_action_type action)
> +{
> +	int actions_n = 0;
> +	int flag = 0;
> +
> +	for (; actions->type != RTE_FLOW_ACTION_TYPE_END; actions++) {
> +		if (actions->type == action)
> +			flag = 1;
> +		actions_n++;
> +	}
> +	/* Count RTE_FLOW_ACTION_TYPE_END. */
> +	return flag ? actions_n + 1 : 0;
> +}
> +
> +/**
> + * Split the sample flow.
> + *
> + * As sample flow will split to two sub flow, sample flow with
> + * sample action, the other actions will move to new suffix flow.
> + *
> + * Also add unique tag id with tag action in the sample flow,
> + * the same tag id will be as match in the suffix flow.
> + *
> + * @param dev
> + *   Pointer to Ethernet device.
> + * @param[in] attr
> + *   Flow rule attributes.
> + * @param[out] sfx_items
> + *   Suffix flow match items (list terminated by the END pattern item).
> + * @param[in] actions
> + *   Associated actions (list terminated by the END action).
> + * @param[out] actions_sfx
> + *   Suffix flow actions.
> + * @param[out] actions_pre
> + *   Prefix flow actions.
> + *
> + * @return
> + *   0 on success.


It looks like the function also returns the tag id.

> + */
> +static int
> +flow_sample_split_prep(struct rte_eth_dev *dev,
> +		 const struct rte_flow_attr *attr,
> +		 struct rte_flow_item sfx_items[],
> +		 const struct rte_flow_action actions[],
> +		 struct rte_flow_action actions_sfx[],
> +		 struct rte_flow_action actions_pre[])
> +{
> +	struct mlx5_rte_flow_action_set_tag *set_tag;
> +	struct mlx5_rte_flow_item_tag *tag_spec;
> +	struct mlx5_rte_flow_item_tag *tag_mask;
> +	struct rte_flow_item *tag_item;
> +	struct rte_flow_action *tag_action = NULL;
> +	bool pre_sample = true;
> +	struct rte_flow_error error;
> +	uint32_t tag_id;
> +
> +	/* Prepare the actions for prefix and suffix flow. */
> +	for (; actions->type != RTE_FLOW_ACTION_TYPE_END; actions++) {
> +		struct rte_flow_action **action_cur = NULL;
> +
> +		switch (actions->type) {
> +		case RTE_FLOW_ACTION_TYPE_SAMPLE:
> +			/* Add the extra tag action first. */
> +			tag_action = actions_pre;
> +			tag_action->type = (enum rte_flow_action_type)
> +
> 	MLX5_RTE_FLOW_ACTION_TYPE_TAG;
> +			actions_pre++;
> +			break;
> +		case RTE_FLOW_ACTION_TYPE_JUMP:
> +		case RTE_FLOW_ACTION_TYPE_METER:
> +			action_cur = &actions_sfx;
> +			break;
> +		default:
> +			break;
> +		}
> +		if (pre_sample && !action_cur)
> +			action_cur = &actions_pre;
> +		else
> +			action_cur = &actions_sfx;
> +		memcpy(*action_cur, actions, sizeof(struct rte_flow_action));
> +		(*action_cur)++;
> +		if (actions->type == RTE_FLOW_ACTION_TYPE_SAMPLE)
> +			pre_sample = false;
> +	}
> +	/* Add end action to the actions. */
> +	actions_sfx->type = RTE_FLOW_ACTION_TYPE_END;
> +	actions_pre->type = RTE_FLOW_ACTION_TYPE_END;
> +	actions_pre++;
> +	/* Set the tag. */
> +	set_tag = (void *)actions_pre;
> +	set_tag->id = mlx5_flow_get_reg_id(dev, attr->transfer ?
> +			MLX5_SAMPLE_FDB : MLX5_APP_TAG, 0, &error);
> +	tag_id = flow_qrss_get_id(dev);
> +	set_tag->data = tag_id;
> +	assert(tag_action);
> +	tag_action->conf = set_tag;
> +	/* Prepare the suffix subflow items. */
> +	if (sfx_items) {
> +		tag_item = sfx_items++;
> +		sfx_items->type = RTE_FLOW_ITEM_TYPE_END;
> +		sfx_items++;
> +		tag_spec = (struct mlx5_rte_flow_item_tag *)sfx_items;
> +		tag_spec->data = tag_id;
> +		tag_spec->id = set_tag->id;
> +		tag_mask = tag_spec + 1;
> +		tag_mask->data = UINT32_MAX;
> +		tag_mask->id = UINT16_MAX;
> +		tag_item->type = (enum rte_flow_item_type)
> +				MLX5_RTE_FLOW_ITEM_TYPE_TAG;
> +		tag_item->spec = tag_spec;
> +		tag_item->last = NULL;
> +		tag_item->mask = tag_mask;
> +	}
> +	return tag_id;
> +}
> +
>  /**
>   * The splitting for metadata feature.
>   *
> @@ -4137,6 +4270,7 @@ uint32_t mlx5_flow_adjust_priority(struct
> rte_eth_dev *dev, int32_t priority,
>  static int
>  flow_create_split_meter(struct rte_eth_dev *dev,
>  			   struct rte_flow *flow,
> +			   uint64_t prefix_layers,
>  			   const struct rte_flow_attr *attr,
>  			   const struct rte_flow_item items[],
>  			   const struct rte_flow_action actions[],
> @@ -4183,8 +4317,9 @@ uint32_t mlx5_flow_adjust_priority(struct
> rte_eth_dev *dev, int32_t priority,
>  			goto exit;
>  		}
>  		/* Add the prefix subflow. */
> -		ret = flow_create_split_inner(dev, flow, &dev_flow, 0, attr,
> -					      items, pre_actions, external,
> +		ret = flow_create_split_inner(dev, flow, &dev_flow,
> +					      prefix_layers, attr, items,
> +					      pre_actions, external,
>  					      flow_idx, error);
>  		if (ret) {
>  			ret = -rte_errno;
> @@ -4199,7 +4334,7 @@ uint32_t mlx5_flow_adjust_priority(struct
> rte_eth_dev *dev, int32_t priority,
>  	/* Add the prefix subflow. */
>  	ret = flow_create_split_metadata(dev, flow, dev_flow ?
> 
> flow_get_prefix_layer_flags(dev_flow) :
> -					 0, &sfx_attr,
> +					 prefix_layers, &sfx_attr,
>  					 sfx_items ? sfx_items : items,
>  					 sfx_actions ? sfx_actions : actions,
>  					 external, flow_idx, error);
> @@ -4210,6 +4345,117 @@ uint32_t mlx5_flow_adjust_priority(struct
> rte_eth_dev *dev, int32_t priority,
>  }
> 
>  /**
> + * The splitting for sample feature.
> + *
> + * The sample flow will be split to two flows as prefix and
> + * suffix flow.
> + *
> + * @param dev
> + *   Pointer to Ethernet device.
> + * @param[in] flow
> + *   Parent flow structure pointer.
> + * @param[in] attr
> + *   Flow rule attributes.
> + * @param[in] items
> + *   Pattern specification (list terminated by the END pattern item).
> + * @param[in] actions
> + *   Associated actions (list terminated by the END action).
> + * @param[in] external
> + *   This flow rule is created by request external to PMD.
> + * @param[in] flow_idx
> + *   This memory pool index to the flow.
> + * @param[out] error
> + *   Perform verbose error reporting if not NULL.
> + * @return
> + *   0 on success, negative value otherwise
> + */
> +static int
> +flow_create_split_sample(struct rte_eth_dev *dev,
> +			   struct rte_flow *flow,
> +			   const struct rte_flow_attr *attr,
> +			   const struct rte_flow_item items[],
> +			   const struct rte_flow_action actions[],
> +			   bool external, uint32_t flow_idx,
> +			   struct rte_flow_error *error)
> +{
> +	struct mlx5_priv *priv = dev->data->dev_private;
> +	struct rte_flow_action *sfx_actions = NULL;
> +	struct rte_flow_action *pre_actions = NULL;
> +	struct rte_flow_item *sfx_items = NULL;
> +	struct mlx5_flow *dev_flow = NULL;
> +	struct rte_flow_attr sfx_attr = *attr;
> +	struct mlx5_flow_dv_sample_resource *sample_res;
> +	struct mlx5_flow_tbl_data_entry *sfx_tbl_data;
> +	struct mlx5_flow_tbl_resource *sfx_tbl;
> +	union mlx5_flow_tbl_key sfx_table_key;
> +	size_t act_size;
> +	size_t item_size;
> +	uint32_t tag_id = 0;
> +	int actions_n = 0;
> +	int ret = 0;
> +
> +	if (priv->sampler_en)
> +		actions_n = flow_check_match_action(actions,
> +					RTE_FLOW_ACTION_TYPE_SAMPLE);
> +	if (actions_n) {
> +		/* The prefix actions must includes sample, tag, end. */
> +		act_size = sizeof(struct rte_flow_action) * (actions_n * 2) +
> +			   sizeof(struct mlx5_rte_flow_action_set_tag);
> +		/* tag, end. */
> +#define SAMPLE_SUFFIX_ITEM 2
> +		item_size = sizeof(struct rte_flow_item) *
> SAMPLE_SUFFIX_ITEM +
> +			    sizeof(struct mlx5_rte_flow_item_tag) * 2;
> +		sfx_actions = rte_zmalloc(__func__, (act_size + item_size), 0);
> +		if (!sfx_actions)
> +			return rte_flow_error_set(error, ENOMEM,
> +
> RTE_FLOW_ERROR_TYPE_ACTION,
> +						  NULL, "no memory to split "
> +						  "sample flow");
> +		if (!attr->transfer)
> +			sfx_items = (struct rte_flow_item *)((char
> *)sfx_actions
> +					+ act_size);
> +		pre_actions = sfx_actions + actions_n;
> +		tag_id = flow_sample_split_prep(dev, attr, sfx_items,
> +						   actions, sfx_actions,
> +						   pre_actions);
> +		if (!tag_id) {
> +			ret = -rte_errno;
> +			goto exit;
> +		}
> +		/* Add the prefix subflow. */
> +		ret = flow_create_split_inner(dev, flow, &dev_flow, 0, attr,
> +					      items, pre_actions, external,
> +					      flow_idx, error);
> +		if (ret) {
> +			ret = -rte_errno;
> +			goto exit;
> +		}
> +		dev_flow->handle->split_flow_id = tag_id;
> +		/* Set the sfx group attr. */
> +		sample_res = (struct mlx5_flow_dv_sample_resource *)
> +					dev_flow->dv.sample_res;
> +		sfx_tbl = (struct mlx5_flow_tbl_resource *)
> +					sample_res->normal_path_tbl;
> +		sfx_tbl_data = container_of(sfx_tbl,
> +					struct mlx5_flow_tbl_data_entry, tbl);
> +		sfx_table_key.v64 = sfx_tbl_data->entry.key;
> +		sfx_attr.group = sfx_attr.transfer ?
> +					(sfx_table_key.table_id - 1) :
> +					sfx_table_key.table_id;
> +	}
> +	/* Add the suffix subflow. */
> +	ret = flow_create_split_meter(dev, flow, dev_flow ?
> +				 flow_get_prefix_layer_flags(dev_flow) : 0,
> +				 &sfx_attr, sfx_items ? sfx_items : items,
> +				 sfx_actions ? sfx_actions : actions,
> +				 external, flow_idx, error);
> +exit:
> +	if (sfx_actions)
> +		rte_free(sfx_actions);
> +	return ret;
> +}
> +
> +/**
>   * Split the flow to subflow set. The splitters might be linked
>   * in the chain, like this:
>   * flow_create_split_outer() calls:
> @@ -4257,7 +4503,7 @@ uint32_t mlx5_flow_adjust_priority(struct
> rte_eth_dev *dev, int32_t priority,
>  {
>  	int ret;
> 
> -	ret = flow_create_split_meter(dev, flow, attr, items,
> +	ret = flow_create_split_sample(dev, flow, attr, items,
>  					 actions, external, flow_idx, error);
>  	MLX5_ASSERT(ret <= 0);
>  	return ret;
> diff --git a/drivers/net/mlx5/mlx5_flow.h b/drivers/net/mlx5/mlx5_flow.h
> index 902380b..941de5f 100644
> --- a/drivers/net/mlx5/mlx5_flow.h
> +++ b/drivers/net/mlx5/mlx5_flow.h
> @@ -79,6 +79,7 @@ enum mlx5_feature_name {
>  	MLX5_COPY_MARK,
>  	MLX5_MTR_COLOR,
>  	MLX5_MTR_SFX,
> +	MLX5_SAMPLE_FDB,
>  };
> 
>  /* Pattern outer Layer bits. */
> @@ -498,6 +499,38 @@ struct mlx5_flow_tbl_data_entry {
>  	uint32_t idx; /**< index for the indexed mempool. */
>  };
> 
> +/* Sub rdma-core actions list. */
> +struct mlx5_flow_sub_actions_list {
> +	uint32_t actions_num; /**< Number of sample actions. */
> +	uint64_t action_flags;
> +	void *dr_queue_action;
> +	void *dr_tag_action;
> +	void *dr_cnt_action;
> +};
> +
> +/* Sample sub-actions resource list. */
> +struct mlx5_flow_sub_actions_idx {
> +	uint32_t rix_hrxq; /**< Hash Rx queue object index. */
> +	uint32_t rix_tag; /**< Index to the tag action. */
> +	uint32_t cnt;
> +};
> +
> +/* Sample action resource structure. */
> +struct mlx5_flow_dv_sample_resource {
> +	ILIST_ENTRY(uint32_t)next; /**< Pointer to next element. */
> +	rte_atomic32_t refcnt; /**< Reference counter. */
> +	void *verbs_action; /**< Verbs sample action object. */
> +	uint8_t ft_type; /** Flow Table Type */
> +	uint32_t ft_id; /** Flow Table Level */
> +	void *normal_path_tbl; /** Flow Table pointer */
> +	void *default_miss; /** default_miss dr_action. */
> +	uint32_t ratio;   /** Sample Ratio */
> +	struct mlx5_flow_sub_actions_idx sample_idx;
> +	/**< Action index resources. */
> +	struct mlx5_flow_sub_actions_list sample_act;
> +	/**< Action resources. */
> +};
> +
>  /* Verbs specification header. */
>  struct ibv_spec_header {
>  	enum ibv_flow_spec_type type;
> @@ -526,6 +559,8 @@ struct mlx5_flow_handle_dv {
>  	/**< Index to push VLAN action resource in cache. */
>  	uint32_t rix_tag;
>  	/**< Index to the tag action. */
> +	uint32_t rix_sample;
> +	/**< Index to sample action resource in cache. */
>  } __rte_packed;
> 
>  /** Device flow handle structure: used both for creating & destroying. */
> @@ -589,6 +624,8 @@ struct mlx5_flow_dv_workspace {
>  	/**< Pointer to the jump action resource. */
>  	struct mlx5_flow_dv_match_params value;
>  	/**< Holds the value that the packet is compared to. */
> +	struct mlx5_flow_dv_sample_resource *sample_res;
> +	/**< Pointer to the sample action resource. */
>  };
> 
>  /*
> --
> 1.8.3.1