[PATCH net-next 0/2] net/mlx5e: add nat support in ct_metadata

[PATCH net-next 0/2] net/mlx5e: add nat support in ct_metadata

[wenxu]

From: wenxu <wenxu@ucloud.cn>

Currently all the conntrack entry offfload rules will be add
in both ct and ct_nat flow table in the mlx5e driver. It is
not makesense.

This serise provide nat attribute in the ct_metadata action which
tell driver the rule should add to ct or ct_nat flow table 

wenxu (2):
  net/sched: act_ct: add nat attribute in ct_metadata
  net/mlx5e: add ct_metadata.nat support in ct offload

 drivers/net/ethernet/mellanox/mlx5/core/en/tc_ct.c | 34 ++++++++--------------
 include/net/flow_offload.h                         |  1 +
 net/sched/act_ct.c                                 |  1 +
 3 files changed, 14 insertions(+), 22 deletions(-)

-- 
1.8.3.1

[PATCH net-next 1/2] net/sched: act_ct: add nat attribute in ct_metadata

[wenxu]

From: wenxu <wenxu@ucloud.cn>

Add nat attribute in the ct_metadata action. This tell driver the offload
conntrack entry is nat one or not.

Signed-off-by: wenxu <wenxu@ucloud.cn>
---
 include/net/flow_offload.h | 1 +
 net/sched/act_ct.c         | 1 +
 2 files changed, 2 insertions(+)

diff --git a/include/net/flow_offload.h b/include/net/flow_offload.h
index 95d6337..e3f09dd 100644
--- a/include/net/flow_offload.h
+++ b/include/net/flow_offload.h
@@ -244,6 +244,7 @@ struct flow_action_entry {
 			unsigned long cookie;
 			u32 mark;
 			u32 labels[4];
+			bool nat;
 		} ct_metadata;
 		struct {				/* FLOW_ACTION_MPLS_PUSH */
 			u32		label;
diff --git a/net/sched/act_ct.c b/net/sched/act_ct.c
index 9adff83..f70ab543 100644
--- a/net/sched/act_ct.c
+++ b/net/sched/act_ct.c
@@ -183,6 +183,7 @@ static void tcf_ct_flow_table_add_action_meta(struct nf_conn *ct,
 					     IP_CT_ESTABLISHED_REPLY;
 	/* aligns with the CT reference on the SKB nf_ct_set */
 	entry->ct_metadata.cookie = (unsigned long)ct | ctinfo;
+	entry->ct_metadata.nat = ct->status & IPS_NAT_MASK;
 
 	act_ct_labels = entry->ct_metadata.labels;
 	ct_labels = nf_ct_labels_find(ct);
-- 
1.8.3.1

[PATCH net-next 2/2] net/mlx5e: add ct_metadata.nat support in ct offload

[wenxu]

From: wenxu <wenxu@ucloud.cn>

In the ct offload all the conntrack entry offload  rules 
will be add to both ct ft and ct_nat ft twice. 
It is not makesense. The ct_metadat.nat will tell driver
the rule should add to ct or ct_nat flow table 

Signed-off-by: wenxu <wenxu@ucloud.cn>
---
 drivers/net/ethernet/mellanox/mlx5/core/en/tc_ct.c | 34 ++++++++--------------
 1 file changed, 12 insertions(+), 22 deletions(-)

diff --git a/drivers/net/ethernet/mellanox/mlx5/core/en/tc_ct.c b/drivers/net/ethernet/mellanox/mlx5/core/en/tc_ct.c
index 995b2ef..02ecd24 100644
--- a/drivers/net/ethernet/mellanox/mlx5/core/en/tc_ct.c
+++ b/drivers/net/ethernet/mellanox/mlx5/core/en/tc_ct.c
@@ -88,7 +88,7 @@ struct mlx5_ct_entry {
 	struct mlx5_fc *counter;
 	unsigned long cookie;
 	unsigned long restore_cookie;
-	struct mlx5_ct_zone_rule zone_rules[2];
+	struct mlx5_ct_zone_rule zone_rule;
 };
 
 static const struct rhashtable_params cts_ht_params = {
@@ -238,10 +238,9 @@ struct mlx5_ct_entry {
 
 static void
 mlx5_tc_ct_entry_del_rule(struct mlx5_tc_ct_priv *ct_priv,
-			  struct mlx5_ct_entry *entry,
-			  bool nat)
+			  struct mlx5_ct_entry *entry)
 {
-	struct mlx5_ct_zone_rule *zone_rule = &entry->zone_rules[nat];
+	struct mlx5_ct_zone_rule *zone_rule = &entry->zone_rule;
 	struct mlx5_esw_flow_attr *attr = &zone_rule->attr;
 	struct mlx5_eswitch *esw = ct_priv->esw;
 
@@ -256,8 +255,7 @@ struct mlx5_ct_entry {
 mlx5_tc_ct_entry_del_rules(struct mlx5_tc_ct_priv *ct_priv,
 			   struct mlx5_ct_entry *entry)
 {
-	mlx5_tc_ct_entry_del_rule(ct_priv, entry, true);
-	mlx5_tc_ct_entry_del_rule(ct_priv, entry, false);
+	mlx5_tc_ct_entry_del_rule(ct_priv, entry);
 
 	mlx5_fc_destroy(ct_priv->esw->dev, entry->counter);
 }
@@ -493,7 +491,7 @@ struct mlx5_ct_entry {
 			  struct mlx5_ct_entry *entry,
 			  bool nat)
 {
-	struct mlx5_ct_zone_rule *zone_rule = &entry->zone_rules[nat];
+	struct mlx5_ct_zone_rule *zone_rule = &entry->zone_rule;
 	struct mlx5_esw_flow_attr *attr = &zone_rule->attr;
 	struct mlx5_eswitch *esw = ct_priv->esw;
 	struct mlx5_flow_spec *spec = NULL;
@@ -562,7 +560,8 @@ struct mlx5_ct_entry {
 static int
 mlx5_tc_ct_entry_add_rules(struct mlx5_tc_ct_priv *ct_priv,
 			   struct flow_rule *flow_rule,
-			   struct mlx5_ct_entry *entry)
+			   struct mlx5_ct_entry *entry,
+			   bool nat)
 {
 	struct mlx5_eswitch *esw = ct_priv->esw;
 	int err;
@@ -574,20 +573,10 @@ struct mlx5_ct_entry {
 		return err;
 	}
 
-	err = mlx5_tc_ct_entry_add_rule(ct_priv, flow_rule, entry, false);
+	err = mlx5_tc_ct_entry_add_rule(ct_priv, flow_rule, entry, nat);
 	if (err)
-		goto err_orig;
-
-	err = mlx5_tc_ct_entry_add_rule(ct_priv, flow_rule, entry, true);
-	if (err)
-		goto err_nat;
-
-	return 0;
+		mlx5_fc_destroy(esw->dev, entry->counter);
 
-err_nat:
-	mlx5_tc_ct_entry_del_rule(ct_priv, entry, false);
-err_orig:
-	mlx5_fc_destroy(esw->dev, entry->counter);
 	return err;
 }
 
@@ -619,7 +608,8 @@ struct mlx5_ct_entry {
 	entry->cookie = flow->cookie;
 	entry->restore_cookie = meta_action->ct_metadata.cookie;
 
-	err = mlx5_tc_ct_entry_add_rules(ct_priv, flow_rule, entry);
+	err = mlx5_tc_ct_entry_add_rules(ct_priv, flow_rule, entry,
+					 meta_action->ct_metadata.nat);
 	if (err)
 		goto err_rules;
 
@@ -1620,7 +1610,7 @@ struct mlx5_flow_handle *
 		return false;
 
 	entry = container_of(zone_rule, struct mlx5_ct_entry,
-			     zone_rules[zone_rule->nat]);
+			     zone_rule);
 	tcf_ct_flow_table_restore_skb(skb, entry->restore_cookie);
 
 	return true;
-- 
1.8.3.1

Re: [PATCH net-next 0/2] net/mlx5e: add nat support in ct_metadata

[Edward Cree]

On 28/05/2020 08:15, wenxu@ucloud.cn wrote:
> From: wenxu <wenxu@ucloud.cn>
>
> Currently all the conntrack entry offfload rules will be add
> in both ct and ct_nat flow table in the mlx5e driver. It is
> not makesense.
>
> This serise provide nat attribute in the ct_metadata action which
> tell driver the rule should add to ct or ct_nat flow table 
I don't understand why changes to the core are needed.
A conntrack entry should be a NAT if and only if it has
 FLOW_ACTION_MANGLE actions.  AIUI this is sufficient information
 to distinguish NAT from non-NAT conntrack, and there's no need
 for an additional bool in ct_metadata.
But it's possible my understanding is wrong.

-ed

Re: [PATCH net-next 0/2] net/mlx5e: add nat support in ct_metadata

[wenxu]

On 5/28/2020 7:35 PM, Edward Cree wrote:
> On 28/05/2020 08:15, wenxu@ucloud.cn wrote:
>> From: wenxu <wenxu@ucloud.cn>
>>
>> Currently all the conntrack entry offfload rules will be add
>> in both ct and ct_nat flow table in the mlx5e driver. It is
>> not makesense.
>>
>> This serise provide nat attribute in the ct_metadata action which
>> tell driver the rule should add to ct or ct_nat flow table 
> I don't understand why changes to the core are needed.
> A conntrack entry should be a NAT if and only if it has
>  FLOW_ACTION_MANGLE actions.  AIUI this is sufficient information
>  to distinguish NAT from non-NAT conntrack, and there's no need
>  for an additional bool in ct_metadata.
> But it's possible my understanding is wrong.

Yes, Currently the  FLOW_ACTION_MANGLE can distinguish this.

But I think the right way to get nat or non-nat conntrack  is through the

nf_conn->status ?


 
>
> -ed
>

Re: [PATCH net-next 2/2] net/mlx5e: add ct_metadata.nat support in ct offload

[Oz Shlomo]

Hi Wenxu,

On 5/28/2020 10:15 AM, wenxu@ucloud.cn wrote:
> From: wenxu <wenxu@ucloud.cn>
> 
> In the ct offload all the conntrack entry offload  rules
> will be add to both ct ft and ct_nat ft twice.
> It is not makesense. The ct_metadat.nat will tell driver

Adding the connection to both tables is required because the user may
perform a CT action without NAT even though a NAT entry was allocated
when the connection was committed.

> the rule should add to ct or ct_nat flow table
> 
> Signed-off-by: wenxu <wenxu@ucloud.cn>
> ---
>   drivers/net/ethernet/mellanox/mlx5/core/en/tc_ct.c | 34 ++++++++--------------
>   1 file changed, 12 insertions(+), 22 deletions(-)
> 
> diff --git a/drivers/net/ethernet/mellanox/mlx5/core/en/tc_ct.c b/drivers/net/ethernet/mellanox/mlx5/core/en/tc_ct.c
> index 995b2ef..02ecd24 100644
> --- a/drivers/net/ethernet/mellanox/mlx5/core/en/tc_ct.c
> +++ b/drivers/net/ethernet/mellanox/mlx5/core/en/tc_ct.c
> @@ -88,7 +88,7 @@ struct mlx5_ct_entry {
>   	struct mlx5_fc *counter;
>   	unsigned long cookie;
>   	unsigned long restore_cookie;
> -	struct mlx5_ct_zone_rule zone_rules[2];
> +	struct mlx5_ct_zone_rule zone_rule;
>   };
>   
>   static const struct rhashtable_params cts_ht_params = {
> @@ -238,10 +238,9 @@ struct mlx5_ct_entry {
>   
>   static void
>   mlx5_tc_ct_entry_del_rule(struct mlx5_tc_ct_priv *ct_priv,
> -			  struct mlx5_ct_entry *entry,
> -			  bool nat)
> +			  struct mlx5_ct_entry *entry)
>   {
> -	struct mlx5_ct_zone_rule *zone_rule = &entry->zone_rules[nat];
> +	struct mlx5_ct_zone_rule *zone_rule = &entry->zone_rule;
>   	struct mlx5_esw_flow_attr *attr = &zone_rule->attr;
>   	struct mlx5_eswitch *esw = ct_priv->esw;
>   
> @@ -256,8 +255,7 @@ struct mlx5_ct_entry {
>   mlx5_tc_ct_entry_del_rules(struct mlx5_tc_ct_priv *ct_priv,
>   			   struct mlx5_ct_entry *entry)
>   {
> -	mlx5_tc_ct_entry_del_rule(ct_priv, entry, true);
> -	mlx5_tc_ct_entry_del_rule(ct_priv, entry, false);
> +	mlx5_tc_ct_entry_del_rule(ct_priv, entry);
>   
>   	mlx5_fc_destroy(ct_priv->esw->dev, entry->counter);
>   }
> @@ -493,7 +491,7 @@ struct mlx5_ct_entry {
>   			  struct mlx5_ct_entry *entry,
>   			  bool nat)
>   {
> -	struct mlx5_ct_zone_rule *zone_rule = &entry->zone_rules[nat];
> +	struct mlx5_ct_zone_rule *zone_rule = &entry->zone_rule;
>   	struct mlx5_esw_flow_attr *attr = &zone_rule->attr;
>   	struct mlx5_eswitch *esw = ct_priv->esw;
>   	struct mlx5_flow_spec *spec = NULL;
> @@ -562,7 +560,8 @@ struct mlx5_ct_entry {
>   static int
>   mlx5_tc_ct_entry_add_rules(struct mlx5_tc_ct_priv *ct_priv,
>   			   struct flow_rule *flow_rule,
> -			   struct mlx5_ct_entry *entry)
> +			   struct mlx5_ct_entry *entry,
> +			   bool nat)
>   {
>   	struct mlx5_eswitch *esw = ct_priv->esw;
>   	int err;
> @@ -574,20 +573,10 @@ struct mlx5_ct_entry {
>   		return err;
>   	}
>   
> -	err = mlx5_tc_ct_entry_add_rule(ct_priv, flow_rule, entry, false);
> +	err = mlx5_tc_ct_entry_add_rule(ct_priv, flow_rule, entry, nat);
>   	if (err)
> -		goto err_orig;
> -
> -	err = mlx5_tc_ct_entry_add_rule(ct_priv, flow_rule, entry, true);
> -	if (err)
> -		goto err_nat;
> -
> -	return 0;
> +		mlx5_fc_destroy(esw->dev, entry->counter);
>   
> -err_nat:
> -	mlx5_tc_ct_entry_del_rule(ct_priv, entry, false);
> -err_orig:
> -	mlx5_fc_destroy(esw->dev, entry->counter);
>   	return err;
>   }
>   
> @@ -619,7 +608,8 @@ struct mlx5_ct_entry {
>   	entry->cookie = flow->cookie;
>   	entry->restore_cookie = meta_action->ct_metadata.cookie;
>   
> -	err = mlx5_tc_ct_entry_add_rules(ct_priv, flow_rule, entry);
> +	err = mlx5_tc_ct_entry_add_rules(ct_priv, flow_rule, entry,
> +					 meta_action->ct_metadata.nat);
>   	if (err)
>   		goto err_rules;
>   
> @@ -1620,7 +1610,7 @@ struct mlx5_flow_handle *
>   		return false;
>   
>   	entry = container_of(zone_rule, struct mlx5_ct_entry,
> -			     zone_rules[zone_rule->nat]);
> +			     zone_rule);
>   	tcf_ct_flow_table_restore_skb(skb, entry->restore_cookie);
>   
>   	return true;
> 

Re: [PATCH net-next 2/2] net/mlx5e: add ct_metadata.nat support in ct offload

[wenxu]

On 5/31/2020 4:01 PM, Oz Shlomo wrote:
> Hi Wenxu,
>
> On 5/28/2020 10:15 AM, wenxu@ucloud.cn wrote:
>> From: wenxu <wenxu@ucloud.cn>
>>
>> In the ct offload all the conntrack entry offload  rules
>> will be add to both ct ft and ct_nat ft twice.
>> It is not makesense. The ct_metadat.nat will tell driver
>
> Adding the connection to both tables is required because the user may
> perform a CT action without NAT even though a NAT entry was allocated
> when the connection was committed.

Thanks, understood.  But I just wonder what use case for this behavior?
>
>> the rule should add to ct or ct_nat flow table
>>
>> Signed-off-by: wenxu <wenxu@ucloud.cn>
>> ---
>>   drivers/net/ethernet/mellanox/mlx5/core/en/tc_ct.c | 34 ++++++++--------------
>>   1 file changed, 12 insertions(+), 22 deletions(-)
>>
>> diff --git a/drivers/net/ethernet/mellanox/mlx5/core/en/tc_ct.c b/drivers/net/ethernet/mellanox/mlx5/core/en/tc_ct.c
>> index 995b2ef..02ecd24 100644
>> --- a/drivers/net/ethernet/mellanox/mlx5/core/en/tc_ct.c
>> +++ b/drivers/net/ethernet/mellanox/mlx5/core/en/tc_ct.c
>> @@ -88,7 +88,7 @@ struct mlx5_ct_entry {
>>       struct mlx5_fc *counter;
>>       unsigned long cookie;
>>       unsigned long restore_cookie;
>> -    struct mlx5_ct_zone_rule zone_rules[2];
>> +    struct mlx5_ct_zone_rule zone_rule;
>>   };
>>     static const struct rhashtable_params cts_ht_params = {
>> @@ -238,10 +238,9 @@ struct mlx5_ct_entry {
>>     static void
>>   mlx5_tc_ct_entry_del_rule(struct mlx5_tc_ct_priv *ct_priv,
>> -              struct mlx5_ct_entry *entry,
>> -              bool nat)
>> +              struct mlx5_ct_entry *entry)
>>   {
>> -    struct mlx5_ct_zone_rule *zone_rule = &entry->zone_rules[nat];
>> +    struct mlx5_ct_zone_rule *zone_rule = &entry->zone_rule;
>>       struct mlx5_esw_flow_attr *attr = &zone_rule->attr;
>>       struct mlx5_eswitch *esw = ct_priv->esw;
>>   @@ -256,8 +255,7 @@ struct mlx5_ct_entry {
>>   mlx5_tc_ct_entry_del_rules(struct mlx5_tc_ct_priv *ct_priv,
>>                  struct mlx5_ct_entry *entry)
>>   {
>> -    mlx5_tc_ct_entry_del_rule(ct_priv, entry, true);
>> -    mlx5_tc_ct_entry_del_rule(ct_priv, entry, false);
>> +    mlx5_tc_ct_entry_del_rule(ct_priv, entry);
>>         mlx5_fc_destroy(ct_priv->esw->dev, entry->counter);
>>   }
>> @@ -493,7 +491,7 @@ struct mlx5_ct_entry {
>>                 struct mlx5_ct_entry *entry,
>>                 bool nat)
>>   {
>> -    struct mlx5_ct_zone_rule *zone_rule = &entry->zone_rules[nat];
>> +    struct mlx5_ct_zone_rule *zone_rule = &entry->zone_rule;
>>       struct mlx5_esw_flow_attr *attr = &zone_rule->attr;
>>       struct mlx5_eswitch *esw = ct_priv->esw;
>>       struct mlx5_flow_spec *spec = NULL;
>> @@ -562,7 +560,8 @@ struct mlx5_ct_entry {
>>   static int
>>   mlx5_tc_ct_entry_add_rules(struct mlx5_tc_ct_priv *ct_priv,
>>                  struct flow_rule *flow_rule,
>> -               struct mlx5_ct_entry *entry)
>> +               struct mlx5_ct_entry *entry,
>> +               bool nat)
>>   {
>>       struct mlx5_eswitch *esw = ct_priv->esw;
>>       int err;
>> @@ -574,20 +573,10 @@ struct mlx5_ct_entry {
>>           return err;
>>       }
>>   -    err = mlx5_tc_ct_entry_add_rule(ct_priv, flow_rule, entry, false);
>> +    err = mlx5_tc_ct_entry_add_rule(ct_priv, flow_rule, entry, nat);
>>       if (err)
>> -        goto err_orig;
>> -
>> -    err = mlx5_tc_ct_entry_add_rule(ct_priv, flow_rule, entry, true);
>> -    if (err)
>> -        goto err_nat;
>> -
>> -    return 0;
>> +        mlx5_fc_destroy(esw->dev, entry->counter);
>>   -err_nat:
>> -    mlx5_tc_ct_entry_del_rule(ct_priv, entry, false);
>> -err_orig:
>> -    mlx5_fc_destroy(esw->dev, entry->counter);
>>       return err;
>>   }
>>   @@ -619,7 +608,8 @@ struct mlx5_ct_entry {
>>       entry->cookie = flow->cookie;
>>       entry->restore_cookie = meta_action->ct_metadata.cookie;
>>   -    err = mlx5_tc_ct_entry_add_rules(ct_priv, flow_rule, entry);
>> +    err = mlx5_tc_ct_entry_add_rules(ct_priv, flow_rule, entry,
>> +                     meta_action->ct_metadata.nat);
>>       if (err)
>>           goto err_rules;
>>   @@ -1620,7 +1610,7 @@ struct mlx5_flow_handle *
>>           return false;
>>         entry = container_of(zone_rule, struct mlx5_ct_entry,
>> -                 zone_rules[zone_rule->nat]);
>> +                 zone_rule);
>>       tcf_ct_flow_table_restore_skb(skb, entry->restore_cookie);
>>         return true;
>>
>