[PATCH net-next] net/mlx5e: avoid check the hw_stats of flow_action for FT flow

[PATCH net-next] net/mlx5e: avoid check the hw_stats of flow_action for FT flow

[wenxu]

From: wenxu <wenxu@ucloud.cn>

The hw_stats in flow_action can't be supported in nftable
flowtables. This check will lead the nft flowtable offload
failed. So don't check the hw_stats of flow_action for FT
flow.

Signed-off-by: wenxu <wenxu@ucloud.cn>
---
 drivers/net/ethernet/mellanox/mlx5/core/en_tc.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/drivers/net/ethernet/mellanox/mlx5/core/en_tc.c b/drivers/net/ethernet/mellanox/mlx5/core/en_tc.c
index 901b5fa..4666015 100644
--- a/drivers/net/ethernet/mellanox/mlx5/core/en_tc.c
+++ b/drivers/net/ethernet/mellanox/mlx5/core/en_tc.c
@@ -3703,7 +3703,8 @@ static int parse_tc_fdb_actions(struct mlx5e_priv *priv,
 	if (!flow_action_has_entries(flow_action))
 		return -EINVAL;
 
-	if (!flow_action_hw_stats_check(flow_action, extack,
+	if (!ft_flow &&
+	    !flow_action_hw_stats_check(flow_action, extack,
 					FLOW_ACTION_HW_STATS_DELAYED_BIT))
 		return -EOPNOTSUPP;
 
-- 
1.8.3.1

Re: [PATCH net-next] net/mlx5e: avoid check the hw_stats of flow_action for FT flow

[Saeed Mahameed]

On Sun, 2020-03-29 at 14:56 +0800, wenxu@ucloud.cn wrote:
> From: wenxu <wenxu@ucloud.cn>
> 
> The hw_stats in flow_action can't be supported in nftable
> flowtables. This check will lead the nft flowtable offload
> failed. So don't check the hw_stats of flow_action for FT
> flow.
> 

This looks like a work around not a solution .. if the user requested a
hw_stats action that the hw can't support, no matter what the request
is, we should fail it even if it was for ft offloads.

if it is not support by nftable, then the caller shouldn't ask for
hw_stats action in first place.

> Signed-off-by: wenxu <wenxu@ucloud.cn>
> ---
>  drivers/net/ethernet/mellanox/mlx5/core/en_tc.c | 3 ++-
>  1 file changed, 2 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/net/ethernet/mellanox/mlx5/core/en_tc.c
> b/drivers/net/ethernet/mellanox/mlx5/core/en_tc.c
> index 901b5fa..4666015 100644
> --- a/drivers/net/ethernet/mellanox/mlx5/core/en_tc.c
> +++ b/drivers/net/ethernet/mellanox/mlx5/core/en_tc.c
> @@ -3703,7 +3703,8 @@ static int parse_tc_fdb_actions(struct
> mlx5e_priv *priv,
>  	if (!flow_action_has_entries(flow_action))
>  		return -EINVAL;
>  
> -	if (!flow_action_hw_stats_check(flow_action, extack,
> +	if (!ft_flow &&
> +	    !flow_action_hw_stats_check(flow_action, extack,
>  					FLOW_ACTION_HW_STATS_DELAYED_BI
> T))
>  		return -EOPNOTSUPP;
>  

Re: [PATCH net-next] net/mlx5e: avoid check the hw_stats of flow_action for FT flow

[wenxu]

On 4/3/2020 10:59 AM, Saeed Mahameed wrote:
> On Sun, 2020-03-29 at 14:56 +0800, wenxu@ucloud.cn wrote:
>> From: wenxu <wenxu@ucloud.cn>
>>
>> The hw_stats in flow_action can't be supported in nftable
>> flowtables. This check will lead the nft flowtable offload
>> failed. So don't check the hw_stats of flow_action for FT
>> flow.
>>
> This looks like a work around not a solution .. if the user requested a
> hw_stats action that the hw can't support, no matter what the request
> is, we should fail it even if it was for ft offloads.
>
> if it is not support by nftable, then the caller shouldn't ask for
> hw_stats action in first place.

The action entries in nft didn't set the hw_stats and the vlaue is 0.


The following function check the hw_stats should contain FLOW_ACTION_HW_STATS_DELAYED_BIT.

flow_action_hw_stats_check(flow_action, extack, FLOW_ACTION_HW_STATS_DELAYED_BIT)



Maybe the following patch is better?


__flow_action_hw_stats_check(const struct flow_action *action,
                             struct netlink_ext_ack *extack,
                             bool check_allow_bit,
                             enum flow_action_hw_stats_bit allow_bit)
{
        const struct flow_action_entry *action_entry;

        if (!flow_action_has_entries(action))
                return true;
        if (!flow_action_mixed_hw_stats_check(action, extack))
                return false;
        action_entry = flow_action_first_entry_get(action);
        if (!check_allow_bit &&
            action_entry->hw_stats != FLOW_ACTION_HW_STATS_ANY) {
                NL_SET_ERR_MSG_MOD(extack, "Driver supports only default HW stats type \"any\"");
                return false;
-        } else if (check_allow_bit &&
+        } else if (check_allow_bit && action_entry->hw_stats &&
                   !(action_entry->hw_stats & BIT(allow_bit))) {
                NL_SET_ERR_MSG_MOD(extack, "Driver does not support selected HW stats type");
                return false;
        }   
        return true;
}




>> Signed-off-by: wenxu <wenxu@ucloud.cn>
>> ---
>>  drivers/net/ethernet/mellanox/mlx5/core/en_tc.c | 3 ++-
>>  1 file changed, 2 insertions(+), 1 deletion(-)
>>
>> diff --git a/drivers/net/ethernet/mellanox/mlx5/core/en_tc.c
>> b/drivers/net/ethernet/mellanox/mlx5/core/en_tc.c
>> index 901b5fa..4666015 100644
>> --- a/drivers/net/ethernet/mellanox/mlx5/core/en_tc.c
>> +++ b/drivers/net/ethernet/mellanox/mlx5/core/en_tc.c
>> @@ -3703,7 +3703,8 @@ static int parse_tc_fdb_actions(struct
>> mlx5e_priv *priv,
>>  	if (!flow_action_has_entries(flow_action))
>>  		return -EINVAL;
>>  
>> -	if (!flow_action_hw_stats_check(flow_action, extack,
>> +	if (!ft_flow &&
>> +	    !flow_action_hw_stats_check(flow_action, extack,
>>  					FLOW_ACTION_HW_STATS_DELAYED_BI
>> T))
>>  		return -EOPNOTSUPP;
>>  

The size of ct offoad mlx5_flow_table in mlx5e driver

[wenxu]

Hi  Paul,


I have a question about the size of ct and ct nat flow table.


There are two global mlx5_flow_table tables ct and ct_nat for act_ct offload.


The ct and ct_nat flow table create through mlx5_esw_chains_create_global_table

and get the size through mlx5_esw_chains_get_avail_sz_from_pool(esw, POOL_NEXT_SIZE);


Firmware currently has 4 pool of 4 sizes that it supports (ESW_POOLS),

and a virtual memory region of 16M (ESW_SIZE).  It allocates up to 16M of each pool.


ESW_POOLS[] = { 4 * 1024 * 1024,
                             1 * 1024 * 1024,
                             64 * 1024,
                             128 };

So it means the biggest flow table size is 4M. The ct and ct_nat flowtable create in advance,

The size of ct and ct_nat is 4M.

It means there are almost 4M conntrack entry offload to the hardware?

The flow table map is fixed in the FW? And the size can be changed to 8M through the following？

ESW_POOLS[] = { 8 * 1024 * 1024,
                             1 * 1024 * 1024,
                             64 * 1024,
                             128 };


BR

wenxu

Re: The size of ct offoad mlx5_flow_table in mlx5e driver

[Oz Shlomo]

Hi Wenxu,

I'll reply on behalf of Paul

On 5/28/2020 7:02 AM, wenxu wrote:
> Hi  Paul,
> 
> 
> I have a question about the size of ct and ct nat flow table.
> 
> 
> There are two global mlx5_flow_table tables ct and ct_nat for act_ct offload.
> 
> 
> The ct and ct_nat flow table create through mlx5_esw_chains_create_global_table
> 
> and get the size through mlx5_esw_chains_get_avail_sz_from_pool(esw, POOL_NEXT_SIZE);
> 
> 
> Firmware currently has 4 pool of 4 sizes that it supports (ESW_POOLS),
> 
> and a virtual memory region of 16M (ESW_SIZE).  It allocates up to 16M of each pool.
> 
> 
> ESW_POOLS[] = { 4 * 1024 * 1024,
>                               1 * 1024 * 1024,
>                               64 * 1024,
>                               128 };
> 
> So it means the biggest flow table size is 4M. The ct and ct_nat flowtable create in advance,
> 
> The size of ct and ct_nat is 4M.
> 
> It means there are almost 4M conntrack entry offload to the hardware?

Yes, the conntrack table has 4M entries.


> 
> The flow table map is fixed in the FW? And the size can be changed to 8M through the following？
> 
> ESW_POOLS[] = { 8 * 1024 * 1024,
>                               1 * 1024 * 1024,
>                               64 * 1024,
>                               128 };

The size cannot be increased due to internal FW limitations.
We are currently working on an alternative design for increased scalability.


> 
> 
> BR
> 
> wenxu
> 
> 
> 
> 
> 
> 
> 
> 
>