diff mbox series

[net-next,v2,8/8] nfp: flower-ct: add tc merge functionality

Message ID 20210531124607.29602-9-simon.horman@corigine.com (mailing list archive)
State Superseded
Delegated to: Netdev Maintainers
Headers show
Series Introduce conntrack offloading to the nfp driver | expand

Checks

Context Check Description
netdev/cover_letter success Link
netdev/fixes_present success Link
netdev/patch_count success Link
netdev/tree_selection success Clearly marked for net-next
netdev/subject_prefix success Link
netdev/cc_maintainers success CCed 7 of 7 maintainers
netdev/source_inline success Was 0 now: 0
netdev/verify_signedoff success Link
netdev/module_param success Was 0 now: 0
netdev/build_32bit success Errors and warnings before: 1 this patch: 1
netdev/kdoc success Errors and warnings before: 1 this patch: 1
netdev/verify_fixes success Link
netdev/checkpatch warning WARNING: line length of 82 exceeds 80 columns WARNING: line length of 85 exceeds 80 columns WARNING: line length of 86 exceeds 80 columns
netdev/build_allmodconfig_warn success Errors and warnings before: 1 this patch: 1
netdev/header_inline success Link

Commit Message

Simon Horman May 31, 2021, 12:46 p.m. UTC
From: Louis Peens <louis.peens@corigine.com>

Add merging of pre/post_ct flow rules into the tc_merge table.
Pre_ct flows needs to be merge with post_ct flows and vice versa.

This needs to be done for all flows in the same zone table, as well
as with the wc_zone_table, which is for flows masking out ct_zone
info.

Cleanup is happening when all the tables are cleared up and prints
a warning traceback as this is not expected in the final version.
At this point we are not actually returning success for the offload,
so we do not get any delete requests for flows, so we can't delete
them that way yet. This means that cleanup happens in what would
usually be an exception path.

Signed-off-by: Louis Peens <louis.peens@corigine.com>
Signed-off-by: Yinjun Zhang <yinjun.zhang@corigine.com>
Signed-off-by: Simon Horman <simon.horman@corigine.com>
---
 .../ethernet/netronome/nfp/flower/conntrack.c | 157 +++++++++++++++++-
 1 file changed, 153 insertions(+), 4 deletions(-)

Comments

Marcelo Ricardo Leitner May 31, 2021, 6:24 p.m. UTC | #1
On Mon, May 31, 2021 at 02:46:07PM +0200, Simon Horman wrote:
> +static int nfp_ct_do_tc_merge(struct nfp_fl_ct_zone_entry *zt,
> +			      struct nfp_fl_ct_flow_entry *ct_entry1,
> +			      struct nfp_fl_ct_flow_entry *ct_entry2)
> +{
> +	struct nfp_fl_ct_flow_entry *post_ct_entry, *pre_ct_entry;
> +	struct nfp_fl_ct_tc_merge *m_entry;
> +	unsigned long new_cookie[2];
> +	int err;
> +
> +	if (ct_entry1->type == CT_TYPE_PRE_CT) {
> +		pre_ct_entry = ct_entry1;
> +		post_ct_entry = ct_entry2;
> +	} else {
> +		post_ct_entry = ct_entry1;
> +		pre_ct_entry = ct_entry2;
> +	}
> +
> +	if (post_ct_entry->netdev != pre_ct_entry->netdev)
> +		return -EINVAL;
> +	if (post_ct_entry->chain_index != pre_ct_entry->chain_index)
> +		return -EINVAL;

I would expect this to always fail with OVS/OVN offload, as it always
jump to a new chain after an action:ct call.

  Marcelo
Louis Peens June 1, 2021, 1:43 p.m. UTC | #2
On 2021/05/31 20:24, Marcelo Ricardo Leitner wrote:
> On Mon, May 31, 2021 at 02:46:07PM +0200, Simon Horman wrote:
>> +static int nfp_ct_do_tc_merge(struct nfp_fl_ct_zone_entry *zt,
>> +			      struct nfp_fl_ct_flow_entry *ct_entry1,
>> +			      struct nfp_fl_ct_flow_entry *ct_entry2)
>> +{
>> +	struct nfp_fl_ct_flow_entry *post_ct_entry, *pre_ct_entry;
>> +	struct nfp_fl_ct_tc_merge *m_entry;
>> +	unsigned long new_cookie[2];
>> +	int err;
>> +
>> +	if (ct_entry1->type == CT_TYPE_PRE_CT) {
>> +		pre_ct_entry = ct_entry1;
>> +		post_ct_entry = ct_entry2;
>> +	} else {
>> +		post_ct_entry = ct_entry1;
>> +		pre_ct_entry = ct_entry2;
>> +	}
>> +
>> +	if (post_ct_entry->netdev != pre_ct_entry->netdev)
>> +		return -EINVAL;
>> +	if (post_ct_entry->chain_index != pre_ct_entry->chain_index)
>> +		return -EINVAL;
> 
> I would expect this to always fail with OVS/OVN offload, as it always
> jump to a new chain after an action:ct call.
Ah, I can see that this may look confusing, I will considering adding
a short comment here for future me as well. The origin for the chain_index
is different for pre_ct and post_ct. For pre_ct the chain_index is populated from
the GOTO action, and for post_ct it is from the match. This checks that the
chain in the action matches the chain in the filter of the next
flow.

The assignment happens in the nfp_fl_ct_handle_pre_ct and nfp_fl_ct_handle_post_ct
functions of [PATCH net-next v2 5/8] nfp: flower-ct: add nfp_fl_ct_flow_entries
> 
>   Marcelo
>
Marcelo Ricardo Leitner June 1, 2021, 2:22 p.m. UTC | #3
On Tue, Jun 01, 2021 at 03:43:25PM +0200, Louis Peens wrote:
>
>
> On 2021/05/31 20:24, Marcelo Ricardo Leitner wrote:
> > On Mon, May 31, 2021 at 02:46:07PM +0200, Simon Horman wrote:
> >> +static int nfp_ct_do_tc_merge(struct nfp_fl_ct_zone_entry *zt,
> >> +			      struct nfp_fl_ct_flow_entry *ct_entry1,
> >> +			      struct nfp_fl_ct_flow_entry *ct_entry2)
> >> +{
> >> +	struct nfp_fl_ct_flow_entry *post_ct_entry, *pre_ct_entry;
> >> +	struct nfp_fl_ct_tc_merge *m_entry;
> >> +	unsigned long new_cookie[2];
> >> +	int err;
> >> +
> >> +	if (ct_entry1->type == CT_TYPE_PRE_CT) {
> >> +		pre_ct_entry = ct_entry1;
> >> +		post_ct_entry = ct_entry2;
> >> +	} else {
> >> +		post_ct_entry = ct_entry1;
> >> +		pre_ct_entry = ct_entry2;
> >> +	}
> >> +
> >> +	if (post_ct_entry->netdev != pre_ct_entry->netdev)
> >> +		return -EINVAL;
> >> +	if (post_ct_entry->chain_index != pre_ct_entry->chain_index)
> >> +		return -EINVAL;
> >
> > I would expect this to always fail with OVS/OVN offload, as it always
> > jump to a new chain after an action:ct call.
> Ah, I can see that this may look confusing, I will considering adding
> a short comment here for future me as well. The origin for the chain_index
> is different for pre_ct and post_ct. For pre_ct the chain_index is populated from
> the GOTO action, and for post_ct it is from the match. This checks that the
> chain in the action matches the chain in the filter of the next
> flow.
>
> The assignment happens in the nfp_fl_ct_handle_pre_ct and nfp_fl_ct_handle_post_ct
> functions of [PATCH net-next v2 5/8] nfp: flower-ct: add nfp_fl_ct_flow_entries

Got it. Thanks.

  Marcelo
diff mbox series

Patch

diff --git a/drivers/net/ethernet/netronome/nfp/flower/conntrack.c b/drivers/net/ethernet/netronome/nfp/flower/conntrack.c
index cf17c9510fbb..ff262d04a73b 100644
--- a/drivers/net/ethernet/netronome/nfp/flower/conntrack.c
+++ b/drivers/net/ethernet/netronome/nfp/flower/conntrack.c
@@ -63,6 +63,74 @@  bool is_post_ct_flow(struct flow_cls_offload *flow)
 	return false;
 }
 
+static int nfp_ct_merge_check(struct nfp_fl_ct_flow_entry *entry1,
+			      struct nfp_fl_ct_flow_entry *entry2)
+{
+	return 0;
+}
+
+static int nfp_ct_do_tc_merge(struct nfp_fl_ct_zone_entry *zt,
+			      struct nfp_fl_ct_flow_entry *ct_entry1,
+			      struct nfp_fl_ct_flow_entry *ct_entry2)
+{
+	struct nfp_fl_ct_flow_entry *post_ct_entry, *pre_ct_entry;
+	struct nfp_fl_ct_tc_merge *m_entry;
+	unsigned long new_cookie[2];
+	int err;
+
+	if (ct_entry1->type == CT_TYPE_PRE_CT) {
+		pre_ct_entry = ct_entry1;
+		post_ct_entry = ct_entry2;
+	} else {
+		post_ct_entry = ct_entry1;
+		pre_ct_entry = ct_entry2;
+	}
+
+	if (post_ct_entry->netdev != pre_ct_entry->netdev)
+		return -EINVAL;
+	if (post_ct_entry->chain_index != pre_ct_entry->chain_index)
+		return -EINVAL;
+
+	err = nfp_ct_merge_check(post_ct_entry, pre_ct_entry);
+	if (err)
+		return err;
+
+	new_cookie[0] = pre_ct_entry->cookie;
+	new_cookie[1] = post_ct_entry->cookie;
+	m_entry = get_hashentry(&zt->tc_merge_tb, &new_cookie,
+				nfp_tc_ct_merge_params, sizeof(*m_entry));
+	if (IS_ERR(m_entry))
+		return PTR_ERR(m_entry);
+
+	/* m_entry already present, not merging again */
+	if (!memcmp(&new_cookie, m_entry->cookie, sizeof(new_cookie)))
+		return 0;
+
+	memcpy(&m_entry->cookie, &new_cookie, sizeof(new_cookie));
+	m_entry->zt = zt;
+	m_entry->post_ct_parent = post_ct_entry;
+	m_entry->pre_ct_parent = pre_ct_entry;
+
+	/* Add this entry to the pre_ct and post_ct lists */
+	list_add(&m_entry->post_ct_list, &post_ct_entry->children);
+	list_add(&m_entry->pre_ct_list, &pre_ct_entry->children);
+	INIT_LIST_HEAD(&m_entry->children);
+
+	err = rhashtable_insert_fast(&zt->tc_merge_tb, &m_entry->hash_node,
+				     nfp_tc_ct_merge_params);
+	if (err)
+		goto err_ct_tc_merge_insert;
+	zt->tc_merge_count++;
+
+	return 0;
+
+err_ct_tc_merge_insert:
+	list_del(&m_entry->post_ct_list);
+	list_del(&m_entry->pre_ct_list);
+	kfree(m_entry);
+	return err;
+}
+
 static struct
 nfp_fl_ct_zone_entry *get_nfp_zone_entry(struct nfp_flower_priv *priv,
 					 u16 zone, bool wildcarded)
@@ -209,12 +277,48 @@  nfp_fl_ct_flow_entry *nfp_fl_ct_add_flow(struct nfp_fl_ct_zone_entry *zt,
 	return ERR_PTR(err);
 }
 
-static void nfp_free_tc_merge_children(struct nfp_fl_ct_flow_entry *entry)
+static void nfp_free_nft_merge_children(void *entry, bool is_nft_flow)
 {
 }
 
-static void nfp_free_nft_merge_children(void *entry, bool is_nft_flow)
+static void nfp_del_tc_merge_entry(struct nfp_fl_ct_tc_merge *m_ent)
 {
+	struct nfp_fl_ct_zone_entry *zt;
+	int err;
+
+	zt = m_ent->zt;
+	err = rhashtable_remove_fast(&zt->tc_merge_tb,
+				     &m_ent->hash_node,
+				     nfp_tc_ct_merge_params);
+	if (err)
+		pr_warn("WARNING: could not remove merge_entry from hashtable\n");
+	zt->tc_merge_count--;
+	list_del(&m_ent->post_ct_list);
+	list_del(&m_ent->pre_ct_list);
+
+	if (!list_empty(&m_ent->children))
+		nfp_free_nft_merge_children(m_ent, false);
+	kfree(m_ent);
+}
+
+static void nfp_free_tc_merge_children(struct nfp_fl_ct_flow_entry *entry)
+{
+	struct nfp_fl_ct_tc_merge *m_ent, *tmp;
+
+	switch (entry->type) {
+	case CT_TYPE_PRE_CT:
+		list_for_each_entry_safe(m_ent, tmp, &entry->children, pre_ct_list) {
+			nfp_del_tc_merge_entry(m_ent);
+		}
+		break;
+	case CT_TYPE_POST_CT:
+		list_for_each_entry_safe(m_ent, tmp, &entry->children, post_ct_list) {
+			nfp_del_tc_merge_entry(m_ent);
+		}
+		break;
+	default:
+		break;
+	}
 }
 
 void nfp_fl_ct_clean_flow_entry(struct nfp_fl_ct_flow_entry *entry)
@@ -247,6 +351,25 @@  static struct flow_action_entry *get_flow_act(struct flow_cls_offload *flow,
 	return NULL;
 }
 
+static void
+nfp_ct_merge_tc_entries(struct nfp_fl_ct_flow_entry *ct_entry1,
+			struct nfp_fl_ct_zone_entry *zt_src,
+			struct nfp_fl_ct_zone_entry *zt_dst)
+{
+	struct nfp_fl_ct_flow_entry *ct_entry2, *ct_tmp;
+	struct list_head *ct_list;
+
+	if (ct_entry1->type == CT_TYPE_PRE_CT)
+		ct_list = &zt_src->post_ct_list;
+	else if (ct_entry1->type == CT_TYPE_POST_CT)
+		ct_list = &zt_src->pre_ct_list;
+
+	list_for_each_entry_safe(ct_entry2, ct_tmp, ct_list,
+				 list_node) {
+		nfp_ct_do_tc_merge(zt_dst, ct_entry2, ct_entry1);
+	}
+}
+
 int nfp_fl_ct_handle_pre_ct(struct nfp_flower_priv *priv,
 			    struct net_device *netdev,
 			    struct flow_cls_offload *flow,
@@ -289,8 +412,13 @@  int nfp_fl_ct_handle_pre_ct(struct nfp_flower_priv *priv,
 	list_add(&ct_entry->list_node, &zt->pre_ct_list);
 	zt->pre_ct_count++;
 
+	nfp_ct_merge_tc_entries(ct_entry, zt, zt);
+
+	/* Need to check and merge with tables in the wc_zone as well */
+	if (priv->ct_zone_wc)
+		nfp_ct_merge_tc_entries(ct_entry, priv->ct_zone_wc, zt);
+
 	NL_SET_ERR_MSG_MOD(extack, "unsupported offload: Conntrack action not supported");
-	nfp_fl_ct_clean_flow_entry(ct_entry);
 	return -EOPNOTSUPP;
 }
 
@@ -331,7 +459,28 @@  int nfp_fl_ct_handle_post_ct(struct nfp_flower_priv *priv,
 	list_add(&ct_entry->list_node, &zt->post_ct_list);
 	zt->post_ct_count++;
 
+	if (wildcarded) {
+		/* Iterate through all zone tables if not empty, look for merges with
+		 * pre_ct entries and merge them.
+		 */
+		struct rhashtable_iter iter;
+		struct nfp_fl_ct_zone_entry *zone_table;
+
+		rhashtable_walk_enter(&priv->ct_zone_table, &iter);
+		rhashtable_walk_start(&iter);
+		while ((zone_table = rhashtable_walk_next(&iter)) != NULL) {
+			if (IS_ERR(zone_table))
+				continue;
+			rhashtable_walk_stop(&iter);
+			nfp_ct_merge_tc_entries(ct_entry, zone_table, zone_table);
+			rhashtable_walk_start(&iter);
+		}
+		rhashtable_walk_stop(&iter);
+		rhashtable_walk_exit(&iter);
+	} else {
+		nfp_ct_merge_tc_entries(ct_entry, zt, zt);
+	}
+
 	NL_SET_ERR_MSG_MOD(extack, "unsupported offload: Conntrack match not supported");
-	nfp_fl_ct_clean_flow_entry(ct_entry);
 	return -EOPNOTSUPP;
 }