Re: [dpdk-dev] [PATCH] net/mlx5: make FDB default rule optional

[Kevin Traynor <ktraynor@redhat.com>]

On 30/01/2020 13:06, Raslan Darawsheh wrote:
> Hi,
>> -----Original Message-----
>> From: dev <dev-bounces@dpdk.org> On Behalf Of Matan Azrad
>> Sent: Tuesday, January 28, 2020 7:07 PM
>> To: Slava Ovsiienko <viacheslavo@mellanox.com>
>> Cc: dev@dpdk.org; stable@dpdk.org
>> Subject: [dpdk-dev] [PATCH] net/mlx5: make FDB default rule optional
>>
>> There are RDMA-CORE versions which are not supported multi-table for
>> some Mellanox mlx5 devices.
>>
>> Hence, the optimization added in commit [1] which forwards all the FDB
>> traffic to table 1 cannot be configured.
>>
>> Make the above optimization optional:
>> Do not fail when either table 1 cannot be created or the jump rule
>> (all =>jump to table 1) is not configured successfully.
>> In this case, all the flows will be configured to table 0.
>>
>> [1] commit b67b4ecbde22 ("net/mlx5: skip table zero to improve
>> insertion rate")
>>

Hi, can we replace with correct 'Fixes:' tag before it gets to master to
be consistent and so it can be found/used by stable scripts. Thanks.

>> Cc: stable@dpdk.org
>>
>> Signed-off-by: Matan Azrad <matan@mellanox.com>
>> ---
>>  drivers/net/mlx5/mlx5.h         |  1 +
>>  drivers/net/mlx5/mlx5_flow.c    |  6 ++++--
>>  drivers/net/mlx5/mlx5_flow.h    |  4 ++--
>>  drivers/net/mlx5/mlx5_flow_dv.c | 11 ++++++-----
>>  drivers/net/mlx5/mlx5_trigger.c | 11 ++++++++---
>>  5 files changed, 21 insertions(+), 12 deletions(-)
>>
>> diff --git a/drivers/net/mlx5/mlx5.h b/drivers/net/mlx5/mlx5.h
>> index 5818349..1fc2063 100644
>> --- a/drivers/net/mlx5/mlx5.h
>> +++ b/drivers/net/mlx5/mlx5.h
>> @@ -792,6 +792,7 @@ struct mlx5_priv {
>>  	/* UAR same-page access control required in 32bit implementations.
>> */
>>  #endif
>>  	uint8_t skip_default_rss_reta; /* Skip configuration of default reta.
>> */
>> +	uint8_t fdb_def_rule; /* Whether fdb jump to table 1 is configured.
>> */
>>  };
>>
>>  #define PORT_ID(priv) ((priv)->dev_data->port_id)
>> diff --git a/drivers/net/mlx5/mlx5_flow.c b/drivers/net/mlx5/mlx5_flow.c
>> index 5c9fea6..ffaf8a2 100644
>> --- a/drivers/net/mlx5/mlx5_flow.c
>> +++ b/drivers/net/mlx5/mlx5_flow.c
>> @@ -5575,6 +5575,8 @@ struct mlx5_flow_counter *
>>   *   Value is part of flow rule created by request external to PMD.
>>   * @param[in] group
>>   *   rte_flow group index value.
>> + * @param[out] fdb_def_rule
>> + *   Whether fdb jump to table 1 is configured.
>>   * @param[out] table
>>   *   HW table value.
>>   * @param[out] error
>> @@ -5585,10 +5587,10 @@ struct mlx5_flow_counter *
>>   */
>>  int
>>  mlx5_flow_group_to_table(const struct rte_flow_attr *attributes, bool
>> external,
>> -			 uint32_t group, uint32_t *table,
>> +			 uint32_t group, bool fdb_def_rule, uint32_t *table,
>>  			 struct rte_flow_error *error)
>>  {
>> -	if (attributes->transfer && external) {
>> +	if (attributes->transfer && external && fdb_def_rule) {
>>  		if (group == UINT32_MAX)
>>  			return rte_flow_error_set
>>  						(error, EINVAL,
>> diff --git a/drivers/net/mlx5/mlx5_flow.h b/drivers/net/mlx5/mlx5_flow.h
>> index 39be5ba..82b4292 100644
>> --- a/drivers/net/mlx5/mlx5_flow.h
>> +++ b/drivers/net/mlx5/mlx5_flow.h
>> @@ -753,8 +753,8 @@ struct mlx5_flow_driver_ops {
>>  uint32_t mlx5_flow_id_release(struct mlx5_flow_id_pool *pool,
>>  			      uint32_t id);
>>  int mlx5_flow_group_to_table(const struct rte_flow_attr *attributes,
>> -			     bool external, uint32_t group, uint32_t *table,
>> -			     struct rte_flow_error *error);
>> +			     bool external, uint32_t group, bool fdb_def_rule,
>> +			     uint32_t *table, struct rte_flow_error *error);
>>  uint64_t mlx5_flow_hashfields_adjust(struct mlx5_flow *dev_flow, int
>> tunnel,
>>  				     uint64_t layer_types,
>>  				     uint64_t hash_fields);
>> diff --git a/drivers/net/mlx5/mlx5_flow_dv.c
>> b/drivers/net/mlx5/mlx5_flow_dv.c
>> index b90734e..d57d360 100644
>> --- a/drivers/net/mlx5/mlx5_flow_dv.c
>> +++ b/drivers/net/mlx5/mlx5_flow_dv.c
>> @@ -3357,7 +3357,7 @@ struct field_modify_info modify_tcp[] = {
>>  	target_group =
>>  		((const struct rte_flow_action_jump *)action->conf)->group;
>>  	ret = mlx5_flow_group_to_table(attributes, external, target_group,
>> -				       &table, error);
>> +				       true, &table, error);
>>  	if (ret)
>>  		return ret;
>>  	if (attributes->group == target_group)
>> @@ -4334,7 +4334,7 @@ struct field_modify_info modify_tcp[] = {
>>  	int ret;
>>
>>  	ret = mlx5_flow_group_to_table(attributes, external,
>> -				       attributes->group,
>> +				       attributes->group, !!priv->fdb_def_rule,
>>  				       &table, error);
>>  	if (ret)
>>  		return ret;
>> @@ -7011,7 +7011,7 @@ struct field_modify_info modify_tcp[] = {
>>  	mhdr_res->ft_type = attr->egress ?
>> MLX5DV_FLOW_TABLE_TYPE_NIC_TX :
>>
>> MLX5DV_FLOW_TABLE_TYPE_NIC_RX;
>>  	ret = mlx5_flow_group_to_table(attr, dev_flow->external, attr-
>>> group,
>> -				       &table, error);
>> +				       !!priv->fdb_def_rule, &table, error);
>>  	if (ret)
>>  		return ret;
>>  	dev_flow->group = table;
>> @@ -7279,8 +7279,9 @@ struct field_modify_info modify_tcp[] = {
>>  		case RTE_FLOW_ACTION_TYPE_JUMP:
>>  			jump_data = action->conf;
>>  			ret = mlx5_flow_group_to_table(attr, dev_flow-
>>> external,
>> -						       jump_data->group,
>> &table,
>> -						       error);
>> +						       jump_data->group,
>> +						       !!priv->fdb_def_rule,
>> +						       &table, error);
>>  			if (ret)
>>  				return ret;
>>  			tbl = flow_dv_tbl_resource_get(dev, table,
>> diff --git a/drivers/net/mlx5/mlx5_trigger.c
>> b/drivers/net/mlx5/mlx5_trigger.c
>> index ab6937a..7e12cd5 100644
>> --- a/drivers/net/mlx5/mlx5_trigger.c
>> +++ b/drivers/net/mlx5/mlx5_trigger.c
>> @@ -423,9 +423,14 @@
>>  		}
>>  		mlx5_txq_release(dev, i);
>>  	}
>> -	if (priv->config.dv_esw_en && !priv->config.vf)
>> -		if (!mlx5_flow_create_esw_table_zero_flow(dev))
>> -			goto error;
>> +	if (priv->config.dv_esw_en && !priv->config.vf) {
>> +		if (mlx5_flow_create_esw_table_zero_flow(dev))
>> +			priv->fdb_def_rule = 1;
>> +		else
>> +			DRV_LOG(INFO, "port %u FDB default rule cannot
>> be"
>> +				" configured - only Eswitch group 0 flows are"
>> +				" supported.", dev->data->port_id);
>> +	}
>>  	if (priv->isolated)
>>  		return 0;
>>  	if (dev->data->promiscuous) {
>> --
>> 1.8.3.1
> 
> 
> Patch applied to next-net-mlx,
> 
> Kindest regards,
> Raslan Darawsheh
>