Re: [dpdk-dev] [PATCH] net/mlx5: optimize memory of mlx5 flow meter

[Raslan Darawsheh <rasland@mellanox.com>]

Hi,

> -----Original Message-----
> From: dev <dev-bounces@dpdk.org> On Behalf Of Wentao Cui
> Sent: Monday, April 20, 2020 5:40 AM
> To: Matan Azrad <matan@mellanox.com>; Shahaf Shuler
> <shahafs@mellanox.com>; Slava Ovsiienko <viacheslavo@mellanox.com>
> Cc: dev@dpdk.org
> Subject: [dpdk-dev] [PATCH] net/mlx5: optimize memory of mlx5 flow meter
> 
> This commit focus on flow meter data structures
> optimization: mlx5_flow_meter.
> 
> Optimize memory consumption of flow meter data structure.
> Reorganize flow meter data structure,delete unnecessary
> data fields.
> 
> Signed-off-by: Wentao Cui <wentaoc@mellanox.com>
> ---
>  drivers/net/mlx5/mlx5_flow.h       | 37 ++++++++++++++++++++++++++----
>  drivers/net/mlx5/mlx5_flow_dv.c    |  8 +++----
>  drivers/net/mlx5/mlx5_flow_meter.c | 26 ++++++++++++---------
>  3 files changed, 52 insertions(+), 19 deletions(-)
> 
> diff --git a/drivers/net/mlx5/mlx5_flow.h b/drivers/net/mlx5/mlx5_flow.h
> index d697eba0e9..74017cfa31 100644
> --- a/drivers/net/mlx5/mlx5_flow.h
> +++ b/drivers/net/mlx5/mlx5_flow.h
> @@ -709,12 +709,41 @@ struct mlx5_flow_meter {
>  	uint32_t idx; /* Index to meter object. */
>  	uint32_t meter_id;
>  	/**< Meter id. */
> -	struct rte_mtr_params params;
> -	/**< Meter rule parameters. */
>  	struct mlx5_flow_meter_profile *profile;
>  	/**< Meter profile parameters. */
> -	struct rte_flow_attr attr;
> -	/**< Flow attributes. */
> +
> +	/** Policer actions (per meter output color). */
> +	enum rte_mtr_policer_action action[RTE_COLORS];
> +
> +	/** Set of stats counters to be enabled.
> +	 * @see enum rte_mtr_stats_type
> +	 */
> +	uint64_t stats_mask;
> +
> +	/**< Rule applies to ingress traffic. */
> +	uint32_t ingress:1;
> +
> +	/**< Rule applies to egress traffic. */
> +	uint32_t egress:1;
> +	/**
> +	 * Instead of simply matching the properties of traffic as it would
> +	 * appear on a given DPDK port ID, enabling this attribute transfers
> +	 * a flow rule to the lowest possible level of any device endpoints
> +	 * found in the pattern.
> +	 *
> +	 * When supported, this effectively enables an application to
> +	 * re-route traffic not necessarily intended for it (e.g. coming
> +	 * from or addressed to different physical ports, VFs or
> +	 * applications) at the device level.
> +	 *
> +	 * It complements the behavior of some pattern items such as
> +	 * RTE_FLOW_ITEM_TYPE_PHY_PORT and is meaningless without
> them.
> +	 *
> +	 * When transferring flow rules, ingress and egress attributes keep
> +	 * their original meaning, as if processing traffic emitted or
> +	 * received by the application.
> +	 */
> +	uint32_t transfer:1;
>  	struct mlx5_meter_domains_infos *mfts;
>  	/**< Flow table created for this meter. */
>  	struct mlx5_flow_policer_stats policer_stats;
> diff --git a/drivers/net/mlx5/mlx5_flow_dv.c
> b/drivers/net/mlx5/mlx5_flow_dv.c
> index cfc911c9ca..b552b0ea7c 100644
> --- a/drivers/net/mlx5/mlx5_flow_dv.c
> +++ b/drivers/net/mlx5/mlx5_flow_dv.c
> @@ -3707,9 +3707,9 @@ mlx5_flow_validate_action_meter(struct
> rte_eth_dev *dev,
>  		return rte_flow_error_set(error, EINVAL,
>  					  RTE_FLOW_ERROR_TYPE_ACTION,
> NULL,
>  					  "Meter not found");
> -	if (fm->ref_cnt && (!(fm->attr.transfer == attr->transfer ||
> -	      (!fm->attr.ingress && !attr->ingress && attr->egress) ||
> -	      (!fm->attr.egress && !attr->egress && attr->ingress))))
> +	if (fm->ref_cnt && (!(fm->transfer == attr->transfer ||
> +	      (!fm->ingress && !attr->ingress && attr->egress) ||
> +	      (!fm->egress && !attr->egress && attr->ingress))))
>  		return rte_flow_error_set(error, EINVAL,
>  					  RTE_FLOW_ERROR_TYPE_ACTION,
> NULL,
>  					  "Flow attributes are either invalid "
> @@ -9008,7 +9008,7 @@ flow_dv_create_policer_forward_rule(struct
> mlx5_flow_meter *fm,
>  				       rte_col_2_mlx5_col(i), UINT8_MAX);
>  		if (mtb->count_actns[i])
>  			actions[j++] = mtb->count_actns[i];
> -		if (fm->params.action[i] == MTR_POLICER_ACTION_DROP)
> +		if (fm->action[i] == MTR_POLICER_ACTION_DROP)
>  			actions[j++] = mtb->drop_actn;
>  		else
>  			actions[j++] = dtb->jump_actn;
> diff --git a/drivers/net/mlx5/mlx5_flow_meter.c
> b/drivers/net/mlx5/mlx5_flow_meter.c
> index b5fbf5d316..08f7dc8d11 100644
> --- a/drivers/net/mlx5/mlx5_flow_meter.c
> +++ b/drivers/net/mlx5/mlx5_flow_meter.c
> @@ -55,8 +55,8 @@ mlx5_flow_meter_action_create(struct mlx5_priv *priv,
>  	MLX5_SET(flow_meter_parameters,
>  		 attr, ebs_mantissa, srtcm->ebs_mantissa);
>  	mtr_init.next_table =
> -		fm->attr.transfer ? fm->mfts->transfer.tbl->obj :
> -		    fm->attr.egress ? fm->mfts->egress.tbl->obj :
> +		fm->transfer ? fm->mfts->transfer.tbl->obj :
> +		    fm->egress ? fm->mfts->egress.tbl->obj :
>  				       fm->mfts->ingress.tbl->obj;
>  	mtr_init.reg_c_index = priv->mtr_color_reg - REG_C_0;
>  	mtr_init.flow_meter_parameter = fm->mfts->fmp;
> @@ -657,7 +657,9 @@ mlx5_flow_meter_create(struct rte_eth_dev *dev,
> uint32_t meter_id,
>  	/* Fill the flow meter parameters. */
>  	fm->meter_id = meter_id;
>  	fm->profile = fmp;
> -	fm->params = *params;
> +	memcpy(fm->action, params->action, sizeof(params->action));
> +	fm->stats_mask = params->stats_mask;
> +
>  	/* Alloc policer counters. */
>  	for (i = 0; i < RTE_DIM(fm->policer_stats.cnt); i++) {
>  		fm->policer_stats.cnt[i] = mlx5_counter_alloc(dev);
> @@ -1050,7 +1052,7 @@ mlx5_flow_meter_stats_read(struct rte_eth_dev
> *dev,
>  						 &bytes);
>  			if (ret)
>  				goto error;
> -			if (fm->params.action[i] ==
> MTR_POLICER_ACTION_DROP) {
> +			if (fm->action[i] == MTR_POLICER_ACTION_DROP) {
>  				pkts_dropped += pkts;
>  				bytes_dropped += bytes;
>  			}
> @@ -1181,7 +1183,9 @@ mlx5_flow_meter_attach(struct mlx5_priv *priv,
> uint32_t meter_id,
>  	}
>  	if (!fm->ref_cnt++) {
>  		MLX5_ASSERT(!fm->mfts->meter_action);
> -		fm->attr = *attr;
> +		fm->ingress = attr->ingress;
> +		fm->egress = attr->egress;
> +		fm->transfer = attr->transfer;
>  		/* This also creates the meter object. */
>  		fm->mfts->meter_action =
> mlx5_flow_meter_action_create(priv,
>  								       fm);
> @@ -1189,9 +1193,9 @@ mlx5_flow_meter_attach(struct mlx5_priv *priv,
> uint32_t meter_id,
>  			goto error_detach;
>  	} else {
>  		MLX5_ASSERT(fm->mfts->meter_action);
> -		if (attr->transfer != fm->attr.transfer ||
> -		    attr->ingress != fm->attr.ingress ||
> -		    attr->egress != fm->attr.egress) {
> +		if (attr->transfer != fm->transfer ||
> +		    attr->ingress != fm->ingress ||
> +		    attr->egress != fm->egress) {
>  			DRV_LOG(ERR, "meter I/O attributes do not "
>  				"match flow I/O attributes.");
>  			goto error_detach;
> @@ -1216,15 +1220,15 @@ mlx5_flow_meter_attach(struct mlx5_priv *priv,
> uint32_t meter_id,
>  void
>  mlx5_flow_meter_detach(struct mlx5_flow_meter *fm)
>  {
> -	const struct rte_flow_attr attr = { 0 };
> -
>  	MLX5_ASSERT(fm->ref_cnt);
>  	if (--fm->ref_cnt)
>  		return;
>  	if (fm->mfts->meter_action)
>  		mlx5_glue->destroy_flow_action(fm->mfts->meter_action);
>  	fm->mfts->meter_action = NULL;
> -	fm->attr = attr;
> +	fm->ingress = 0;
> +	fm->egress = 0;
> +	fm->transfer = 0;
>  }
> 
>  /**
> --
> 2.21.0


Patch applied to next-net-mlx,

Kindest regards,
Raslan Darawsheh