* [PATCH net-next 0/2] net/mlx5e: add nat support in ct_metadata
@ 2020-05-28 7:15 wenxu
2020-05-28 7:15 ` [PATCH net-next 1/2] net/sched: act_ct: add nat attribute " wenxu
` (2 more replies)
0 siblings, 3 replies; 7+ messages in thread
From: wenxu @ 2020-05-28 7:15 UTC (permalink / raw)
To: paulb, saeedm; +Cc: netdev
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
^ permalink raw reply [flat|nested] 7+ messages in thread* [PATCH net-next 1/2] net/sched: act_ct: add nat attribute in ct_metadata 2020-05-28 7:15 [PATCH net-next 0/2] net/mlx5e: add nat support in ct_metadata wenxu @ 2020-05-28 7:15 ` wenxu 2020-05-28 7:15 ` [PATCH net-next 2/2] net/mlx5e: add ct_metadata.nat support in ct offload wenxu 2020-05-28 11:35 ` [PATCH net-next 0/2] net/mlx5e: add nat support in ct_metadata Edward Cree 2 siblings, 0 replies; 7+ messages in thread From: wenxu @ 2020-05-28 7:15 UTC (permalink / raw) To: paulb, saeedm; +Cc: netdev 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 ^ permalink raw reply related [flat|nested] 7+ messages in thread
* [PATCH net-next 2/2] net/mlx5e: add ct_metadata.nat support in ct offload 2020-05-28 7:15 [PATCH net-next 0/2] net/mlx5e: add nat support in ct_metadata wenxu 2020-05-28 7:15 ` [PATCH net-next 1/2] net/sched: act_ct: add nat attribute " wenxu @ 2020-05-28 7:15 ` wenxu 2020-05-31 8:01 ` Oz Shlomo 2020-05-28 11:35 ` [PATCH net-next 0/2] net/mlx5e: add nat support in ct_metadata Edward Cree 2 siblings, 1 reply; 7+ messages in thread From: wenxu @ 2020-05-28 7:15 UTC (permalink / raw) To: paulb, saeedm; +Cc: netdev 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 ^ permalink raw reply related [flat|nested] 7+ messages in thread
* Re: [PATCH net-next 2/2] net/mlx5e: add ct_metadata.nat support in ct offload 2020-05-28 7:15 ` [PATCH net-next 2/2] net/mlx5e: add ct_metadata.nat support in ct offload wenxu @ 2020-05-31 8:01 ` Oz Shlomo 2020-06-01 3:11 ` wenxu 0 siblings, 1 reply; 7+ messages in thread From: Oz Shlomo @ 2020-05-31 8:01 UTC (permalink / raw) To: wenxu, paulb, saeedm; +Cc: netdev 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; > ^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH net-next 2/2] net/mlx5e: add ct_metadata.nat support in ct offload 2020-05-31 8:01 ` Oz Shlomo @ 2020-06-01 3:11 ` wenxu 0 siblings, 0 replies; 7+ messages in thread From: wenxu @ 2020-06-01 3:11 UTC (permalink / raw) To: Oz Shlomo, paulb, saeedm; +Cc: netdev 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; >> > ^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH net-next 0/2] net/mlx5e: add nat support in ct_metadata 2020-05-28 7:15 [PATCH net-next 0/2] net/mlx5e: add nat support in ct_metadata wenxu 2020-05-28 7:15 ` [PATCH net-next 1/2] net/sched: act_ct: add nat attribute " wenxu 2020-05-28 7:15 ` [PATCH net-next 2/2] net/mlx5e: add ct_metadata.nat support in ct offload wenxu @ 2020-05-28 11:35 ` Edward Cree 2020-05-29 3:40 ` wenxu 2 siblings, 1 reply; 7+ messages in thread From: Edward Cree @ 2020-05-28 11:35 UTC (permalink / raw) To: wenxu, paulb, saeedm; +Cc: netdev 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 ^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH net-next 0/2] net/mlx5e: add nat support in ct_metadata 2020-05-28 11:35 ` [PATCH net-next 0/2] net/mlx5e: add nat support in ct_metadata Edward Cree @ 2020-05-29 3:40 ` wenxu 0 siblings, 0 replies; 7+ messages in thread From: wenxu @ 2020-05-29 3:40 UTC (permalink / raw) To: Edward Cree, paulb, saeedm; +Cc: netdev 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 > ^ permalink raw reply [flat|nested] 7+ messages in thread
end of thread, other threads:[~2020-06-01 3:12 UTC | newest] Thread overview: 7+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2020-05-28 7:15 [PATCH net-next 0/2] net/mlx5e: add nat support in ct_metadata wenxu 2020-05-28 7:15 ` [PATCH net-next 1/2] net/sched: act_ct: add nat attribute " wenxu 2020-05-28 7:15 ` [PATCH net-next 2/2] net/mlx5e: add ct_metadata.nat support in ct offload wenxu 2020-05-31 8:01 ` Oz Shlomo 2020-06-01 3:11 ` wenxu 2020-05-28 11:35 ` [PATCH net-next 0/2] net/mlx5e: add nat support in ct_metadata Edward Cree 2020-05-29 3:40 ` wenxu
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for NNTP newsgroup(s).