[dpdk-dev] [PATCH 0/2] LPM changes

[dpdk-dev] [PATCH 0/2] LPM changes

[Ruifeng Wang]

The rte_lpm structure is exported because lookup API is inlined.
But most of the structure can be hidden.
Discussion at: http://patches.dpdk.org/patch/72403/
This patch set aimed to hide the rte_lpm structure as much as possible.

A data free issue was identified and fixed.

Ruifeng Wang (2):
  lpm: fix free of data structure
  lpm: hide internal data

 lib/librte_lpm/rte_lpm.c | 154 +++++++++++++++++++++++----------------
 lib/librte_lpm/rte_lpm.h |   7 --
 2 files changed, 92 insertions(+), 69 deletions(-)

-- 
2.17.1

[dpdk-dev] [PATCH 1/2] lpm: fix free of data structure

[Ruifeng Wang]

The container structure should be freed instead of rte_lpm structure
after wrapping rte_lpm into internal structure __rte_lpm.

Fixes: 8a9f8564e9f9 ("lpm: implement RCU rule reclamation")
Cc: stable@dpdk.org

Signed-off-by: Ruifeng Wang <ruifeng.wang@arm.com>
Reviewed-by: Phil Yang <phil.yang@arm.com>
---
 lib/librte_lpm/rte_lpm.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/lib/librte_lpm/rte_lpm.c b/lib/librte_lpm/rte_lpm.c
index 757436f49..51a0ae578 100644
--- a/lib/librte_lpm/rte_lpm.c
+++ b/lib/librte_lpm/rte_lpm.c
@@ -268,7 +268,7 @@ rte_lpm_free(struct rte_lpm *lpm)
 		rte_rcu_qsbr_dq_delete(internal_lpm->dq);
 	rte_free(lpm->tbl8);
 	rte_free(lpm->rules_tbl);
-	rte_free(lpm);
+	rte_free(internal_lpm);
 	rte_free(te);
 }
 
-- 
2.17.1

Re: [dpdk-dev] [PATCH 1/2] lpm: fix free of data structure

[Bruce Richardson]

On Mon, Sep 07, 2020 at 04:15:16PM +0800, Ruifeng Wang wrote:
> The container structure should be freed instead of rte_lpm structure
> after wrapping rte_lpm into internal structure __rte_lpm.
> 
> Fixes: 8a9f8564e9f9 ("lpm: implement RCU rule reclamation")
> Cc: stable@dpdk.org
> 
> Signed-off-by: Ruifeng Wang <ruifeng.wang@arm.com>
> Reviewed-by: Phil Yang <phil.yang@arm.com>
> ---
>  lib/librte_lpm/rte_lpm.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/lib/librte_lpm/rte_lpm.c b/lib/librte_lpm/rte_lpm.c
> index 757436f49..51a0ae578 100644
> --- a/lib/librte_lpm/rte_lpm.c
> +++ b/lib/librte_lpm/rte_lpm.c
> @@ -268,7 +268,7 @@ rte_lpm_free(struct rte_lpm *lpm)
>  		rte_rcu_qsbr_dq_delete(internal_lpm->dq);
>  	rte_free(lpm->tbl8);
>  	rte_free(lpm->rules_tbl);
> -	rte_free(lpm);
> +	rte_free(internal_lpm);
>  	rte_free(te);
>  }
>  
> -- 
Acked-by: Bruce Richardson <bruce.richardson@intel.com>

Re: [dpdk-dev] [PATCH 1/2] lpm: fix free of data structure

[Medvedkin, Vladimir]

On 07/09/2020 09:15, Ruifeng Wang wrote:
> The container structure should be freed instead of rte_lpm structure
> after wrapping rte_lpm into internal structure __rte_lpm.
> 
> Fixes: 8a9f8564e9f9 ("lpm: implement RCU rule reclamation")
> Cc: stable@dpdk.org
> 
> Signed-off-by: Ruifeng Wang <ruifeng.wang@arm.com>
> Reviewed-by: Phil Yang <phil.yang@arm.com>
> ---
>   lib/librte_lpm/rte_lpm.c | 2 +-
>   1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/lib/librte_lpm/rte_lpm.c b/lib/librte_lpm/rte_lpm.c
> index 757436f49..51a0ae578 100644
> --- a/lib/librte_lpm/rte_lpm.c
> +++ b/lib/librte_lpm/rte_lpm.c
> @@ -268,7 +268,7 @@ rte_lpm_free(struct rte_lpm *lpm)
>   		rte_rcu_qsbr_dq_delete(internal_lpm->dq);
>   	rte_free(lpm->tbl8);
>   	rte_free(lpm->rules_tbl);
> -	rte_free(lpm);
> +	rte_free(internal_lpm);
>   	rte_free(te);
>   }
>   
> 

Acked-by: Vladimir Medvedkin <vladimir.medvedkin@intel.com>

-- 
Regards,
Vladimir

[dpdk-dev] [PATCH 2/2] lpm: hide internal data

[Ruifeng Wang]

Fields except tbl24 and tbl8 in rte_lpm structure have no
need to be exposed to the user.
Hide the unneeded exposure of structure fields for better
ABI maintainability.

Suggested-by: David Marchand <david.marchand@redhat.com>
Signed-off-by: Ruifeng Wang <ruifeng.wang@arm.com>
Reviewed-by: Phil Yang <phil.yang@arm.com>
---
 lib/librte_lpm/rte_lpm.c | 152 +++++++++++++++++++++++----------------
 lib/librte_lpm/rte_lpm.h |   7 --
 2 files changed, 91 insertions(+), 68 deletions(-)

diff --git a/lib/librte_lpm/rte_lpm.c b/lib/librte_lpm/rte_lpm.c
index 51a0ae578..88d31df6d 100644
--- a/lib/librte_lpm/rte_lpm.c
+++ b/lib/librte_lpm/rte_lpm.c
@@ -42,9 +42,17 @@ enum valid_flag {
 
 /** @internal LPM structure. */
 struct __rte_lpm {
-	/* LPM metadata. */
+	/* Exposed LPM data. */
 	struct rte_lpm lpm;
 
+	/* LPM metadata. */
+	char name[RTE_LPM_NAMESIZE];        /**< Name of the lpm. */
+	uint32_t max_rules; /**< Max. balanced rules per lpm. */
+	uint32_t number_tbl8s; /**< Number of tbl8s. */
+	/**< Rule info table. */
+	struct rte_lpm_rule_info rule_info[RTE_LPM_MAX_DEPTH];
+	struct rte_lpm_rule *rules_tbl; /**< LPM rules. */
+
 	/* RCU config. */
 	struct rte_rcu_qsbr *v;		/* RCU QSBR variable. */
 	enum rte_lpm_qsbr_mode rcu_mode;/* Blocking, defer queue. */
@@ -104,7 +112,7 @@ depth_to_range(uint8_t depth)
 struct rte_lpm *
 rte_lpm_find_existing(const char *name)
 {
-	struct rte_lpm *l = NULL;
+	struct __rte_lpm *l = NULL;
 	struct rte_tailq_entry *te;
 	struct rte_lpm_list *lpm_list;
 
@@ -123,7 +131,7 @@ rte_lpm_find_existing(const char *name)
 		return NULL;
 	}
 
-	return l;
+	return &l->lpm;
 }
 
 /*
@@ -157,8 +165,8 @@ rte_lpm_create(const char *name, int socket_id,
 
 	/* guarantee there's no existing */
 	TAILQ_FOREACH(te, lpm_list, next) {
-		lpm = te->data;
-		if (strncmp(name, lpm->name, RTE_LPM_NAMESIZE) == 0)
+		internal_lpm = te->data;
+		if (strncmp(name, internal_lpm->name, RTE_LPM_NAMESIZE) == 0)
 			break;
 	}
 
@@ -193,10 +201,10 @@ rte_lpm_create(const char *name, int socket_id,
 	}
 
 	lpm = &internal_lpm->lpm;
-	lpm->rules_tbl = rte_zmalloc_socket(NULL,
+	internal_lpm->rules_tbl = rte_zmalloc_socket(NULL,
 			(size_t)rules_size, RTE_CACHE_LINE_SIZE, socket_id);
 
-	if (lpm->rules_tbl == NULL) {
+	if (internal_lpm->rules_tbl == NULL) {
 		RTE_LOG(ERR, LPM, "LPM rules_tbl memory allocation failed\n");
 		rte_free(internal_lpm);
 		internal_lpm = NULL;
@@ -211,7 +219,7 @@ rte_lpm_create(const char *name, int socket_id,
 
 	if (lpm->tbl8 == NULL) {
 		RTE_LOG(ERR, LPM, "LPM tbl8 memory allocation failed\n");
-		rte_free(lpm->rules_tbl);
+		rte_free(internal_lpm->rules_tbl);
 		rte_free(internal_lpm);
 		internal_lpm = NULL;
 		lpm = NULL;
@@ -221,11 +229,11 @@ rte_lpm_create(const char *name, int socket_id,
 	}
 
 	/* Save user arguments. */
-	lpm->max_rules = config->max_rules;
-	lpm->number_tbl8s = config->number_tbl8s;
-	strlcpy(lpm->name, name, sizeof(lpm->name));
+	internal_lpm->max_rules = config->max_rules;
+	internal_lpm->number_tbl8s = config->number_tbl8s;
+	strlcpy(internal_lpm->name, name, sizeof(internal_lpm->name));
 
-	te->data = lpm;
+	te->data = internal_lpm;
 
 	TAILQ_INSERT_TAIL(lpm_list, te, next);
 
@@ -241,7 +249,7 @@ rte_lpm_create(const char *name, int socket_id,
 void
 rte_lpm_free(struct rte_lpm *lpm)
 {
-	struct __rte_lpm *internal_lpm;
+	struct __rte_lpm *internal_lpm = NULL;
 	struct rte_lpm_list *lpm_list;
 	struct rte_tailq_entry *te;
 
@@ -255,7 +263,8 @@ rte_lpm_free(struct rte_lpm *lpm)
 
 	/* find our tailq entry */
 	TAILQ_FOREACH(te, lpm_list, next) {
-		if (te->data == (void *) lpm)
+		internal_lpm = te->data;
+		if (&internal_lpm->lpm == lpm)
 			break;
 	}
 	if (te != NULL)
@@ -263,11 +272,10 @@ rte_lpm_free(struct rte_lpm *lpm)
 
 	rte_mcfg_tailq_write_unlock();
 
-	internal_lpm = container_of(lpm, struct __rte_lpm, lpm);
 	if (internal_lpm->dq != NULL)
 		rte_rcu_qsbr_dq_delete(internal_lpm->dq);
 	rte_free(lpm->tbl8);
-	rte_free(lpm->rules_tbl);
+	rte_free(internal_lpm->rules_tbl);
 	rte_free(internal_lpm);
 	rte_free(te);
 }
@@ -310,11 +318,11 @@ rte_lpm_rcu_qsbr_add(struct rte_lpm *lpm, struct rte_lpm_rcu_config *cfg)
 	} else if (cfg->mode == RTE_LPM_QSBR_MODE_DQ) {
 		/* Init QSBR defer queue. */
 		snprintf(rcu_dq_name, sizeof(rcu_dq_name),
-				"LPM_RCU_%s", lpm->name);
+				"LPM_RCU_%s", internal_lpm->name);
 		params.name = rcu_dq_name;
 		params.size = cfg->dq_size;
 		if (params.size == 0)
-			params.size = lpm->number_tbl8s;
+			params.size = internal_lpm->number_tbl8s;
 		params.trigger_reclaim_limit = cfg->reclaim_thd;
 		params.max_reclaim_size = cfg->reclaim_max;
 		if (params.max_reclaim_size == 0)
@@ -352,74 +360,79 @@ static int32_t
 rule_add(struct rte_lpm *lpm, uint32_t ip_masked, uint8_t depth,
 	uint32_t next_hop)
 {
-	uint32_t rule_gindex, rule_index, last_rule;
+	uint32_t rule_gindex, rule_index, last_rule, first_index;
+	struct __rte_lpm *i_lpm;
 	int i;
 
 	VERIFY_DEPTH(depth);
 
+	i_lpm = container_of(lpm, struct __rte_lpm, lpm);
 	/* Scan through rule group to see if rule already exists. */
-	if (lpm->rule_info[depth - 1].used_rules > 0) {
+	if (i_lpm->rule_info[depth - 1].used_rules > 0) {
 
 		/* rule_gindex stands for rule group index. */
-		rule_gindex = lpm->rule_info[depth - 1].first_rule;
+		rule_gindex = i_lpm->rule_info[depth - 1].first_rule;
 		/* Initialise rule_index to point to start of rule group. */
 		rule_index = rule_gindex;
 		/* Last rule = Last used rule in this rule group. */
-		last_rule = rule_gindex + lpm->rule_info[depth - 1].used_rules;
+		last_rule = rule_gindex
+				+ i_lpm->rule_info[depth - 1].used_rules;
 
 		for (; rule_index < last_rule; rule_index++) {
 
 			/* If rule already exists update next hop and return. */
-			if (lpm->rules_tbl[rule_index].ip == ip_masked) {
+			if (i_lpm->rules_tbl[rule_index].ip == ip_masked) {
 
-				if (lpm->rules_tbl[rule_index].next_hop
+				if (i_lpm->rules_tbl[rule_index].next_hop
 						== next_hop)
 					return -EEXIST;
-				lpm->rules_tbl[rule_index].next_hop = next_hop;
+				i_lpm->rules_tbl[rule_index].next_hop
+					= next_hop;
 
 				return rule_index;
 			}
 		}
 
-		if (rule_index == lpm->max_rules)
+		if (rule_index == i_lpm->max_rules)
 			return -ENOSPC;
 	} else {
 		/* Calculate the position in which the rule will be stored. */
 		rule_index = 0;
 
 		for (i = depth - 1; i > 0; i--) {
-			if (lpm->rule_info[i - 1].used_rules > 0) {
-				rule_index = lpm->rule_info[i - 1].first_rule
-						+ lpm->rule_info[i - 1].used_rules;
+			if (i_lpm->rule_info[i - 1].used_rules > 0) {
+				rule_index = i_lpm->rule_info[i - 1].first_rule
+					+ i_lpm->rule_info[i - 1].used_rules;
 				break;
 			}
 		}
-		if (rule_index == lpm->max_rules)
+		if (rule_index == i_lpm->max_rules)
 			return -ENOSPC;
 
-		lpm->rule_info[depth - 1].first_rule = rule_index;
+		i_lpm->rule_info[depth - 1].first_rule = rule_index;
 	}
 
 	/* Make room for the new rule in the array. */
 	for (i = RTE_LPM_MAX_DEPTH; i > depth; i--) {
-		if (lpm->rule_info[i - 1].first_rule
-				+ lpm->rule_info[i - 1].used_rules == lpm->max_rules)
+		first_index = i_lpm->rule_info[i - 1].first_rule;
+		if (first_index + i_lpm->rule_info[i - 1].used_rules
+				== i_lpm->max_rules)
 			return -ENOSPC;
 
-		if (lpm->rule_info[i - 1].used_rules > 0) {
-			lpm->rules_tbl[lpm->rule_info[i - 1].first_rule
-				+ lpm->rule_info[i - 1].used_rules]
-					= lpm->rules_tbl[lpm->rule_info[i - 1].first_rule];
-			lpm->rule_info[i - 1].first_rule++;
+		if (i_lpm->rule_info[i - 1].used_rules > 0) {
+			i_lpm->rules_tbl[first_index
+				+ i_lpm->rule_info[i - 1].used_rules]
+					= i_lpm->rules_tbl[first_index];
+			i_lpm->rule_info[i - 1].first_rule++;
 		}
 	}
 
 	/* Add the new rule. */
-	lpm->rules_tbl[rule_index].ip = ip_masked;
-	lpm->rules_tbl[rule_index].next_hop = next_hop;
+	i_lpm->rules_tbl[rule_index].ip = ip_masked;
+	i_lpm->rules_tbl[rule_index].next_hop = next_hop;
 
 	/* Increment the used rules counter for this rule group. */
-	lpm->rule_info[depth - 1].used_rules++;
+	i_lpm->rule_info[depth - 1].used_rules++;
 
 	return rule_index;
 }
@@ -432,23 +445,25 @@ static void
 rule_delete(struct rte_lpm *lpm, int32_t rule_index, uint8_t depth)
 {
 	int i;
+	struct __rte_lpm *i_lpm;
 
 	VERIFY_DEPTH(depth);
 
-	lpm->rules_tbl[rule_index] =
-			lpm->rules_tbl[lpm->rule_info[depth - 1].first_rule
-			+ lpm->rule_info[depth - 1].used_rules - 1];
+	i_lpm = container_of(lpm, struct __rte_lpm, lpm);
+	i_lpm->rules_tbl[rule_index] =
+			i_lpm->rules_tbl[i_lpm->rule_info[depth - 1].first_rule
+			+ i_lpm->rule_info[depth - 1].used_rules - 1];
 
 	for (i = depth; i < RTE_LPM_MAX_DEPTH; i++) {
-		if (lpm->rule_info[i].used_rules > 0) {
-			lpm->rules_tbl[lpm->rule_info[i].first_rule - 1] =
-					lpm->rules_tbl[lpm->rule_info[i].first_rule
-						+ lpm->rule_info[i].used_rules - 1];
-			lpm->rule_info[i].first_rule--;
+		if (i_lpm->rule_info[i].used_rules > 0) {
+			i_lpm->rules_tbl[i_lpm->rule_info[i].first_rule - 1] =
+				i_lpm->rules_tbl[i_lpm->rule_info[i].first_rule
+					+ i_lpm->rule_info[i].used_rules - 1];
+			i_lpm->rule_info[i].first_rule--;
 		}
 	}
 
-	lpm->rule_info[depth - 1].used_rules--;
+	i_lpm->rule_info[depth - 1].used_rules--;
 }
 
 /*
@@ -459,16 +474,18 @@ static int32_t
 rule_find(struct rte_lpm *lpm, uint32_t ip_masked, uint8_t depth)
 {
 	uint32_t rule_gindex, last_rule, rule_index;
+	struct __rte_lpm *internal_lpm;
 
 	VERIFY_DEPTH(depth);
 
-	rule_gindex = lpm->rule_info[depth - 1].first_rule;
-	last_rule = rule_gindex + lpm->rule_info[depth - 1].used_rules;
+	internal_lpm = container_of(lpm, struct __rte_lpm, lpm);
+	rule_gindex = internal_lpm->rule_info[depth - 1].first_rule;
+	last_rule = rule_gindex + internal_lpm->rule_info[depth - 1].used_rules;
 
 	/* Scan used rules at given depth to find rule. */
 	for (rule_index = rule_gindex; rule_index < last_rule; rule_index++) {
 		/* If rule is found return the rule index. */
-		if (lpm->rules_tbl[rule_index].ip == ip_masked)
+		if (internal_lpm->rules_tbl[rule_index].ip == ip_masked)
 			return rule_index;
 	}
 
@@ -484,9 +501,11 @@ _tbl8_alloc(struct rte_lpm *lpm)
 {
 	uint32_t group_idx; /* tbl8 group index. */
 	struct rte_lpm_tbl_entry *tbl8_entry;
+	struct __rte_lpm *i_lpm;
 
+	i_lpm = container_of(lpm, struct __rte_lpm, lpm);
 	/* Scan through tbl8 to find a free (i.e. INVALID) tbl8 group. */
-	for (group_idx = 0; group_idx < lpm->number_tbl8s; group_idx++) {
+	for (group_idx = 0; group_idx < i_lpm->number_tbl8s; group_idx++) {
 		tbl8_entry = &lpm->tbl8[group_idx *
 					RTE_LPM_TBL8_GROUP_NUM_ENTRIES];
 		/* If a free tbl8 group is found clean it and set as VALID. */
@@ -844,6 +863,7 @@ uint32_t *next_hop)
 {
 	uint32_t ip_masked;
 	int32_t rule_index;
+	struct __rte_lpm *internal_lpm;
 
 	/* Check user arguments. */
 	if ((lpm == NULL) ||
@@ -855,8 +875,9 @@ uint32_t *next_hop)
 	ip_masked = ip & depth_to_mask(depth);
 	rule_index = rule_find(lpm, ip_masked, depth);
 
+	internal_lpm = container_of(lpm, struct __rte_lpm, lpm);
 	if (rule_index >= 0) {
-		*next_hop = lpm->rules_tbl[rule_index].next_hop;
+		*next_hop = internal_lpm->rules_tbl[rule_index].next_hop;
 		return 1;
 	}
 
@@ -897,7 +918,9 @@ delete_depth_small(struct rte_lpm *lpm, uint32_t ip_masked,
 	tbl24_range = depth_to_range(depth);
 	tbl24_index = (ip_masked >> 8);
 	struct rte_lpm_tbl_entry zero_tbl24_entry = {0};
+	struct __rte_lpm *i_lpm;
 
+	i_lpm = container_of(lpm, struct __rte_lpm, lpm);
 	/*
 	 * Firstly check the sub_rule_index. A -1 indicates no replacement rule
 	 * and a positive number indicates a sub_rule_index.
@@ -939,7 +962,7 @@ delete_depth_small(struct rte_lpm *lpm, uint32_t ip_masked,
 		 */
 
 		struct rte_lpm_tbl_entry new_tbl24_entry = {
-			.next_hop = lpm->rules_tbl[sub_rule_index].next_hop,
+			.next_hop = i_lpm->rules_tbl[sub_rule_index].next_hop,
 			.valid = VALID,
 			.valid_group = 0,
 			.depth = sub_rule_depth,
@@ -949,7 +972,7 @@ delete_depth_small(struct rte_lpm *lpm, uint32_t ip_masked,
 			.valid = VALID,
 			.valid_group = VALID,
 			.depth = sub_rule_depth,
-			.next_hop = lpm->rules_tbl
+			.next_hop = i_lpm->rules_tbl
 			[sub_rule_index].next_hop,
 		};
 
@@ -1048,6 +1071,7 @@ delete_depth_big(struct rte_lpm *lpm, uint32_t ip_masked,
 	uint32_t tbl24_index, tbl8_group_index, tbl8_group_start, tbl8_index,
 			tbl8_range, i;
 	int32_t tbl8_recycle_index, status = 0;
+	struct __rte_lpm *i_lpm;
 
 	/*
 	 * Calculate the index into tbl24 and range. Note: All depths larger
@@ -1061,6 +1085,7 @@ delete_depth_big(struct rte_lpm *lpm, uint32_t ip_masked,
 	tbl8_index = tbl8_group_start + (ip_masked & 0xFF);
 	tbl8_range = depth_to_range(depth);
 
+	i_lpm = container_of(lpm, struct __rte_lpm, lpm);
 	if (sub_rule_index < 0) {
 		/*
 		 * Loop through the range of entries on tbl8 for which the
@@ -1076,7 +1101,7 @@ delete_depth_big(struct rte_lpm *lpm, uint32_t ip_masked,
 			.valid = VALID,
 			.depth = sub_rule_depth,
 			.valid_group = lpm->tbl8[tbl8_group_start].valid_group,
-			.next_hop = lpm->rules_tbl[sub_rule_index].next_hop,
+			.next_hop = i_lpm->rules_tbl[sub_rule_index].next_hop,
 		};
 
 		/*
@@ -1188,16 +1213,21 @@ rte_lpm_delete(struct rte_lpm *lpm, uint32_t ip, uint8_t depth)
 void
 rte_lpm_delete_all(struct rte_lpm *lpm)
 {
+	struct __rte_lpm *internal_lpm;
+
+	internal_lpm = container_of(lpm, struct __rte_lpm, lpm);
 	/* Zero rule information. */
-	memset(lpm->rule_info, 0, sizeof(lpm->rule_info));
+	memset(internal_lpm->rule_info, 0, sizeof(internal_lpm->rule_info));
 
 	/* Zero tbl24. */
 	memset(lpm->tbl24, 0, sizeof(lpm->tbl24));
 
 	/* Zero tbl8. */
 	memset(lpm->tbl8, 0, sizeof(lpm->tbl8[0])
-			* RTE_LPM_TBL8_GROUP_NUM_ENTRIES * lpm->number_tbl8s);
+			* RTE_LPM_TBL8_GROUP_NUM_ENTRIES
+			* internal_lpm->number_tbl8s);
 
 	/* Delete all rules form the rules table. */
-	memset(lpm->rules_tbl, 0, sizeof(lpm->rules_tbl[0]) * lpm->max_rules);
+	memset(internal_lpm->rules_tbl, 0,
+		sizeof(internal_lpm->rules_tbl[0]) * internal_lpm->max_rules);
 }
diff --git a/lib/librte_lpm/rte_lpm.h b/lib/librte_lpm/rte_lpm.h
index 03da2d37e..112d96f37 100644
--- a/lib/librte_lpm/rte_lpm.h
+++ b/lib/librte_lpm/rte_lpm.h
@@ -132,17 +132,10 @@ struct rte_lpm_rule_info {
 
 /** @internal LPM structure. */
 struct rte_lpm {
-	/* LPM metadata. */
-	char name[RTE_LPM_NAMESIZE];        /**< Name of the lpm. */
-	uint32_t max_rules; /**< Max. balanced rules per lpm. */
-	uint32_t number_tbl8s; /**< Number of tbl8s. */
-	struct rte_lpm_rule_info rule_info[RTE_LPM_MAX_DEPTH]; /**< Rule info table. */
-
 	/* LPM Tables. */
 	struct rte_lpm_tbl_entry tbl24[RTE_LPM_TBL24_NUM_ENTRIES]
 			__rte_cache_aligned; /**< LPM tbl24 table. */
 	struct rte_lpm_tbl_entry *tbl8; /**< LPM tbl8 table. */
-	struct rte_lpm_rule *rules_tbl; /**< LPM rules. */
 };
 
 /** LPM RCU QSBR configuration structure. */
-- 
2.17.1

Re: [dpdk-dev] [PATCH 2/2] lpm: hide internal data

[Bruce Richardson]

On Mon, Sep 07, 2020 at 04:15:17PM +0800, Ruifeng Wang wrote:
> Fields except tbl24 and tbl8 in rte_lpm structure have no
> need to be exposed to the user.
> Hide the unneeded exposure of structure fields for better
> ABI maintainability.
> 
> Suggested-by: David Marchand <david.marchand@redhat.com>
> Signed-off-by: Ruifeng Wang <ruifeng.wang@arm.com>
> Reviewed-by: Phil Yang <phil.yang@arm.com>
> ---
>  lib/librte_lpm/rte_lpm.c | 152 +++++++++++++++++++++++----------------
>  lib/librte_lpm/rte_lpm.h |   7 --
>  2 files changed, 91 insertions(+), 68 deletions(-)
> 
<snip>
> diff --git a/lib/librte_lpm/rte_lpm.h b/lib/librte_lpm/rte_lpm.h
> index 03da2d37e..112d96f37 100644
> --- a/lib/librte_lpm/rte_lpm.h
> +++ b/lib/librte_lpm/rte_lpm.h
> @@ -132,17 +132,10 @@ struct rte_lpm_rule_info {
>  
>  /** @internal LPM structure. */
>  struct rte_lpm {
> -	/* LPM metadata. */
> -	char name[RTE_LPM_NAMESIZE];        /**< Name of the lpm. */
> -	uint32_t max_rules; /**< Max. balanced rules per lpm. */
> -	uint32_t number_tbl8s; /**< Number of tbl8s. */
> -	struct rte_lpm_rule_info rule_info[RTE_LPM_MAX_DEPTH]; /**< Rule info table. */
> -
>  	/* LPM Tables. */
>  	struct rte_lpm_tbl_entry tbl24[RTE_LPM_TBL24_NUM_ENTRIES]
>  			__rte_cache_aligned; /**< LPM tbl24 table. */
>  	struct rte_lpm_tbl_entry *tbl8; /**< LPM tbl8 table. */
> -	struct rte_lpm_rule *rules_tbl; /**< LPM rules. */
>  };
>  

Since this changes the ABI, does it not need advance notice?

[Basically the return value point from rte_lpm_create() will be different,
and that return value could be used by rte_lpm_lookup() which as a static
inline function will be in the binary and using the old structure offsets.]

>  /** LPM RCU QSBR configuration structure. */
> -- 
> 2.17.1
> 

Re: [dpdk-dev] [PATCH 2/2] lpm: hide internal data

[Medvedkin, Vladimir]

Hi Ruifeng,

On 15/09/2020 17:02, Bruce Richardson wrote:
> On Mon, Sep 07, 2020 at 04:15:17PM +0800, Ruifeng Wang wrote:
>> Fields except tbl24 and tbl8 in rte_lpm structure have no
>> need to be exposed to the user.
>> Hide the unneeded exposure of structure fields for better
>> ABI maintainability.
>>
>> Suggested-by: David Marchand <david.marchand@redhat.com>
>> Signed-off-by: Ruifeng Wang <ruifeng.wang@arm.com>
>> Reviewed-by: Phil Yang <phil.yang@arm.com>
>> ---
>>   lib/librte_lpm/rte_lpm.c | 152 +++++++++++++++++++++++----------------
>>   lib/librte_lpm/rte_lpm.h |   7 --
>>   2 files changed, 91 insertions(+), 68 deletions(-)
>>
> <snip>
>> diff --git a/lib/librte_lpm/rte_lpm.h b/lib/librte_lpm/rte_lpm.h
>> index 03da2d37e..112d96f37 100644
>> --- a/lib/librte_lpm/rte_lpm.h
>> +++ b/lib/librte_lpm/rte_lpm.h
>> @@ -132,17 +132,10 @@ struct rte_lpm_rule_info {
>>   
>>   /** @internal LPM structure. */
>>   struct rte_lpm {
>> -	/* LPM metadata. */
>> -	char name[RTE_LPM_NAMESIZE];        /**< Name of the lpm. */
>> -	uint32_t max_rules; /**< Max. balanced rules per lpm. */
>> -	uint32_t number_tbl8s; /**< Number of tbl8s. */
>> -	struct rte_lpm_rule_info rule_info[RTE_LPM_MAX_DEPTH]; /**< Rule info table. */
>> -
>>   	/* LPM Tables. */
>>   	struct rte_lpm_tbl_entry tbl24[RTE_LPM_TBL24_NUM_ENTRIES]
>>   			__rte_cache_aligned; /**< LPM tbl24 table. */
>>   	struct rte_lpm_tbl_entry *tbl8; /**< LPM tbl8 table. */
>> -	struct rte_lpm_rule *rules_tbl; /**< LPM rules. */
>>   };
>>   
> 
> Since this changes the ABI, does it not need advance notice?
> 
> [Basically the return value point from rte_lpm_create() will be different,
> and that return value could be used by rte_lpm_lookup() which as a static
> inline function will be in the binary and using the old structure offsets.]
> 

Agree with Bruce, this patch breaks ABI, so it can't be accepted without 
prior notice.

>>   /** LPM RCU QSBR configuration structure. */
>> -- 
>> 2.17.1
>>

-- 
Regards,
Vladimir

Re: [dpdk-dev] [PATCH 2/2] lpm: hide internal data

[Ruifeng Wang]

> -----Original Message-----
> From: Medvedkin, Vladimir <vladimir.medvedkin@intel.com>
> Sent: Wednesday, September 16, 2020 12:28 AM
> To: Bruce Richardson <bruce.richardson@intel.com>; Ruifeng Wang
> <Ruifeng.Wang@arm.com>
> Cc: dev@dpdk.org; Honnappa Nagarahalli
> <Honnappa.Nagarahalli@arm.com>; nd <nd@arm.com>
> Subject: Re: [PATCH 2/2] lpm: hide internal data
> 
> Hi Ruifeng,
> 
> On 15/09/2020 17:02, Bruce Richardson wrote:
> > On Mon, Sep 07, 2020 at 04:15:17PM +0800, Ruifeng Wang wrote:
> >> Fields except tbl24 and tbl8 in rte_lpm structure have no need to be
> >> exposed to the user.
> >> Hide the unneeded exposure of structure fields for better ABI
> >> maintainability.
> >>
> >> Suggested-by: David Marchand <david.marchand@redhat.com>
> >> Signed-off-by: Ruifeng Wang <ruifeng.wang@arm.com>
> >> Reviewed-by: Phil Yang <phil.yang@arm.com>
> >> ---
> >>   lib/librte_lpm/rte_lpm.c | 152 +++++++++++++++++++++++---------------
> -
> >>   lib/librte_lpm/rte_lpm.h |   7 --
> >>   2 files changed, 91 insertions(+), 68 deletions(-)
> >>
> > <snip>
> >> diff --git a/lib/librte_lpm/rte_lpm.h b/lib/librte_lpm/rte_lpm.h
> >> index 03da2d37e..112d96f37 100644
> >> --- a/lib/librte_lpm/rte_lpm.h
> >> +++ b/lib/librte_lpm/rte_lpm.h
> >> @@ -132,17 +132,10 @@ struct rte_lpm_rule_info {
> >>
> >>   /** @internal LPM structure. */
> >>   struct rte_lpm {
> >> -	/* LPM metadata. */
> >> -	char name[RTE_LPM_NAMESIZE];        /**< Name of the lpm. */
> >> -	uint32_t max_rules; /**< Max. balanced rules per lpm. */
> >> -	uint32_t number_tbl8s; /**< Number of tbl8s. */
> >> -	struct rte_lpm_rule_info rule_info[RTE_LPM_MAX_DEPTH]; /**<
> Rule info table. */
> >> -
> >>   	/* LPM Tables. */
> >>   	struct rte_lpm_tbl_entry tbl24[RTE_LPM_TBL24_NUM_ENTRIES]
> >>   			__rte_cache_aligned; /**< LPM tbl24 table. */
> >>   	struct rte_lpm_tbl_entry *tbl8; /**< LPM tbl8 table. */
> >> -	struct rte_lpm_rule *rules_tbl; /**< LPM rules. */
> >>   };
> >>
> >
> > Since this changes the ABI, does it not need advance notice?
> >
> > [Basically the return value point from rte_lpm_create() will be
> > different, and that return value could be used by rte_lpm_lookup()
> > which as a static inline function will be in the binary and using the
> > old structure offsets.]
> >
> 
> Agree with Bruce, this patch breaks ABI, so it can't be accepted without prior
> notice.
> 
So if the change wants to happen in 20.11, a deprecation notice should have been
added in 20.08.
I should have added a deprecation notice. This change will have to wait for next ABI update window.

Thanks.
Ruifeng
> >>   /** LPM RCU QSBR configuration structure. */
> >> --
> >> 2.17.1
> >>
> 
> --
> Regards,
> Vladimir

Re: [dpdk-dev] [PATCH 2/2] lpm: hide internal data

[Kevin Traynor]

On 16/09/2020 04:17, Ruifeng Wang wrote:
> 
>> -----Original Message-----
>> From: Medvedkin, Vladimir <vladimir.medvedkin@intel.com>
>> Sent: Wednesday, September 16, 2020 12:28 AM
>> To: Bruce Richardson <bruce.richardson@intel.com>; Ruifeng Wang
>> <Ruifeng.Wang@arm.com>
>> Cc: dev@dpdk.org; Honnappa Nagarahalli
>> <Honnappa.Nagarahalli@arm.com>; nd <nd@arm.com>
>> Subject: Re: [PATCH 2/2] lpm: hide internal data
>>
>> Hi Ruifeng,
>>
>> On 15/09/2020 17:02, Bruce Richardson wrote:
>>> On Mon, Sep 07, 2020 at 04:15:17PM +0800, Ruifeng Wang wrote:
>>>> Fields except tbl24 and tbl8 in rte_lpm structure have no need to be
>>>> exposed to the user.
>>>> Hide the unneeded exposure of structure fields for better ABI
>>>> maintainability.
>>>>
>>>> Suggested-by: David Marchand <david.marchand@redhat.com>
>>>> Signed-off-by: Ruifeng Wang <ruifeng.wang@arm.com>
>>>> Reviewed-by: Phil Yang <phil.yang@arm.com>
>>>> ---
>>>>   lib/librte_lpm/rte_lpm.c | 152 +++++++++++++++++++++++---------------
>> -
>>>>   lib/librte_lpm/rte_lpm.h |   7 --
>>>>   2 files changed, 91 insertions(+), 68 deletions(-)
>>>>
>>> <snip>
>>>> diff --git a/lib/librte_lpm/rte_lpm.h b/lib/librte_lpm/rte_lpm.h
>>>> index 03da2d37e..112d96f37 100644
>>>> --- a/lib/librte_lpm/rte_lpm.h
>>>> +++ b/lib/librte_lpm/rte_lpm.h
>>>> @@ -132,17 +132,10 @@ struct rte_lpm_rule_info {
>>>>
>>>>   /** @internal LPM structure. */
>>>>   struct rte_lpm {
>>>> -	/* LPM metadata. */
>>>> -	char name[RTE_LPM_NAMESIZE];        /**< Name of the lpm. */
>>>> -	uint32_t max_rules; /**< Max. balanced rules per lpm. */
>>>> -	uint32_t number_tbl8s; /**< Number of tbl8s. */
>>>> -	struct rte_lpm_rule_info rule_info[RTE_LPM_MAX_DEPTH]; /**<
>> Rule info table. */
>>>> -
>>>>   	/* LPM Tables. */
>>>>   	struct rte_lpm_tbl_entry tbl24[RTE_LPM_TBL24_NUM_ENTRIES]
>>>>   			__rte_cache_aligned; /**< LPM tbl24 table. */
>>>>   	struct rte_lpm_tbl_entry *tbl8; /**< LPM tbl8 table. */
>>>> -	struct rte_lpm_rule *rules_tbl; /**< LPM rules. */
>>>>   };
>>>>
>>>
>>> Since this changes the ABI, does it not need advance notice?
>>>
>>> [Basically the return value point from rte_lpm_create() will be
>>> different, and that return value could be used by rte_lpm_lookup()
>>> which as a static inline function will be in the binary and using the
>>> old structure offsets.]
>>>
>>
>> Agree with Bruce, this patch breaks ABI, so it can't be accepted without prior
>> notice.
>>
> So if the change wants to happen in 20.11, a deprecation notice should have been
> added in 20.08.
> I should have added a deprecation notice. This change will have to wait for next ABI update window.
> 

Do you plan to extend? or is this just speculative?

A quick scan and there seems to be several projects using some of these
members that you are proposing to hide. e.g. BESS, NFF-Go, DPVS,
gatekeeper. I didn't look at the details to see if they are really needed.

Not sure how much notice they'd need or if they update DPDK much, but I
think it's worth having a closer look as to how they use lpm and what
the impact to them is.

> Thanks.
> Ruifeng
>>>>   /** LPM RCU QSBR configuration structure. */
>>>> --
>>>> 2.17.1
>>>>
>>
>> --
>> Regards,
>> Vladimir

Re: [dpdk-dev] [PATCH 2/2] lpm: hide internal data

[Ruifeng Wang]

> -----Original Message-----
> From: Kevin Traynor <ktraynor@redhat.com>
> Sent: Wednesday, September 30, 2020 4:46 PM
> To: Ruifeng Wang <Ruifeng.Wang@arm.com>; Medvedkin, Vladimir
> <vladimir.medvedkin@intel.com>; Bruce Richardson
> <bruce.richardson@intel.com>
> Cc: dev@dpdk.org; Honnappa Nagarahalli
> <Honnappa.Nagarahalli@arm.com>; nd <nd@arm.com>
> Subject: Re: [dpdk-dev] [PATCH 2/2] lpm: hide internal data
> 
> On 16/09/2020 04:17, Ruifeng Wang wrote:
> >
> >> -----Original Message-----
> >> From: Medvedkin, Vladimir <vladimir.medvedkin@intel.com>
> >> Sent: Wednesday, September 16, 2020 12:28 AM
> >> To: Bruce Richardson <bruce.richardson@intel.com>; Ruifeng Wang
> >> <Ruifeng.Wang@arm.com>
> >> Cc: dev@dpdk.org; Honnappa Nagarahalli
> >> <Honnappa.Nagarahalli@arm.com>; nd <nd@arm.com>
> >> Subject: Re: [PATCH 2/2] lpm: hide internal data
> >>
> >> Hi Ruifeng,
> >>
> >> On 15/09/2020 17:02, Bruce Richardson wrote:
> >>> On Mon, Sep 07, 2020 at 04:15:17PM +0800, Ruifeng Wang wrote:
> >>>> Fields except tbl24 and tbl8 in rte_lpm structure have no need to
> >>>> be exposed to the user.
> >>>> Hide the unneeded exposure of structure fields for better ABI
> >>>> maintainability.
> >>>>
> >>>> Suggested-by: David Marchand <david.marchand@redhat.com>
> >>>> Signed-off-by: Ruifeng Wang <ruifeng.wang@arm.com>
> >>>> Reviewed-by: Phil Yang <phil.yang@arm.com>
> >>>> ---
> >>>>   lib/librte_lpm/rte_lpm.c | 152
> >>>> +++++++++++++++++++++++---------------
> >> -
> >>>>   lib/librte_lpm/rte_lpm.h |   7 --
> >>>>   2 files changed, 91 insertions(+), 68 deletions(-)
> >>>>
> >>> <snip>
> >>>> diff --git a/lib/librte_lpm/rte_lpm.h b/lib/librte_lpm/rte_lpm.h
> >>>> index 03da2d37e..112d96f37 100644
> >>>> --- a/lib/librte_lpm/rte_lpm.h
> >>>> +++ b/lib/librte_lpm/rte_lpm.h
> >>>> @@ -132,17 +132,10 @@ struct rte_lpm_rule_info {
> >>>>
> >>>>   /** @internal LPM structure. */
> >>>>   struct rte_lpm {
> >>>> -	/* LPM metadata. */
> >>>> -	char name[RTE_LPM_NAMESIZE];        /**< Name of the lpm. */
> >>>> -	uint32_t max_rules; /**< Max. balanced rules per lpm. */
> >>>> -	uint32_t number_tbl8s; /**< Number of tbl8s. */
> >>>> -	struct rte_lpm_rule_info rule_info[RTE_LPM_MAX_DEPTH]; /**<
> >> Rule info table. */
> >>>> -
> >>>>   	/* LPM Tables. */
> >>>>   	struct rte_lpm_tbl_entry tbl24[RTE_LPM_TBL24_NUM_ENTRIES]
> >>>>   			__rte_cache_aligned; /**< LPM tbl24 table. */
> >>>>   	struct rte_lpm_tbl_entry *tbl8; /**< LPM tbl8 table. */
> >>>> -	struct rte_lpm_rule *rules_tbl; /**< LPM rules. */
> >>>>   };
> >>>>
> >>>
> >>> Since this changes the ABI, does it not need advance notice?
> >>>
> >>> [Basically the return value point from rte_lpm_create() will be
> >>> different, and that return value could be used by rte_lpm_lookup()
> >>> which as a static inline function will be in the binary and using
> >>> the old structure offsets.]
> >>>
> >>
> >> Agree with Bruce, this patch breaks ABI, so it can't be accepted
> >> without prior notice.
> >>
> > So if the change wants to happen in 20.11, a deprecation notice should
> > have been added in 20.08.
> > I should have added a deprecation notice. This change will have to wait for
> next ABI update window.
> >
> 
> Do you plan to extend? or is this just speculative?
It is speculative.

> 
> A quick scan and there seems to be several projects using some of these
> members that you are proposing to hide. e.g. BESS, NFF-Go, DPVS,
> gatekeeper. I didn't look at the details to see if they are really needed.
> 
> Not sure how much notice they'd need or if they update DPDK much, but I
> think it's worth having a closer look as to how they use lpm and what the
> impact to them is.
Checked the projects listed above. BESS, NFF-Go and DPVS don't access the members to be hided.
They will not be impacted by this patch.
But Gatekeeper accesses the rte_lpm internal members that to be hided. Its compilation will be broken with this patch.

> 
> > Thanks.
> > Ruifeng
> >>>>   /** LPM RCU QSBR configuration structure. */
> >>>> --
> >>>> 2.17.1
> >>>>
> >>
> >> --
> >> Regards,
> >> Vladimir

Re: [dpdk-dev] [PATCH 2/2] lpm: hide internal data

[Kevin Traynor]

Hi Gatekeeper maintainers (I think),

fyi - there is a proposal to remove some members of a struct in DPDK LPM
API that Gatekeeper is using [1]. It would be only from DPDK 20.11 but
as it's an LTS I guess it would probably hit Debian in a few months.

The full thread is here:
http://inbox.dpdk.org/dev/20200907081518.46350-1-ruifeng.wang@arm.com/

Maybe you can take a look and tell us if they are needed in Gatekeeper
or you can workaround it?

thanks,
Kevin.

[1]
https://github.com/AltraMayor/gatekeeper/blob/master/gt/lua_lpm.c#L235-L248

On 09/10/2020 07:54, Ruifeng Wang wrote:
> 
>> -----Original Message-----
>> From: Kevin Traynor <ktraynor@redhat.com>
>> Sent: Wednesday, September 30, 2020 4:46 PM
>> To: Ruifeng Wang <Ruifeng.Wang@arm.com>; Medvedkin, Vladimir
>> <vladimir.medvedkin@intel.com>; Bruce Richardson
>> <bruce.richardson@intel.com>
>> Cc: dev@dpdk.org; Honnappa Nagarahalli
>> <Honnappa.Nagarahalli@arm.com>; nd <nd@arm.com>
>> Subject: Re: [dpdk-dev] [PATCH 2/2] lpm: hide internal data
>>
>> On 16/09/2020 04:17, Ruifeng Wang wrote:
>>>
>>>> -----Original Message-----
>>>> From: Medvedkin, Vladimir <vladimir.medvedkin@intel.com>
>>>> Sent: Wednesday, September 16, 2020 12:28 AM
>>>> To: Bruce Richardson <bruce.richardson@intel.com>; Ruifeng Wang
>>>> <Ruifeng.Wang@arm.com>
>>>> Cc: dev@dpdk.org; Honnappa Nagarahalli
>>>> <Honnappa.Nagarahalli@arm.com>; nd <nd@arm.com>
>>>> Subject: Re: [PATCH 2/2] lpm: hide internal data
>>>>
>>>> Hi Ruifeng,
>>>>
>>>> On 15/09/2020 17:02, Bruce Richardson wrote:
>>>>> On Mon, Sep 07, 2020 at 04:15:17PM +0800, Ruifeng Wang wrote:
>>>>>> Fields except tbl24 and tbl8 in rte_lpm structure have no need to
>>>>>> be exposed to the user.
>>>>>> Hide the unneeded exposure of structure fields for better ABI
>>>>>> maintainability.
>>>>>>
>>>>>> Suggested-by: David Marchand <david.marchand@redhat.com>
>>>>>> Signed-off-by: Ruifeng Wang <ruifeng.wang@arm.com>
>>>>>> Reviewed-by: Phil Yang <phil.yang@arm.com>
>>>>>> ---
>>>>>>   lib/librte_lpm/rte_lpm.c | 152
>>>>>> +++++++++++++++++++++++---------------
>>>> -
>>>>>>   lib/librte_lpm/rte_lpm.h |   7 --
>>>>>>   2 files changed, 91 insertions(+), 68 deletions(-)
>>>>>>
>>>>> <snip>
>>>>>> diff --git a/lib/librte_lpm/rte_lpm.h b/lib/librte_lpm/rte_lpm.h
>>>>>> index 03da2d37e..112d96f37 100644
>>>>>> --- a/lib/librte_lpm/rte_lpm.h
>>>>>> +++ b/lib/librte_lpm/rte_lpm.h
>>>>>> @@ -132,17 +132,10 @@ struct rte_lpm_rule_info {
>>>>>>
>>>>>>   /** @internal LPM structure. */
>>>>>>   struct rte_lpm {
>>>>>> -	/* LPM metadata. */
>>>>>> -	char name[RTE_LPM_NAMESIZE];        /**< Name of the lpm. */
>>>>>> -	uint32_t max_rules; /**< Max. balanced rules per lpm. */
>>>>>> -	uint32_t number_tbl8s; /**< Number of tbl8s. */
>>>>>> -	struct rte_lpm_rule_info rule_info[RTE_LPM_MAX_DEPTH]; /**<
>>>> Rule info table. */
>>>>>> -
>>>>>>   	/* LPM Tables. */
>>>>>>   	struct rte_lpm_tbl_entry tbl24[RTE_LPM_TBL24_NUM_ENTRIES]
>>>>>>   			__rte_cache_aligned; /**< LPM tbl24 table. */
>>>>>>   	struct rte_lpm_tbl_entry *tbl8; /**< LPM tbl8 table. */
>>>>>> -	struct rte_lpm_rule *rules_tbl; /**< LPM rules. */
>>>>>>   };
>>>>>>
>>>>>
>>>>> Since this changes the ABI, does it not need advance notice?
>>>>>
>>>>> [Basically the return value point from rte_lpm_create() will be
>>>>> different, and that return value could be used by rte_lpm_lookup()
>>>>> which as a static inline function will be in the binary and using
>>>>> the old structure offsets.]
>>>>>
>>>>
>>>> Agree with Bruce, this patch breaks ABI, so it can't be accepted
>>>> without prior notice.
>>>>
>>> So if the change wants to happen in 20.11, a deprecation notice should
>>> have been added in 20.08.
>>> I should have added a deprecation notice. This change will have to wait for
>> next ABI update window.
>>>
>>
>> Do you plan to extend? or is this just speculative?
> It is speculative.
> 
>>
>> A quick scan and there seems to be several projects using some of these
>> members that you are proposing to hide. e.g. BESS, NFF-Go, DPVS,
>> gatekeeper. I didn't look at the details to see if they are really needed.
>>
>> Not sure how much notice they'd need or if they update DPDK much, but I
>> think it's worth having a closer look as to how they use lpm and what the
>> impact to them is.
> Checked the projects listed above. BESS, NFF-Go and DPVS don't access the members to be hided.
> They will not be impacted by this patch.
> But Gatekeeper accesses the rte_lpm internal members that to be hided. Its compilation will be broken with this patch.
> 
>>
>>> Thanks.
>>> Ruifeng
>>>>>>   /** LPM RCU QSBR configuration structure. */
>>>>>> --
>>>>>> 2.17.1
>>>>>>
>>>>
>>>> --
>>>> Regards,
>>>> Vladimir
> 

Re: [dpdk-dev] [PATCH 2/2] lpm: hide internal data

[Michel Machado]

Hi Kevin,

    We do need fields max_rules and number_tbl8s of struct rte_lpm, so 
the removal would force us to have another patch to our local copy of 
DPDK. We'd rather avoid this new local patch because we wish to 
eventually be in sync with the stock DPDK.

    Those fields are needed in Gatekeeper because we found a condition 
in an ongoing deployment in which the entries of some LPM tables may 
suddenly change a lot to reflect policy changes. To avoid getting into a 
state in which the LPM table is inconsistent because it cannot fit all 
the new entries, we compute the needed parameters to support the new 
entries, and compare with the current parameters. If the current table 
doesn't fit everything, we have to replace it with a new LPM table.

    If there were a way to obtain the struct rte_lpm_config of a given 
LPM table, it would cleanly address our need. We have the same need in 
IPv6 and have a local patch to work around it (see 
https://github.com/cjdoucette/dpdk/commit/3eaf124a781349b8ec8cd880db26a78115cb8c8f). 
Thus, an IPv4 and IPv6 solution would be best.

    PS: I've added Qiaobin Fu, another Gatekeeper maintainer, to this 
disscussion.

[ ]'s
Michel Machado

On 10/13/20 9:53 AM, Kevin Traynor wrote:
> Hi Gatekeeper maintainers (I think),
> 
> fyi - there is a proposal to remove some members of a struct in DPDK LPM
> API that Gatekeeper is using [1]. It would be only from DPDK 20.11 but
> as it's an LTS I guess it would probably hit Debian in a few months.
> 
> The full thread is here:
> http://inbox.dpdk.org/dev/20200907081518.46350-1-ruifeng.wang@arm.com/
> 
> Maybe you can take a look and tell us if they are needed in Gatekeeper
> or you can workaround it?
> 
> thanks,
> Kevin.
> 
> [1]
> https://github.com/AltraMayor/gatekeeper/blob/master/gt/lua_lpm.c#L235-L248
> 
> On 09/10/2020 07:54, Ruifeng Wang wrote:
>>
>>> -----Original Message-----
>>> From: Kevin Traynor <ktraynor@redhat.com>
>>> Sent: Wednesday, September 30, 2020 4:46 PM
>>> To: Ruifeng Wang <Ruifeng.Wang@arm.com>; Medvedkin, Vladimir
>>> <vladimir.medvedkin@intel.com>; Bruce Richardson
>>> <bruce.richardson@intel.com>
>>> Cc: dev@dpdk.org; Honnappa Nagarahalli
>>> <Honnappa.Nagarahalli@arm.com>; nd <nd@arm.com>
>>> Subject: Re: [dpdk-dev] [PATCH 2/2] lpm: hide internal data
>>>
>>> On 16/09/2020 04:17, Ruifeng Wang wrote:
>>>>
>>>>> -----Original Message-----
>>>>> From: Medvedkin, Vladimir <vladimir.medvedkin@intel.com>
>>>>> Sent: Wednesday, September 16, 2020 12:28 AM
>>>>> To: Bruce Richardson <bruce.richardson@intel.com>; Ruifeng Wang
>>>>> <Ruifeng.Wang@arm.com>
>>>>> Cc: dev@dpdk.org; Honnappa Nagarahalli
>>>>> <Honnappa.Nagarahalli@arm.com>; nd <nd@arm.com>
>>>>> Subject: Re: [PATCH 2/2] lpm: hide internal data
>>>>>
>>>>> Hi Ruifeng,
>>>>>
>>>>> On 15/09/2020 17:02, Bruce Richardson wrote:
>>>>>> On Mon, Sep 07, 2020 at 04:15:17PM +0800, Ruifeng Wang wrote:
>>>>>>> Fields except tbl24 and tbl8 in rte_lpm structure have no need to
>>>>>>> be exposed to the user.
>>>>>>> Hide the unneeded exposure of structure fields for better ABI
>>>>>>> maintainability.
>>>>>>>
>>>>>>> Suggested-by: David Marchand <david.marchand@redhat.com>
>>>>>>> Signed-off-by: Ruifeng Wang <ruifeng.wang@arm.com>
>>>>>>> Reviewed-by: Phil Yang <phil.yang@arm.com>
>>>>>>> ---
>>>>>>>    lib/librte_lpm/rte_lpm.c | 152
>>>>>>> +++++++++++++++++++++++---------------
>>>>> -
>>>>>>>    lib/librte_lpm/rte_lpm.h |   7 --
>>>>>>>    2 files changed, 91 insertions(+), 68 deletions(-)
>>>>>>>
>>>>>> <snip>
>>>>>>> diff --git a/lib/librte_lpm/rte_lpm.h b/lib/librte_lpm/rte_lpm.h
>>>>>>> index 03da2d37e..112d96f37 100644
>>>>>>> --- a/lib/librte_lpm/rte_lpm.h
>>>>>>> +++ b/lib/librte_lpm/rte_lpm.h
>>>>>>> @@ -132,17 +132,10 @@ struct rte_lpm_rule_info {
>>>>>>>
>>>>>>>    /** @internal LPM structure. */
>>>>>>>    struct rte_lpm {
>>>>>>> -	/* LPM metadata. */
>>>>>>> -	char name[RTE_LPM_NAMESIZE];        /**< Name of the lpm. */
>>>>>>> -	uint32_t max_rules; /**< Max. balanced rules per lpm. */
>>>>>>> -	uint32_t number_tbl8s; /**< Number of tbl8s. */
>>>>>>> -	struct rte_lpm_rule_info rule_info[RTE_LPM_MAX_DEPTH]; /**<
>>>>> Rule info table. */
>>>>>>> -
>>>>>>>    	/* LPM Tables. */
>>>>>>>    	struct rte_lpm_tbl_entry tbl24[RTE_LPM_TBL24_NUM_ENTRIES]
>>>>>>>    			__rte_cache_aligned; /**< LPM tbl24 table. */
>>>>>>>    	struct rte_lpm_tbl_entry *tbl8; /**< LPM tbl8 table. */
>>>>>>> -	struct rte_lpm_rule *rules_tbl; /**< LPM rules. */
>>>>>>>    };
>>>>>>>
>>>>>>
>>>>>> Since this changes the ABI, does it not need advance notice?
>>>>>>
>>>>>> [Basically the return value point from rte_lpm_create() will be
>>>>>> different, and that return value could be used by rte_lpm_lookup()
>>>>>> which as a static inline function will be in the binary and using
>>>>>> the old structure offsets.]
>>>>>>
>>>>>
>>>>> Agree with Bruce, this patch breaks ABI, so it can't be accepted
>>>>> without prior notice.
>>>>>
>>>> So if the change wants to happen in 20.11, a deprecation notice should
>>>> have been added in 20.08.
>>>> I should have added a deprecation notice. This change will have to wait for
>>> next ABI update window.
>>>>
>>>
>>> Do you plan to extend? or is this just speculative?
>> It is speculative.
>>
>>>
>>> A quick scan and there seems to be several projects using some of these
>>> members that you are proposing to hide. e.g. BESS, NFF-Go, DPVS,
>>> gatekeeper. I didn't look at the details to see if they are really needed.
>>>
>>> Not sure how much notice they'd need or if they update DPDK much, but I
>>> think it's worth having a closer look as to how they use lpm and what the
>>> impact to them is.
>> Checked the projects listed above. BESS, NFF-Go and DPVS don't access the members to be hided.
>> They will not be impacted by this patch.
>> But Gatekeeper accesses the rte_lpm internal members that to be hided. Its compilation will be broken with this patch.
>>
>>>
>>>> Thanks.
>>>> Ruifeng
>>>>>>>    /** LPM RCU QSBR configuration structure. */
>>>>>>> --
>>>>>>> 2.17.1
>>>>>>>
>>>>>
>>>>> --
>>>>> Regards,
>>>>> Vladimir
>>
> 

Re: [dpdk-dev] [PATCH 2/2] lpm: hide internal data

[Medvedkin, Vladimir]

Hi Michel,

Could you please describe a condition when LPM gets inconsistent? As I 
can see if there is no free tbl8 it will return -ENOSPC.

On 13/10/2020 15:58, Michel Machado wrote:
> Hi Kevin,
> 
>     We do need fields max_rules and number_tbl8s of struct rte_lpm, so 
> the removal would force us to have another patch to our local copy of 
> DPDK. We'd rather avoid this new local patch because we wish to 
> eventually be in sync with the stock DPDK.
> 
>     Those fields are needed in Gatekeeper because we found a condition 
> in an ongoing deployment in which the entries of some LPM tables may 
> suddenly change a lot to reflect policy changes. To avoid getting into a 
> state in which the LPM table is inconsistent because it cannot fit all 
> the new entries, we compute the needed parameters to support the new 
> entries, and compare with the current parameters. If the current table 
> doesn't fit everything, we have to replace it with a new LPM table.
> 
>     If there were a way to obtain the struct rte_lpm_config of a given 
> LPM table, it would cleanly address our need. We have the same need in 
> IPv6 and have a local patch to work around it (see 
> https://github.com/cjdoucette/dpdk/commit/3eaf124a781349b8ec8cd880db26a78115cb8c8f). 
> Thus, an IPv4 and IPv6 solution would be best.
> 
>     PS: I've added Qiaobin Fu, another Gatekeeper maintainer, to this 
> disscussion.
> 
> [ ]'s
> Michel Machado
> 
> On 10/13/20 9:53 AM, Kevin Traynor wrote:
>> Hi Gatekeeper maintainers (I think),
>>
>> fyi - there is a proposal to remove some members of a struct in DPDK LPM
>> API that Gatekeeper is using [1]. It would be only from DPDK 20.11 but
>> as it's an LTS I guess it would probably hit Debian in a few months.
>>
>> The full thread is here:
>> http://inbox.dpdk.org/dev/20200907081518.46350-1-ruifeng.wang@arm.com/
>>
>> Maybe you can take a look and tell us if they are needed in Gatekeeper
>> or you can workaround it?
>>
>> thanks,
>> Kevin.
>>
>> [1]
>> https://github.com/AltraMayor/gatekeeper/blob/master/gt/lua_lpm.c#L235-L248 
>>
>>
>> On 09/10/2020 07:54, Ruifeng Wang wrote:
>>>
>>>> -----Original Message-----
>>>> From: Kevin Traynor <ktraynor@redhat.com>
>>>> Sent: Wednesday, September 30, 2020 4:46 PM
>>>> To: Ruifeng Wang <Ruifeng.Wang@arm.com>; Medvedkin, Vladimir
>>>> <vladimir.medvedkin@intel.com>; Bruce Richardson
>>>> <bruce.richardson@intel.com>
>>>> Cc: dev@dpdk.org; Honnappa Nagarahalli
>>>> <Honnappa.Nagarahalli@arm.com>; nd <nd@arm.com>
>>>> Subject: Re: [dpdk-dev] [PATCH 2/2] lpm: hide internal data
>>>>
>>>> On 16/09/2020 04:17, Ruifeng Wang wrote:
>>>>>
>>>>>> -----Original Message-----
>>>>>> From: Medvedkin, Vladimir <vladimir.medvedkin@intel.com>
>>>>>> Sent: Wednesday, September 16, 2020 12:28 AM
>>>>>> To: Bruce Richardson <bruce.richardson@intel.com>; Ruifeng Wang
>>>>>> <Ruifeng.Wang@arm.com>
>>>>>> Cc: dev@dpdk.org; Honnappa Nagarahalli
>>>>>> <Honnappa.Nagarahalli@arm.com>; nd <nd@arm.com>
>>>>>> Subject: Re: [PATCH 2/2] lpm: hide internal data
>>>>>>
>>>>>> Hi Ruifeng,
>>>>>>
>>>>>> On 15/09/2020 17:02, Bruce Richardson wrote:
>>>>>>> On Mon, Sep 07, 2020 at 04:15:17PM +0800, Ruifeng Wang wrote:
>>>>>>>> Fields except tbl24 and tbl8 in rte_lpm structure have no need to
>>>>>>>> be exposed to the user.
>>>>>>>> Hide the unneeded exposure of structure fields for better ABI
>>>>>>>> maintainability.
>>>>>>>>
>>>>>>>> Suggested-by: David Marchand <david.marchand@redhat.com>
>>>>>>>> Signed-off-by: Ruifeng Wang <ruifeng.wang@arm.com>
>>>>>>>> Reviewed-by: Phil Yang <phil.yang@arm.com>
>>>>>>>> ---
>>>>>>>>    lib/librte_lpm/rte_lpm.c | 152
>>>>>>>> +++++++++++++++++++++++---------------
>>>>>> -
>>>>>>>>    lib/librte_lpm/rte_lpm.h |   7 --
>>>>>>>>    2 files changed, 91 insertions(+), 68 deletions(-)
>>>>>>>>
>>>>>>> <snip>
>>>>>>>> diff --git a/lib/librte_lpm/rte_lpm.h b/lib/librte_lpm/rte_lpm.h
>>>>>>>> index 03da2d37e..112d96f37 100644
>>>>>>>> --- a/lib/librte_lpm/rte_lpm.h
>>>>>>>> +++ b/lib/librte_lpm/rte_lpm.h
>>>>>>>> @@ -132,17 +132,10 @@ struct rte_lpm_rule_info {
>>>>>>>>
>>>>>>>>    /** @internal LPM structure. */
>>>>>>>>    struct rte_lpm {
>>>>>>>> -    /* LPM metadata. */
>>>>>>>> -    char name[RTE_LPM_NAMESIZE];        /**< Name of the lpm. */
>>>>>>>> -    uint32_t max_rules; /**< Max. balanced rules per lpm. */
>>>>>>>> -    uint32_t number_tbl8s; /**< Number of tbl8s. */
>>>>>>>> -    struct rte_lpm_rule_info rule_info[RTE_LPM_MAX_DEPTH]; /**<
>>>>>> Rule info table. */
>>>>>>>> -
>>>>>>>>        /* LPM Tables. */
>>>>>>>>        struct rte_lpm_tbl_entry tbl24[RTE_LPM_TBL24_NUM_ENTRIES]
>>>>>>>>                __rte_cache_aligned; /**< LPM tbl24 table. */
>>>>>>>>        struct rte_lpm_tbl_entry *tbl8; /**< LPM tbl8 table. */
>>>>>>>> -    struct rte_lpm_rule *rules_tbl; /**< LPM rules. */
>>>>>>>>    };
>>>>>>>>
>>>>>>>
>>>>>>> Since this changes the ABI, does it not need advance notice?
>>>>>>>
>>>>>>> [Basically the return value point from rte_lpm_create() will be
>>>>>>> different, and that return value could be used by rte_lpm_lookup()
>>>>>>> which as a static inline function will be in the binary and using
>>>>>>> the old structure offsets.]
>>>>>>>
>>>>>>
>>>>>> Agree with Bruce, this patch breaks ABI, so it can't be accepted
>>>>>> without prior notice.
>>>>>>
>>>>> So if the change wants to happen in 20.11, a deprecation notice should
>>>>> have been added in 20.08.
>>>>> I should have added a deprecation notice. This change will have to 
>>>>> wait for
>>>> next ABI update window.
>>>>>
>>>>
>>>> Do you plan to extend? or is this just speculative?
>>> It is speculative.
>>>
>>>>
>>>> A quick scan and there seems to be several projects using some of these
>>>> members that you are proposing to hide. e.g. BESS, NFF-Go, DPVS,
>>>> gatekeeper. I didn't look at the details to see if they are really 
>>>> needed.
>>>>
>>>> Not sure how much notice they'd need or if they update DPDK much, but I
>>>> think it's worth having a closer look as to how they use lpm and 
>>>> what the
>>>> impact to them is.
>>> Checked the projects listed above. BESS, NFF-Go and DPVS don't access 
>>> the members to be hided.
>>> They will not be impacted by this patch.
>>> But Gatekeeper accesses the rte_lpm internal members that to be 
>>> hided. Its compilation will be broken with this patch.
>>>
>>>>
>>>>> Thanks.
>>>>> Ruifeng
>>>>>>>>    /** LPM RCU QSBR configuration structure. */
>>>>>>>> -- 
>>>>>>>> 2.17.1
>>>>>>>>
>>>>>>
>>>>>> -- 
>>>>>> Regards,
>>>>>> Vladimir
>>>
>>

-- 
Regards,
Vladimir

Re: [dpdk-dev] [PATCH 2/2] lpm: hide internal data

[Michel Machado]

On 10/13/20 11:41 AM, Medvedkin, Vladimir wrote:
> Hi Michel,
> 
> Could you please describe a condition when LPM gets inconsistent? As I 
> can see if there is no free tbl8 it will return -ENOSPC.

    Consider this simple example, we need to add the following two 
prefixes with different next hops: 10.99.0.0/16, 18.99.99.128/25. If the 
LPM table is out of tbl8s, the second prefix is not added and Gatekeeper 
will make decisions in violation of the policy. The data structure of 
the LPM table is consistent, but its content inconsistent with the policy.

    We minimize the need of replacing a LPM table by allocating LPM 
tables with the double of what we need (see example here 
https://github.com/AltraMayor/gatekeeper/blob/95d1d6e8201861a0d0c698bfd06ad606674f1e07/lua/examples/policy.lua#L172-L183), 
but the code must be ready for unexpected needs that may arise in 
production.

> 
> On 13/10/2020 15:58, Michel Machado wrote:
>> Hi Kevin,
>>
>>     We do need fields max_rules and number_tbl8s of struct rte_lpm, so 
>> the removal would force us to have another patch to our local copy of 
>> DPDK. We'd rather avoid this new local patch because we wish to 
>> eventually be in sync with the stock DPDK.
>>
>>     Those fields are needed in Gatekeeper because we found a condition 
>> in an ongoing deployment in which the entries of some LPM tables may 
>> suddenly change a lot to reflect policy changes. To avoid getting into 
>> a state in which the LPM table is inconsistent because it cannot fit 
>> all the new entries, we compute the needed parameters to support the 
>> new entries, and compare with the current parameters. If the current 
>> table doesn't fit everything, we have to replace it with a new LPM table.
>>
>>     If there were a way to obtain the struct rte_lpm_config of a given 
>> LPM table, it would cleanly address our need. We have the same need in 
>> IPv6 and have a local patch to work around it (see 
>> https://github.com/cjdoucette/dpdk/commit/3eaf124a781349b8ec8cd880db26a78115cb8c8f). 
>> Thus, an IPv4 and IPv6 solution would be best.
>>
>>     PS: I've added Qiaobin Fu, another Gatekeeper maintainer, to this 
>> disscussion.
>>
>> [ ]'s
>> Michel Machado
>>
>> On 10/13/20 9:53 AM, Kevin Traynor wrote:
>>> Hi Gatekeeper maintainers (I think),
>>>
>>> fyi - there is a proposal to remove some members of a struct in DPDK LPM
>>> API that Gatekeeper is using [1]. It would be only from DPDK 20.11 but
>>> as it's an LTS I guess it would probably hit Debian in a few months.
>>>
>>> The full thread is here:
>>> http://inbox.dpdk.org/dev/20200907081518.46350-1-ruifeng.wang@arm.com/
>>>
>>> Maybe you can take a look and tell us if they are needed in Gatekeeper
>>> or you can workaround it?
>>>
>>> thanks,
>>> Kevin.
>>>
>>> [1]
>>> https://github.com/AltraMayor/gatekeeper/blob/master/gt/lua_lpm.c#L235-L248 
>>>
>>>
>>> On 09/10/2020 07:54, Ruifeng Wang wrote:
>>>>
>>>>> -----Original Message-----
>>>>> From: Kevin Traynor <ktraynor@redhat.com>
>>>>> Sent: Wednesday, September 30, 2020 4:46 PM
>>>>> To: Ruifeng Wang <Ruifeng.Wang@arm.com>; Medvedkin, Vladimir
>>>>> <vladimir.medvedkin@intel.com>; Bruce Richardson
>>>>> <bruce.richardson@intel.com>
>>>>> Cc: dev@dpdk.org; Honnappa Nagarahalli
>>>>> <Honnappa.Nagarahalli@arm.com>; nd <nd@arm.com>
>>>>> Subject: Re: [dpdk-dev] [PATCH 2/2] lpm: hide internal data
>>>>>
>>>>> On 16/09/2020 04:17, Ruifeng Wang wrote:
>>>>>>
>>>>>>> -----Original Message-----
>>>>>>> From: Medvedkin, Vladimir <vladimir.medvedkin@intel.com>
>>>>>>> Sent: Wednesday, September 16, 2020 12:28 AM
>>>>>>> To: Bruce Richardson <bruce.richardson@intel.com>; Ruifeng Wang
>>>>>>> <Ruifeng.Wang@arm.com>
>>>>>>> Cc: dev@dpdk.org; Honnappa Nagarahalli
>>>>>>> <Honnappa.Nagarahalli@arm.com>; nd <nd@arm.com>
>>>>>>> Subject: Re: [PATCH 2/2] lpm: hide internal data
>>>>>>>
>>>>>>> Hi Ruifeng,
>>>>>>>
>>>>>>> On 15/09/2020 17:02, Bruce Richardson wrote:
>>>>>>>> On Mon, Sep 07, 2020 at 04:15:17PM +0800, Ruifeng Wang wrote:
>>>>>>>>> Fields except tbl24 and tbl8 in rte_lpm structure have no need to
>>>>>>>>> be exposed to the user.
>>>>>>>>> Hide the unneeded exposure of structure fields for better ABI
>>>>>>>>> maintainability.
>>>>>>>>>
>>>>>>>>> Suggested-by: David Marchand <david.marchand@redhat.com>
>>>>>>>>> Signed-off-by: Ruifeng Wang <ruifeng.wang@arm.com>
>>>>>>>>> Reviewed-by: Phil Yang <phil.yang@arm.com>
>>>>>>>>> ---
>>>>>>>>>    lib/librte_lpm/rte_lpm.c | 152
>>>>>>>>> +++++++++++++++++++++++---------------
>>>>>>> -
>>>>>>>>>    lib/librte_lpm/rte_lpm.h |   7 --
>>>>>>>>>    2 files changed, 91 insertions(+), 68 deletions(-)
>>>>>>>>>
>>>>>>>> <snip>
>>>>>>>>> diff --git a/lib/librte_lpm/rte_lpm.h b/lib/librte_lpm/rte_lpm.h
>>>>>>>>> index 03da2d37e..112d96f37 100644
>>>>>>>>> --- a/lib/librte_lpm/rte_lpm.h
>>>>>>>>> +++ b/lib/librte_lpm/rte_lpm.h
>>>>>>>>> @@ -132,17 +132,10 @@ struct rte_lpm_rule_info {
>>>>>>>>>
>>>>>>>>>    /** @internal LPM structure. */
>>>>>>>>>    struct rte_lpm {
>>>>>>>>> -    /* LPM metadata. */
>>>>>>>>> -    char name[RTE_LPM_NAMESIZE];        /**< Name of the lpm. */
>>>>>>>>> -    uint32_t max_rules; /**< Max. balanced rules per lpm. */
>>>>>>>>> -    uint32_t number_tbl8s; /**< Number of tbl8s. */
>>>>>>>>> -    struct rte_lpm_rule_info rule_info[RTE_LPM_MAX_DEPTH]; /**<
>>>>>>> Rule info table. */
>>>>>>>>> -
>>>>>>>>>        /* LPM Tables. */
>>>>>>>>>        struct rte_lpm_tbl_entry tbl24[RTE_LPM_TBL24_NUM_ENTRIES]
>>>>>>>>>                __rte_cache_aligned; /**< LPM tbl24 table. */
>>>>>>>>>        struct rte_lpm_tbl_entry *tbl8; /**< LPM tbl8 table. */
>>>>>>>>> -    struct rte_lpm_rule *rules_tbl; /**< LPM rules. */
>>>>>>>>>    };
>>>>>>>>>
>>>>>>>>
>>>>>>>> Since this changes the ABI, does it not need advance notice?
>>>>>>>>
>>>>>>>> [Basically the return value point from rte_lpm_create() will be
>>>>>>>> different, and that return value could be used by rte_lpm_lookup()
>>>>>>>> which as a static inline function will be in the binary and using
>>>>>>>> the old structure offsets.]
>>>>>>>>
>>>>>>>
>>>>>>> Agree with Bruce, this patch breaks ABI, so it can't be accepted
>>>>>>> without prior notice.
>>>>>>>
>>>>>> So if the change wants to happen in 20.11, a deprecation notice 
>>>>>> should
>>>>>> have been added in 20.08.
>>>>>> I should have added a deprecation notice. This change will have to 
>>>>>> wait for
>>>>> next ABI update window.
>>>>>>
>>>>>
>>>>> Do you plan to extend? or is this just speculative?
>>>> It is speculative.
>>>>
>>>>>
>>>>> A quick scan and there seems to be several projects using some of 
>>>>> these
>>>>> members that you are proposing to hide. e.g. BESS, NFF-Go, DPVS,
>>>>> gatekeeper. I didn't look at the details to see if they are really 
>>>>> needed.
>>>>>
>>>>> Not sure how much notice they'd need or if they update DPDK much, 
>>>>> but I
>>>>> think it's worth having a closer look as to how they use lpm and 
>>>>> what the
>>>>> impact to them is.
>>>> Checked the projects listed above. BESS, NFF-Go and DPVS don't 
>>>> access the members to be hided.
>>>> They will not be impacted by this patch.
>>>> But Gatekeeper accesses the rte_lpm internal members that to be 
>>>> hided. Its compilation will be broken with this patch.
>>>>
>>>>>
>>>>>> Thanks.
>>>>>> Ruifeng
>>>>>>>>>    /** LPM RCU QSBR configuration structure. */
>>>>>>>>> -- 
>>>>>>>>> 2.17.1
>>>>>>>>>
>>>>>>>
>>>>>>> -- 
>>>>>>> Regards,
>>>>>>> Vladimir
>>>>
>>>
> 

Re: [dpdk-dev] [PATCH 2/2] lpm: hide internal data

[Medvedkin, Vladimir]

On 13/10/2020 18:46, Michel Machado wrote:
> On 10/13/20 11:41 AM, Medvedkin, Vladimir wrote:
>> Hi Michel,
>>
>> Could you please describe a condition when LPM gets inconsistent? As I 
>> can see if there is no free tbl8 it will return -ENOSPC.
> 
>     Consider this simple example, we need to add the following two 
> prefixes with different next hops: 10.99.0.0/16, 18.99.99.128/25. If the 
> LPM table is out of tbl8s, the second prefix is not added and Gatekeeper 
> will make decisions in violation of the policy. The data structure of 
> the LPM table is consistent, but its content inconsistent with the policy.

Aha, thanks. So do I understand correctly that you need to add a set of 
routes atomically (either the entire set is installed or nothing)?

If so, then I would suggest having 2 lpm and switching them atomically 
after a successful addition. As for now, even if you have enough tbl8's, 
routes are installed non atomically, i.e. there will be a time gap 
between adding two routes, so in this time interval the table will be 
inconsistent with the policy.
Also, if new lpm algorithms are added to the DPDK, they won't have such 
a thing as tbl8.

> 
>     We minimize the need of replacing a LPM table by allocating LPM 
> tables with the double of what we need (see example here 
> https://github.com/AltraMayor/gatekeeper/blob/95d1d6e8201861a0d0c698bfd06ad606674f1e07/lua/examples/policy.lua#L172-L183), 
> but the code must be ready for unexpected needs that may arise in 
> production.
> 

Usually, the table is initialized with a large enough number of entries, 
enough to add a possible number of routes. One tbl8 group takes up 1Kb 
of memory which is nothing comparing to the size of tbl24 which is 64Mb.

P.S. consider using rte_fib library, it has a number of advantages over 
LPM. You can replace the loop in __lookup_fib_bulk() with a bulk lookup 
call and this will probably increase the speed.

>>
>> On 13/10/2020 15:58, Michel Machado wrote:
>>> Hi Kevin,
>>>
>>>     We do need fields max_rules and number_tbl8s of struct rte_lpm, 
>>> so the removal would force us to have another patch to our local copy 
>>> of DPDK. We'd rather avoid this new local patch because we wish to 
>>> eventually be in sync with the stock DPDK.
>>>
>>>     Those fields are needed in Gatekeeper because we found a 
>>> condition in an ongoing deployment in which the entries of some LPM 
>>> tables may suddenly change a lot to reflect policy changes. To avoid 
>>> getting into a state in which the LPM table is inconsistent because 
>>> it cannot fit all the new entries, we compute the needed parameters 
>>> to support the new entries, and compare with the current parameters. 
>>> If the current table doesn't fit everything, we have to replace it 
>>> with a new LPM table.
>>>
>>>     If there were a way to obtain the struct rte_lpm_config of a 
>>> given LPM table, it would cleanly address our need. We have the same 
>>> need in IPv6 and have a local patch to work around it (see 
>>> https://github.com/cjdoucette/dpdk/commit/3eaf124a781349b8ec8cd880db26a78115cb8c8f). 
>>> Thus, an IPv4 and IPv6 solution would be best.
>>>
>>>     PS: I've added Qiaobin Fu, another Gatekeeper maintainer, to this 
>>> disscussion.
>>>
>>> [ ]'s
>>> Michel Machado
>>>
>>> On 10/13/20 9:53 AM, Kevin Traynor wrote:
>>>> Hi Gatekeeper maintainers (I think),
>>>>
>>>> fyi - there is a proposal to remove some members of a struct in DPDK 
>>>> LPM
>>>> API that Gatekeeper is using [1]. It would be only from DPDK 20.11 but
>>>> as it's an LTS I guess it would probably hit Debian in a few months.
>>>>
>>>> The full thread is here:
>>>> http://inbox.dpdk.org/dev/20200907081518.46350-1-ruifeng.wang@arm.com/
>>>>
>>>> Maybe you can take a look and tell us if they are needed in Gatekeeper
>>>> or you can workaround it?
>>>>
>>>> thanks,
>>>> Kevin.
>>>>
>>>> [1]
>>>> https://github.com/AltraMayor/gatekeeper/blob/master/gt/lua_lpm.c#L235-L248 
>>>>
>>>>
>>>> On 09/10/2020 07:54, Ruifeng Wang wrote:
>>>>>
>>>>>> -----Original Message-----
>>>>>> From: Kevin Traynor <ktraynor@redhat.com>
>>>>>> Sent: Wednesday, September 30, 2020 4:46 PM
>>>>>> To: Ruifeng Wang <Ruifeng.Wang@arm.com>; Medvedkin, Vladimir
>>>>>> <vladimir.medvedkin@intel.com>; Bruce Richardson
>>>>>> <bruce.richardson@intel.com>
>>>>>> Cc: dev@dpdk.org; Honnappa Nagarahalli
>>>>>> <Honnappa.Nagarahalli@arm.com>; nd <nd@arm.com>
>>>>>> Subject: Re: [dpdk-dev] [PATCH 2/2] lpm: hide internal data
>>>>>>
>>>>>> On 16/09/2020 04:17, Ruifeng Wang wrote:
>>>>>>>
>>>>>>>> -----Original Message-----
>>>>>>>> From: Medvedkin, Vladimir <vladimir.medvedkin@intel.com>
>>>>>>>> Sent: Wednesday, September 16, 2020 12:28 AM
>>>>>>>> To: Bruce Richardson <bruce.richardson@intel.com>; Ruifeng Wang
>>>>>>>> <Ruifeng.Wang@arm.com>
>>>>>>>> Cc: dev@dpdk.org; Honnappa Nagarahalli
>>>>>>>> <Honnappa.Nagarahalli@arm.com>; nd <nd@arm.com>
>>>>>>>> Subject: Re: [PATCH 2/2] lpm: hide internal data
>>>>>>>>
>>>>>>>> Hi Ruifeng,
>>>>>>>>
>>>>>>>> On 15/09/2020 17:02, Bruce Richardson wrote:
>>>>>>>>> On Mon, Sep 07, 2020 at 04:15:17PM +0800, Ruifeng Wang wrote:
>>>>>>>>>> Fields except tbl24 and tbl8 in rte_lpm structure have no need to
>>>>>>>>>> be exposed to the user.
>>>>>>>>>> Hide the unneeded exposure of structure fields for better ABI
>>>>>>>>>> maintainability.
>>>>>>>>>>
>>>>>>>>>> Suggested-by: David Marchand <david.marchand@redhat.com>
>>>>>>>>>> Signed-off-by: Ruifeng Wang <ruifeng.wang@arm.com>
>>>>>>>>>> Reviewed-by: Phil Yang <phil.yang@arm.com>
>>>>>>>>>> ---
>>>>>>>>>>    lib/librte_lpm/rte_lpm.c | 152
>>>>>>>>>> +++++++++++++++++++++++---------------
>>>>>>>> -
>>>>>>>>>>    lib/librte_lpm/rte_lpm.h |   7 --
>>>>>>>>>>    2 files changed, 91 insertions(+), 68 deletions(-)
>>>>>>>>>>
>>>>>>>>> <snip>
>>>>>>>>>> diff --git a/lib/librte_lpm/rte_lpm.h b/lib/librte_lpm/rte_lpm.h
>>>>>>>>>> index 03da2d37e..112d96f37 100644
>>>>>>>>>> --- a/lib/librte_lpm/rte_lpm.h
>>>>>>>>>> +++ b/lib/librte_lpm/rte_lpm.h
>>>>>>>>>> @@ -132,17 +132,10 @@ struct rte_lpm_rule_info {
>>>>>>>>>>
>>>>>>>>>>    /** @internal LPM structure. */
>>>>>>>>>>    struct rte_lpm {
>>>>>>>>>> -    /* LPM metadata. */
>>>>>>>>>> -    char name[RTE_LPM_NAMESIZE];        /**< Name of the lpm. */
>>>>>>>>>> -    uint32_t max_rules; /**< Max. balanced rules per lpm. */
>>>>>>>>>> -    uint32_t number_tbl8s; /**< Number of tbl8s. */
>>>>>>>>>> -    struct rte_lpm_rule_info rule_info[RTE_LPM_MAX_DEPTH]; /**<
>>>>>>>> Rule info table. */
>>>>>>>>>> -
>>>>>>>>>>        /* LPM Tables. */
>>>>>>>>>>        struct rte_lpm_tbl_entry tbl24[RTE_LPM_TBL24_NUM_ENTRIES]
>>>>>>>>>>                __rte_cache_aligned; /**< LPM tbl24 table. */
>>>>>>>>>>        struct rte_lpm_tbl_entry *tbl8; /**< LPM tbl8 table. */
>>>>>>>>>> -    struct rte_lpm_rule *rules_tbl; /**< LPM rules. */
>>>>>>>>>>    };
>>>>>>>>>>
>>>>>>>>>
>>>>>>>>> Since this changes the ABI, does it not need advance notice?
>>>>>>>>>
>>>>>>>>> [Basically the return value point from rte_lpm_create() will be
>>>>>>>>> different, and that return value could be used by rte_lpm_lookup()
>>>>>>>>> which as a static inline function will be in the binary and using
>>>>>>>>> the old structure offsets.]
>>>>>>>>>
>>>>>>>>
>>>>>>>> Agree with Bruce, this patch breaks ABI, so it can't be accepted
>>>>>>>> without prior notice.
>>>>>>>>
>>>>>>> So if the change wants to happen in 20.11, a deprecation notice 
>>>>>>> should
>>>>>>> have been added in 20.08.
>>>>>>> I should have added a deprecation notice. This change will have 
>>>>>>> to wait for
>>>>>> next ABI update window.
>>>>>>>
>>>>>>
>>>>>> Do you plan to extend? or is this just speculative?
>>>>> It is speculative.
>>>>>
>>>>>>
>>>>>> A quick scan and there seems to be several projects using some of 
>>>>>> these
>>>>>> members that you are proposing to hide. e.g. BESS, NFF-Go, DPVS,
>>>>>> gatekeeper. I didn't look at the details to see if they are really 
>>>>>> needed.
>>>>>>
>>>>>> Not sure how much notice they'd need or if they update DPDK much, 
>>>>>> but I
>>>>>> think it's worth having a closer look as to how they use lpm and 
>>>>>> what the
>>>>>> impact to them is.
>>>>> Checked the projects listed above. BESS, NFF-Go and DPVS don't 
>>>>> access the members to be hided.
>>>>> They will not be impacted by this patch.
>>>>> But Gatekeeper accesses the rte_lpm internal members that to be 
>>>>> hided. Its compilation will be broken with this patch.
>>>>>
>>>>>>
>>>>>>> Thanks.
>>>>>>> Ruifeng
>>>>>>>>>>    /** LPM RCU QSBR configuration structure. */
>>>>>>>>>> -- 
>>>>>>>>>> 2.17.1
>>>>>>>>>>
>>>>>>>>
>>>>>>>> -- 
>>>>>>>> Regards,
>>>>>>>> Vladimir
>>>>>
>>>>
>>

-- 
Regards,
Vladimir

Re: [dpdk-dev] [PATCH 2/2] lpm: hide internal data

[Michel Machado]

On 10/13/20 3:06 PM, Medvedkin, Vladimir wrote:
> 
> 
> On 13/10/2020 18:46, Michel Machado wrote:
>> On 10/13/20 11:41 AM, Medvedkin, Vladimir wrote:
>>> Hi Michel,
>>>
>>> Could you please describe a condition when LPM gets inconsistent? As 
>>> I can see if there is no free tbl8 it will return -ENOSPC.
>>
>>     Consider this simple example, we need to add the following two 
>> prefixes with different next hops: 10.99.0.0/16, 18.99.99.128/25. If 
>> the LPM table is out of tbl8s, the second prefix is not added and 
>> Gatekeeper will make decisions in violation of the policy. The data 
>> structure of the LPM table is consistent, but its content inconsistent 
>> with the policy.
> 
> Aha, thanks. So do I understand correctly that you need to add a set of 
> routes atomically (either the entire set is installed or nothing)?

    Yes.

> If so, then I would suggest having 2 lpm and switching them atomically 
> after a successful addition. As for now, even if you have enough tbl8's, 
> routes are installed non atomically, i.e. there will be a time gap 
> between adding two routes, so in this time interval the table will be 
> inconsistent with the policy.
> Also, if new lpm algorithms are added to the DPDK, they won't have such 
> a thing as tbl8.

    Our code already deals with synchronization.

>>     We minimize the need of replacing a LPM table by allocating LPM 
>> tables with the double of what we need (see example here 
>> https://github.com/AltraMayor/gatekeeper/blob/95d1d6e8201861a0d0c698bfd06ad606674f1e07/lua/examples/policy.lua#L172-L183), 
>> but the code must be ready for unexpected needs that may arise in 
>> production.
>>
> 
> Usually, the table is initialized with a large enough number of entries, 
> enough to add a possible number of routes. One tbl8 group takes up 1Kb 
> of memory which is nothing comparing to the size of tbl24 which is 64Mb.

    When the prefixes come from BGP, initializing a large enough table 
is fine. But when prefixes come from threat intelligence, the number of 
prefixes can vary wildly and the number of prefixes above 24 bits are 
way more common.

> P.S. consider using rte_fib library, it has a number of advantages over 
> LPM. You can replace the loop in __lookup_fib_bulk() with a bulk lookup 
> call and this will probably increase the speed.

    I'm not aware of the rte_fib library. The only documentation that I 
found on Google was https://doc.dpdk.org/api/rte__fib_8h.html and it 
just says "FIB (Forwarding information base) implementation for IPv4 
Longest Prefix Match".

>>>
>>> On 13/10/2020 15:58, Michel Machado wrote:
>>>> Hi Kevin,
>>>>
>>>>     We do need fields max_rules and number_tbl8s of struct rte_lpm, 
>>>> so the removal would force us to have another patch to our local 
>>>> copy of DPDK. We'd rather avoid this new local patch because we wish 
>>>> to eventually be in sync with the stock DPDK.
>>>>
>>>>     Those fields are needed in Gatekeeper because we found a 
>>>> condition in an ongoing deployment in which the entries of some LPM 
>>>> tables may suddenly change a lot to reflect policy changes. To avoid 
>>>> getting into a state in which the LPM table is inconsistent because 
>>>> it cannot fit all the new entries, we compute the needed parameters 
>>>> to support the new entries, and compare with the current parameters. 
>>>> If the current table doesn't fit everything, we have to replace it 
>>>> with a new LPM table.
>>>>
>>>>     If there were a way to obtain the struct rte_lpm_config of a 
>>>> given LPM table, it would cleanly address our need. We have the same 
>>>> need in IPv6 and have a local patch to work around it (see 
>>>> https://github.com/cjdoucette/dpdk/commit/3eaf124a781349b8ec8cd880db26a78115cb8c8f). 
>>>> Thus, an IPv4 and IPv6 solution would be best.
>>>>
>>>>     PS: I've added Qiaobin Fu, another Gatekeeper maintainer, to 
>>>> this disscussion.
>>>>
>>>> [ ]'s
>>>> Michel Machado
>>>>
>>>> On 10/13/20 9:53 AM, Kevin Traynor wrote:
>>>>> Hi Gatekeeper maintainers (I think),
>>>>>
>>>>> fyi - there is a proposal to remove some members of a struct in 
>>>>> DPDK LPM
>>>>> API that Gatekeeper is using [1]. It would be only from DPDK 20.11 but
>>>>> as it's an LTS I guess it would probably hit Debian in a few months.
>>>>>
>>>>> The full thread is here:
>>>>> http://inbox.dpdk.org/dev/20200907081518.46350-1-ruifeng.wang@arm.com/
>>>>>
>>>>> Maybe you can take a look and tell us if they are needed in Gatekeeper
>>>>> or you can workaround it?
>>>>>
>>>>> thanks,
>>>>> Kevin.
>>>>>
>>>>> [1]
>>>>> https://github.com/AltraMayor/gatekeeper/blob/master/gt/lua_lpm.c#L235-L248 
>>>>>
>>>>>
>>>>> On 09/10/2020 07:54, Ruifeng Wang wrote:
>>>>>>
>>>>>>> -----Original Message-----
>>>>>>> From: Kevin Traynor <ktraynor@redhat.com>
>>>>>>> Sent: Wednesday, September 30, 2020 4:46 PM
>>>>>>> To: Ruifeng Wang <Ruifeng.Wang@arm.com>; Medvedkin, Vladimir
>>>>>>> <vladimir.medvedkin@intel.com>; Bruce Richardson
>>>>>>> <bruce.richardson@intel.com>
>>>>>>> Cc: dev@dpdk.org; Honnappa Nagarahalli
>>>>>>> <Honnappa.Nagarahalli@arm.com>; nd <nd@arm.com>
>>>>>>> Subject: Re: [dpdk-dev] [PATCH 2/2] lpm: hide internal data
>>>>>>>
>>>>>>> On 16/09/2020 04:17, Ruifeng Wang wrote:
>>>>>>>>
>>>>>>>>> -----Original Message-----
>>>>>>>>> From: Medvedkin, Vladimir <vladimir.medvedkin@intel.com>
>>>>>>>>> Sent: Wednesday, September 16, 2020 12:28 AM
>>>>>>>>> To: Bruce Richardson <bruce.richardson@intel.com>; Ruifeng Wang
>>>>>>>>> <Ruifeng.Wang@arm.com>
>>>>>>>>> Cc: dev@dpdk.org; Honnappa Nagarahalli
>>>>>>>>> <Honnappa.Nagarahalli@arm.com>; nd <nd@arm.com>
>>>>>>>>> Subject: Re: [PATCH 2/2] lpm: hide internal data
>>>>>>>>>
>>>>>>>>> Hi Ruifeng,
>>>>>>>>>
>>>>>>>>> On 15/09/2020 17:02, Bruce Richardson wrote:
>>>>>>>>>> On Mon, Sep 07, 2020 at 04:15:17PM +0800, Ruifeng Wang wrote:
>>>>>>>>>>> Fields except tbl24 and tbl8 in rte_lpm structure have no 
>>>>>>>>>>> need to
>>>>>>>>>>> be exposed to the user.
>>>>>>>>>>> Hide the unneeded exposure of structure fields for better ABI
>>>>>>>>>>> maintainability.
>>>>>>>>>>>
>>>>>>>>>>> Suggested-by: David Marchand <david.marchand@redhat.com>
>>>>>>>>>>> Signed-off-by: Ruifeng Wang <ruifeng.wang@arm.com>
>>>>>>>>>>> Reviewed-by: Phil Yang <phil.yang@arm.com>
>>>>>>>>>>> ---
>>>>>>>>>>>    lib/librte_lpm/rte_lpm.c | 152
>>>>>>>>>>> +++++++++++++++++++++++---------------
>>>>>>>>> -
>>>>>>>>>>>    lib/librte_lpm/rte_lpm.h |   7 --
>>>>>>>>>>>    2 files changed, 91 insertions(+), 68 deletions(-)
>>>>>>>>>>>
>>>>>>>>>> <snip>
>>>>>>>>>>> diff --git a/lib/librte_lpm/rte_lpm.h b/lib/librte_lpm/rte_lpm.h
>>>>>>>>>>> index 03da2d37e..112d96f37 100644
>>>>>>>>>>> --- a/lib/librte_lpm/rte_lpm.h
>>>>>>>>>>> +++ b/lib/librte_lpm/rte_lpm.h
>>>>>>>>>>> @@ -132,17 +132,10 @@ struct rte_lpm_rule_info {
>>>>>>>>>>>
>>>>>>>>>>>    /** @internal LPM structure. */
>>>>>>>>>>>    struct rte_lpm {
>>>>>>>>>>> -    /* LPM metadata. */
>>>>>>>>>>> -    char name[RTE_LPM_NAMESIZE];        /**< Name of the 
>>>>>>>>>>> lpm. */
>>>>>>>>>>> -    uint32_t max_rules; /**< Max. balanced rules per lpm. */
>>>>>>>>>>> -    uint32_t number_tbl8s; /**< Number of tbl8s. */
>>>>>>>>>>> -    struct rte_lpm_rule_info rule_info[RTE_LPM_MAX_DEPTH]; /**<
>>>>>>>>> Rule info table. */
>>>>>>>>>>> -
>>>>>>>>>>>        /* LPM Tables. */
>>>>>>>>>>>        struct rte_lpm_tbl_entry tbl24[RTE_LPM_TBL24_NUM_ENTRIES]
>>>>>>>>>>>                __rte_cache_aligned; /**< LPM tbl24 table. */
>>>>>>>>>>>        struct rte_lpm_tbl_entry *tbl8; /**< LPM tbl8 table. */
>>>>>>>>>>> -    struct rte_lpm_rule *rules_tbl; /**< LPM rules. */
>>>>>>>>>>>    };
>>>>>>>>>>>
>>>>>>>>>>
>>>>>>>>>> Since this changes the ABI, does it not need advance notice?
>>>>>>>>>>
>>>>>>>>>> [Basically the return value point from rte_lpm_create() will be
>>>>>>>>>> different, and that return value could be used by 
>>>>>>>>>> rte_lpm_lookup()
>>>>>>>>>> which as a static inline function will be in the binary and using
>>>>>>>>>> the old structure offsets.]
>>>>>>>>>>
>>>>>>>>>
>>>>>>>>> Agree with Bruce, this patch breaks ABI, so it can't be accepted
>>>>>>>>> without prior notice.
>>>>>>>>>
>>>>>>>> So if the change wants to happen in 20.11, a deprecation notice 
>>>>>>>> should
>>>>>>>> have been added in 20.08.
>>>>>>>> I should have added a deprecation notice. This change will have 
>>>>>>>> to wait for
>>>>>>> next ABI update window.
>>>>>>>>
>>>>>>>
>>>>>>> Do you plan to extend? or is this just speculative?
>>>>>> It is speculative.
>>>>>>
>>>>>>>
>>>>>>> A quick scan and there seems to be several projects using some of 
>>>>>>> these
>>>>>>> members that you are proposing to hide. e.g. BESS, NFF-Go, DPVS,
>>>>>>> gatekeeper. I didn't look at the details to see if they are 
>>>>>>> really needed.
>>>>>>>
>>>>>>> Not sure how much notice they'd need or if they update DPDK much, 
>>>>>>> but I
>>>>>>> think it's worth having a closer look as to how they use lpm and 
>>>>>>> what the
>>>>>>> impact to them is.
>>>>>> Checked the projects listed above. BESS, NFF-Go and DPVS don't 
>>>>>> access the members to be hided.
>>>>>> They will not be impacted by this patch.
>>>>>> But Gatekeeper accesses the rte_lpm internal members that to be 
>>>>>> hided. Its compilation will be broken with this patch.
>>>>>>
>>>>>>>
>>>>>>>> Thanks.
>>>>>>>> Ruifeng
>>>>>>>>>>>    /** LPM RCU QSBR configuration structure. */
>>>>>>>>>>> -- 
>>>>>>>>>>> 2.17.1
>>>>>>>>>>>
>>>>>>>>>
>>>>>>>>> -- 
>>>>>>>>> Regards,
>>>>>>>>> Vladimir
>>>>>>
>>>>>
>>>
> 

Re: [dpdk-dev] [PATCH 2/2] lpm: hide internal data

[Medvedkin, Vladimir]

On 13/10/2020 20:48, Michel Machado wrote:
> On 10/13/20 3:06 PM, Medvedkin, Vladimir wrote:
>>
>>
>> On 13/10/2020 18:46, Michel Machado wrote:
>>> On 10/13/20 11:41 AM, Medvedkin, Vladimir wrote:
>>>> Hi Michel,
>>>>
>>>> Could you please describe a condition when LPM gets inconsistent? As 
>>>> I can see if there is no free tbl8 it will return -ENOSPC.
>>>
>>>     Consider this simple example, we need to add the following two 
>>> prefixes with different next hops: 10.99.0.0/16, 18.99.99.128/25. If 
>>> the LPM table is out of tbl8s, the second prefix is not added and 
>>> Gatekeeper will make decisions in violation of the policy. The data 
>>> structure of the LPM table is consistent, but its content 
>>> inconsistent with the policy.
>>
>> Aha, thanks. So do I understand correctly that you need to add a set 
>> of routes atomically (either the entire set is installed or nothing)?
> 
>     Yes.
> 
>> If so, then I would suggest having 2 lpm and switching them atomically 
>> after a successful addition. As for now, even if you have enough 
>> tbl8's, routes are installed non atomically, i.e. there will be a time 
>> gap between adding two routes, so in this time interval the table will 
>> be inconsistent with the policy.
>> Also, if new lpm algorithms are added to the DPDK, they won't have 
>> such a thing as tbl8.
> 
>     Our code already deals with synchronization.

OK, so my suggestion here would be to add new routes to the shadow copy 
of the lpm, and if it returns -ENOSPC, than create a new LPM with double 
amount of tbl8's and add all the routes to it. Then switch the 
active-shadow LPM pointers. In this case you'll always add a bulk of 
routes atomically.

> 
>>>     We minimize the need of replacing a LPM table by allocating LPM 
>>> tables with the double of what we need (see example here 
>>> https://github.com/AltraMayor/gatekeeper/blob/95d1d6e8201861a0d0c698bfd06ad606674f1e07/lua/examples/policy.lua#L172-L183), 
>>> but the code must be ready for unexpected needs that may arise in 
>>> production.
>>>
>>
>> Usually, the table is initialized with a large enough number of 
>> entries, enough to add a possible number of routes. One tbl8 group 
>> takes up 1Kb of memory which is nothing comparing to the size of tbl24 
>> which is 64Mb.
> 
>     When the prefixes come from BGP, initializing a large enough table 
> is fine. But when prefixes come from threat intelligence, the number of 
> prefixes can vary wildly and the number of prefixes above 24 bits are 
> way more common.
> 
>> P.S. consider using rte_fib library, it has a number of advantages 
>> over LPM. You can replace the loop in __lookup_fib_bulk() with a bulk 
>> lookup call and this will probably increase the speed.
> 
>     I'm not aware of the rte_fib library. The only documentation that I 
> found on Google was https://doc.dpdk.org/api/rte__fib_8h.html and it 
> just says "FIB (Forwarding information base) implementation for IPv4 
> Longest Prefix Match".

That's true, I'm going to add programmer's guide soon.
Although the fib API is very similar to existing LPM.

> 
>>>>
>>>> On 13/10/2020 15:58, Michel Machado wrote:
>>>>> Hi Kevin,
>>>>>
>>>>>     We do need fields max_rules and number_tbl8s of struct rte_lpm, 
>>>>> so the removal would force us to have another patch to our local 
>>>>> copy of DPDK. We'd rather avoid this new local patch because we 
>>>>> wish to eventually be in sync with the stock DPDK.
>>>>>
>>>>>     Those fields are needed in Gatekeeper because we found a 
>>>>> condition in an ongoing deployment in which the entries of some LPM 
>>>>> tables may suddenly change a lot to reflect policy changes. To 
>>>>> avoid getting into a state in which the LPM table is inconsistent 
>>>>> because it cannot fit all the new entries, we compute the needed 
>>>>> parameters to support the new entries, and compare with the current 
>>>>> parameters. If the current table doesn't fit everything, we have to 
>>>>> replace it with a new LPM table.
>>>>>
>>>>>     If there were a way to obtain the struct rte_lpm_config of a 
>>>>> given LPM table, it would cleanly address our need. We have the 
>>>>> same need in IPv6 and have a local patch to work around it (see 
>>>>> https://github.com/cjdoucette/dpdk/commit/3eaf124a781349b8ec8cd880db26a78115cb8c8f). 
>>>>> Thus, an IPv4 and IPv6 solution would be best.
>>>>>
>>>>>     PS: I've added Qiaobin Fu, another Gatekeeper maintainer, to 
>>>>> this disscussion.
>>>>>
>>>>> [ ]'s
>>>>> Michel Machado
>>>>>
>>>>> On 10/13/20 9:53 AM, Kevin Traynor wrote:
>>>>>> Hi Gatekeeper maintainers (I think),
>>>>>>
>>>>>> fyi - there is a proposal to remove some members of a struct in 
>>>>>> DPDK LPM
>>>>>> API that Gatekeeper is using [1]. It would be only from DPDK 20.11 
>>>>>> but
>>>>>> as it's an LTS I guess it would probably hit Debian in a few months.
>>>>>>
>>>>>> The full thread is here:
>>>>>> http://inbox.dpdk.org/dev/20200907081518.46350-1-ruifeng.wang@arm.com/ 
>>>>>>
>>>>>>
>>>>>> Maybe you can take a look and tell us if they are needed in 
>>>>>> Gatekeeper
>>>>>> or you can workaround it?
>>>>>>
>>>>>> thanks,
>>>>>> Kevin.
>>>>>>
>>>>>> [1]
>>>>>> https://github.com/AltraMayor/gatekeeper/blob/master/gt/lua_lpm.c#L235-L248 
>>>>>>
>>>>>>
>>>>>> On 09/10/2020 07:54, Ruifeng Wang wrote:
>>>>>>>
>>>>>>>> -----Original Message-----
>>>>>>>> From: Kevin Traynor <ktraynor@redhat.com>
>>>>>>>> Sent: Wednesday, September 30, 2020 4:46 PM
>>>>>>>> To: Ruifeng Wang <Ruifeng.Wang@arm.com>; Medvedkin, Vladimir
>>>>>>>> <vladimir.medvedkin@intel.com>; Bruce Richardson
>>>>>>>> <bruce.richardson@intel.com>
>>>>>>>> Cc: dev@dpdk.org; Honnappa Nagarahalli
>>>>>>>> <Honnappa.Nagarahalli@arm.com>; nd <nd@arm.com>
>>>>>>>> Subject: Re: [dpdk-dev] [PATCH 2/2] lpm: hide internal data
>>>>>>>>
>>>>>>>> On 16/09/2020 04:17, Ruifeng Wang wrote:
>>>>>>>>>
>>>>>>>>>> -----Original Message-----
>>>>>>>>>> From: Medvedkin, Vladimir <vladimir.medvedkin@intel.com>
>>>>>>>>>> Sent: Wednesday, September 16, 2020 12:28 AM
>>>>>>>>>> To: Bruce Richardson <bruce.richardson@intel.com>; Ruifeng Wang
>>>>>>>>>> <Ruifeng.Wang@arm.com>
>>>>>>>>>> Cc: dev@dpdk.org; Honnappa Nagarahalli
>>>>>>>>>> <Honnappa.Nagarahalli@arm.com>; nd <nd@arm.com>
>>>>>>>>>> Subject: Re: [PATCH 2/2] lpm: hide internal data
>>>>>>>>>>
>>>>>>>>>> Hi Ruifeng,
>>>>>>>>>>
>>>>>>>>>> On 15/09/2020 17:02, Bruce Richardson wrote:
>>>>>>>>>>> On Mon, Sep 07, 2020 at 04:15:17PM +0800, Ruifeng Wang wrote:
>>>>>>>>>>>> Fields except tbl24 and tbl8 in rte_lpm structure have no 
>>>>>>>>>>>> need to
>>>>>>>>>>>> be exposed to the user.
>>>>>>>>>>>> Hide the unneeded exposure of structure fields for better ABI
>>>>>>>>>>>> maintainability.
>>>>>>>>>>>>
>>>>>>>>>>>> Suggested-by: David Marchand <david.marchand@redhat.com>
>>>>>>>>>>>> Signed-off-by: Ruifeng Wang <ruifeng.wang@arm.com>
>>>>>>>>>>>> Reviewed-by: Phil Yang <phil.yang@arm.com>
>>>>>>>>>>>> ---
>>>>>>>>>>>>    lib/librte_lpm/rte_lpm.c | 152
>>>>>>>>>>>> +++++++++++++++++++++++---------------
>>>>>>>>>> -
>>>>>>>>>>>>    lib/librte_lpm/rte_lpm.h |   7 --
>>>>>>>>>>>>    2 files changed, 91 insertions(+), 68 deletions(-)
>>>>>>>>>>>>
>>>>>>>>>>> <snip>
>>>>>>>>>>>> diff --git a/lib/librte_lpm/rte_lpm.h 
>>>>>>>>>>>> b/lib/librte_lpm/rte_lpm.h
>>>>>>>>>>>> index 03da2d37e..112d96f37 100644
>>>>>>>>>>>> --- a/lib/librte_lpm/rte_lpm.h
>>>>>>>>>>>> +++ b/lib/librte_lpm/rte_lpm.h
>>>>>>>>>>>> @@ -132,17 +132,10 @@ struct rte_lpm_rule_info {
>>>>>>>>>>>>
>>>>>>>>>>>>    /** @internal LPM structure. */
>>>>>>>>>>>>    struct rte_lpm {
>>>>>>>>>>>> -    /* LPM metadata. */
>>>>>>>>>>>> -    char name[RTE_LPM_NAMESIZE];        /**< Name of the 
>>>>>>>>>>>> lpm. */
>>>>>>>>>>>> -    uint32_t max_rules; /**< Max. balanced rules per lpm. */
>>>>>>>>>>>> -    uint32_t number_tbl8s; /**< Number of tbl8s. */
>>>>>>>>>>>> -    struct rte_lpm_rule_info rule_info[RTE_LPM_MAX_DEPTH]; 
>>>>>>>>>>>> /**<
>>>>>>>>>> Rule info table. */
>>>>>>>>>>>> -
>>>>>>>>>>>>        /* LPM Tables. */
>>>>>>>>>>>>        struct rte_lpm_tbl_entry 
>>>>>>>>>>>> tbl24[RTE_LPM_TBL24_NUM_ENTRIES]
>>>>>>>>>>>>                __rte_cache_aligned; /**< LPM tbl24 table. */
>>>>>>>>>>>>        struct rte_lpm_tbl_entry *tbl8; /**< LPM tbl8 table. */
>>>>>>>>>>>> -    struct rte_lpm_rule *rules_tbl; /**< LPM rules. */
>>>>>>>>>>>>    };
>>>>>>>>>>>>
>>>>>>>>>>>
>>>>>>>>>>> Since this changes the ABI, does it not need advance notice?
>>>>>>>>>>>
>>>>>>>>>>> [Basically the return value point from rte_lpm_create() will be
>>>>>>>>>>> different, and that return value could be used by 
>>>>>>>>>>> rte_lpm_lookup()
>>>>>>>>>>> which as a static inline function will be in the binary and 
>>>>>>>>>>> using
>>>>>>>>>>> the old structure offsets.]
>>>>>>>>>>>
>>>>>>>>>>
>>>>>>>>>> Agree with Bruce, this patch breaks ABI, so it can't be accepted
>>>>>>>>>> without prior notice.
>>>>>>>>>>
>>>>>>>>> So if the change wants to happen in 20.11, a deprecation notice 
>>>>>>>>> should
>>>>>>>>> have been added in 20.08.
>>>>>>>>> I should have added a deprecation notice. This change will have 
>>>>>>>>> to wait for
>>>>>>>> next ABI update window.
>>>>>>>>>
>>>>>>>>
>>>>>>>> Do you plan to extend? or is this just speculative?
>>>>>>> It is speculative.
>>>>>>>
>>>>>>>>
>>>>>>>> A quick scan and there seems to be several projects using some 
>>>>>>>> of these
>>>>>>>> members that you are proposing to hide. e.g. BESS, NFF-Go, DPVS,
>>>>>>>> gatekeeper. I didn't look at the details to see if they are 
>>>>>>>> really needed.
>>>>>>>>
>>>>>>>> Not sure how much notice they'd need or if they update DPDK 
>>>>>>>> much, but I
>>>>>>>> think it's worth having a closer look as to how they use lpm and 
>>>>>>>> what the
>>>>>>>> impact to them is.
>>>>>>> Checked the projects listed above. BESS, NFF-Go and DPVS don't 
>>>>>>> access the members to be hided.
>>>>>>> They will not be impacted by this patch.
>>>>>>> But Gatekeeper accesses the rte_lpm internal members that to be 
>>>>>>> hided. Its compilation will be broken with this patch.
>>>>>>>
>>>>>>>>
>>>>>>>>> Thanks.
>>>>>>>>> Ruifeng
>>>>>>>>>>>>    /** LPM RCU QSBR configuration structure. */
>>>>>>>>>>>> -- 
>>>>>>>>>>>> 2.17.1
>>>>>>>>>>>>
>>>>>>>>>>
>>>>>>>>>> -- 
>>>>>>>>>> Regards,
>>>>>>>>>> Vladimir
>>>>>>>
>>>>>>
>>>>
>>

-- 
Regards,
Vladimir

Re: [dpdk-dev] [PATCH 2/2] lpm: hide internal data

[Honnappa Nagarahalli]

<snip>

> >>
> >>
> >> On 13/10/2020 18:46, Michel Machado wrote:
> >>> On 10/13/20 11:41 AM, Medvedkin, Vladimir wrote:
> >>>> Hi Michel,
> >>>>
> >>>> Could you please describe a condition when LPM gets inconsistent?
> >>>> As I can see if there is no free tbl8 it will return -ENOSPC.
> >>>
> >>>     Consider this simple example, we need to add the following two
> >>> prefixes with different next hops: 10.99.0.0/16, 18.99.99.128/25. If
> >>> the LPM table is out of tbl8s, the second prefix is not added and
> >>> Gatekeeper will make decisions in violation of the policy. The data
> >>> structure of the LPM table is consistent, but its content
> >>> inconsistent with the policy.
max_rules and number_tbl8s in 'struct rte_lpm' contain the config information. These 2 fields do not change based on the routes added and do not indicate the amount of space left. So, you cannot use this information to decide if there is enough space to add more routes.

> >>
> >> Aha, thanks. So do I understand correctly that you need to add a set
> >> of routes atomically (either the entire set is installed or nothing)?
> >
> >     Yes.
> >
> >> If so, then I would suggest having 2 lpm and switching them
> >> atomically after a successful addition. As for now, even if you have
> >> enough tbl8's, routes are installed non atomically, i.e. there will
> >> be a time gap between adding two routes, so in this time interval the
> >> table will be inconsistent with the policy.
> >> Also, if new lpm algorithms are added to the DPDK, they won't have
> >> such a thing as tbl8.
> >
> >     Our code already deals with synchronization.
If the application code already deals with synchronization, is it possible to revert back (i.e. delete the routes that got added so far) when the addition of the route-set fails?

> 
> OK, so my suggestion here would be to add new routes to the shadow copy
> of the lpm, and if it returns -ENOSPC, than create a new LPM with double
> amount of tbl8's and add all the routes to it. Then switch the active-shadow
> LPM pointers. In this case you'll always add a bulk of routes atomically.
> 
> >
> >>>     We minimize the need of replacing a LPM table by allocating LPM
> >>> tables with the double of what we need (see example here
> >>>
> https://github.com/AltraMayor/gatekeeper/blob/95d1d6e8201861a0d0c698
> >>> bfd06ad606674f1e07/lua/examples/policy.lua#L172-L183),
> >>> but the code must be ready for unexpected needs that may arise in
> >>> production.
> >>>
> >>
> >> Usually, the table is initialized with a large enough number of
> >> entries, enough to add a possible number of routes. One tbl8 group
> >> takes up 1Kb of memory which is nothing comparing to the size of
> >> tbl24 which is 64Mb.
> >
> >     When the prefixes come from BGP, initializing a large enough table
> > is fine. But when prefixes come from threat intelligence, the number
> > of prefixes can vary wildly and the number of prefixes above 24 bits
> > are way more common.
> >
> >> P.S. consider using rte_fib library, it has a number of advantages
> >> over LPM. You can replace the loop in __lookup_fib_bulk() with a bulk
> >> lookup call and this will probably increase the speed.
> >
> >     I'm not aware of the rte_fib library. The only documentation that
> > I found on Google was https://doc.dpdk.org/api/rte__fib_8h.html and it
> > just says "FIB (Forwarding information base) implementation for IPv4
> > Longest Prefix Match".
> 
> That's true, I'm going to add programmer's guide soon.
> Although the fib API is very similar to existing LPM.
> 
> >
> >>>>
> >>>> On 13/10/2020 15:58, Michel Machado wrote:
> >>>>> Hi Kevin,
> >>>>>
> >>>>>     We do need fields max_rules and number_tbl8s of struct
> >>>>> rte_lpm, so the removal would force us to have another patch to
> >>>>> our local copy of DPDK. We'd rather avoid this new local patch
> >>>>> because we wish to eventually be in sync with the stock DPDK.
> >>>>>
> >>>>>     Those fields are needed in Gatekeeper because we found a
> >>>>> condition in an ongoing deployment in which the entries of some
> >>>>> LPM tables may suddenly change a lot to reflect policy changes. To
> >>>>> avoid getting into a state in which the LPM table is inconsistent
> >>>>> because it cannot fit all the new entries, we compute the needed
> >>>>> parameters to support the new entries, and compare with the
> >>>>> current parameters. If the current table doesn't fit everything,
> >>>>> we have to replace it with a new LPM table.
> >>>>>
> >>>>>     If there were a way to obtain the struct rte_lpm_config of a
> >>>>> given LPM table, it would cleanly address our need. We have the
> >>>>> same need in IPv6 and have a local patch to work around it (see
> >>>>>
> https://github.com/cjdoucette/dpdk/commit/3eaf124a781349b8ec8cd880db
> 26a78115cb8c8f).
I do not see why such an API is not possible, we could add one API that returns max_rules and number_tbl8s (essentially, the config that was passed to rte_lpm_create API).
But, is there a possibility to store that info in the application as that data was passed to rte_lpm from the application?

> >>>>> Thus, an IPv4 and IPv6 solution would be best.
> >>>>>
> >>>>>     PS: I've added Qiaobin Fu, another Gatekeeper maintainer, to
> >>>>> this disscussion.
> >>>>>
> >>>>> [ ]'s
> >>>>> Michel Machado
> >>>>>
> >>>>> On 10/13/20 9:53 AM, Kevin Traynor wrote:
> >>>>>> Hi Gatekeeper maintainers (I think),
> >>>>>>
> >>>>>> fyi - there is a proposal to remove some members of a struct in
> >>>>>> DPDK LPM API that Gatekeeper is using [1]. It would be only from
> >>>>>> DPDK 20.11 but as it's an LTS I guess it would probably hit
> >>>>>> Debian in a few months.
> >>>>>>
> >>>>>> The full thread is here:
> >>>>>> http://inbox.dpdk.org/dev/20200907081518.46350-1-
> ruifeng.wang@arm
> >>>>>> .com/
> >>>>>>
> >>>>>>
> >>>>>> Maybe you can take a look and tell us if they are needed in
> >>>>>> Gatekeeper or you can workaround it?
> >>>>>>
> >>>>>> thanks,
> >>>>>> Kevin.
> >>>>>>
> >>>>>> [1]
> >>>>>>
> https://github.com/AltraMayor/gatekeeper/blob/master/gt/lua_lpm.c
> >>>>>> #L235-L248
> >>>>>>
> >>>>>>
> >>>>>> On 09/10/2020 07:54, Ruifeng Wang wrote:
> >>>>>>>
> >>>>>>>> -----Original Message-----
> >>>>>>>> From: Kevin Traynor <ktraynor@redhat.com>
> >>>>>>>> Sent: Wednesday, September 30, 2020 4:46 PM
> >>>>>>>> To: Ruifeng Wang <Ruifeng.Wang@arm.com>; Medvedkin,
> Vladimir
> >>>>>>>> <vladimir.medvedkin@intel.com>; Bruce Richardson
> >>>>>>>> <bruce.richardson@intel.com>
> >>>>>>>> Cc: dev@dpdk.org; Honnappa Nagarahalli
> >>>>>>>> <Honnappa.Nagarahalli@arm.com>; nd <nd@arm.com>
> >>>>>>>> Subject: Re: [dpdk-dev] [PATCH 2/2] lpm: hide internal data
> >>>>>>>>
> >>>>>>>> On 16/09/2020 04:17, Ruifeng Wang wrote:
> >>>>>>>>>
> >>>>>>>>>> -----Original Message-----
> >>>>>>>>>> From: Medvedkin, Vladimir <vladimir.medvedkin@intel.com>
> >>>>>>>>>> Sent: Wednesday, September 16, 2020 12:28 AM
> >>>>>>>>>> To: Bruce Richardson <bruce.richardson@intel.com>; Ruifeng
> >>>>>>>>>> Wang <Ruifeng.Wang@arm.com>
> >>>>>>>>>> Cc: dev@dpdk.org; Honnappa Nagarahalli
> >>>>>>>>>> <Honnappa.Nagarahalli@arm.com>; nd <nd@arm.com>
> >>>>>>>>>> Subject: Re: [PATCH 2/2] lpm: hide internal data
> >>>>>>>>>>
> >>>>>>>>>> Hi Ruifeng,
> >>>>>>>>>>
> >>>>>>>>>> On 15/09/2020 17:02, Bruce Richardson wrote:
> >>>>>>>>>>> On Mon, Sep 07, 2020 at 04:15:17PM +0800, Ruifeng Wang
> wrote:
> >>>>>>>>>>>> Fields except tbl24 and tbl8 in rte_lpm structure have no
> >>>>>>>>>>>> need to be exposed to the user.
> >>>>>>>>>>>> Hide the unneeded exposure of structure fields for better
> >>>>>>>>>>>> ABI maintainability.
> >>>>>>>>>>>>
> >>>>>>>>>>>> Suggested-by: David Marchand
> <david.marchand@redhat.com>
> >>>>>>>>>>>> Signed-off-by: Ruifeng Wang <ruifeng.wang@arm.com>
> >>>>>>>>>>>> Reviewed-by: Phil Yang <phil.yang@arm.com>
> >>>>>>>>>>>> ---
> >>>>>>>>>>>>    lib/librte_lpm/rte_lpm.c | 152
> >>>>>>>>>>>> +++++++++++++++++++++++---------------
> >>>>>>>>>> -
> >>>>>>>>>>>>    lib/librte_lpm/rte_lpm.h |   7 --
> >>>>>>>>>>>>    2 files changed, 91 insertions(+), 68 deletions(-)
> >>>>>>>>>>>>
> >>>>>>>>>>> <snip>
> >>>>>>>>>>>> diff --git a/lib/librte_lpm/rte_lpm.h
> >>>>>>>>>>>> b/lib/librte_lpm/rte_lpm.h index 03da2d37e..112d96f37
> >>>>>>>>>>>> 100644
> >>>>>>>>>>>> --- a/lib/librte_lpm/rte_lpm.h
> >>>>>>>>>>>> +++ b/lib/librte_lpm/rte_lpm.h
> >>>>>>>>>>>> @@ -132,17 +132,10 @@ struct rte_lpm_rule_info {
> >>>>>>>>>>>>
> >>>>>>>>>>>>    /** @internal LPM structure. */
> >>>>>>>>>>>>    struct rte_lpm {
> >>>>>>>>>>>> -    /* LPM metadata. */
> >>>>>>>>>>>> -    char name[RTE_LPM_NAMESIZE];        /**< Name of the
> >>>>>>>>>>>> lpm. */
> >>>>>>>>>>>> -    uint32_t max_rules; /**< Max. balanced rules per lpm.
> >>>>>>>>>>>> */
> >>>>>>>>>>>> -    uint32_t number_tbl8s; /**< Number of tbl8s. */
> >>>>>>>>>>>> -    struct rte_lpm_rule_info
> rule_info[RTE_LPM_MAX_DEPTH];
> >>>>>>>>>>>> /**<
> >>>>>>>>>> Rule info table. */
> >>>>>>>>>>>> -
> >>>>>>>>>>>>        /* LPM Tables. */
> >>>>>>>>>>>>        struct rte_lpm_tbl_entry
> >>>>>>>>>>>> tbl24[RTE_LPM_TBL24_NUM_ENTRIES]
> >>>>>>>>>>>>                __rte_cache_aligned; /**< LPM tbl24 table.
> >>>>>>>>>>>> */
> >>>>>>>>>>>>        struct rte_lpm_tbl_entry *tbl8; /**< LPM tbl8 table.
> >>>>>>>>>>>> */
> >>>>>>>>>>>> -    struct rte_lpm_rule *rules_tbl; /**< LPM rules. */
> >>>>>>>>>>>>    };
> >>>>>>>>>>>>
> >>>>>>>>>>>
> >>>>>>>>>>> Since this changes the ABI, does it not need advance notice?
> >>>>>>>>>>>
> >>>>>>>>>>> [Basically the return value point from rte_lpm_create() will
> >>>>>>>>>>> be different, and that return value could be used by
> >>>>>>>>>>> rte_lpm_lookup()
> >>>>>>>>>>> which as a static inline function will be in the binary and
> >>>>>>>>>>> using the old structure offsets.]
> >>>>>>>>>>>
> >>>>>>>>>>
> >>>>>>>>>> Agree with Bruce, this patch breaks ABI, so it can't be
> >>>>>>>>>> accepted without prior notice.
> >>>>>>>>>>
> >>>>>>>>> So if the change wants to happen in 20.11, a deprecation
> >>>>>>>>> notice should have been added in 20.08.
> >>>>>>>>> I should have added a deprecation notice. This change will
> >>>>>>>>> have to wait for
> >>>>>>>> next ABI update window.
> >>>>>>>>>
> >>>>>>>>
> >>>>>>>> Do you plan to extend? or is this just speculative?
> >>>>>>> It is speculative.
> >>>>>>>
> >>>>>>>>
> >>>>>>>> A quick scan and there seems to be several projects using some
> >>>>>>>> of these
> >>>>>>>> members that you are proposing to hide. e.g. BESS, NFF-Go, DPVS,
> >>>>>>>> gatekeeper. I didn't look at the details to see if they are
> >>>>>>>> really needed.
> >>>>>>>>
> >>>>>>>> Not sure how much notice they'd need or if they update DPDK
> >>>>>>>> much, but I
> >>>>>>>> think it's worth having a closer look as to how they use lpm and
> >>>>>>>> what the
> >>>>>>>> impact to them is.
> >>>>>>> Checked the projects listed above. BESS, NFF-Go and DPVS don't
> >>>>>>> access the members to be hided.
> >>>>>>> They will not be impacted by this patch.
> >>>>>>> But Gatekeeper accesses the rte_lpm internal members that to be
> >>>>>>> hided. Its compilation will be broken with this patch.
> >>>>>>>
> >>>>>>>>
> >>>>>>>>> Thanks.
> >>>>>>>>> Ruifeng
> >>>>>>>>>>>>    /** LPM RCU QSBR configuration structure. */
> >>>>>>>>>>>> --
> >>>>>>>>>>>> 2.17.1
> >>>>>>>>>>>>
> >>>>>>>>>>
> >>>>>>>>>> --
> >>>>>>>>>> Regards,
> >>>>>>>>>> Vladimir
> >>>>>>>
> >>>>>>
> >>>>
> >>
> 
> --
> Regards,
> Vladimir

Re: [dpdk-dev] [PATCH 2/2] lpm: hide internal data

[Michel Machado]

On 10/14/20 7:57 PM, Honnappa Nagarahalli wrote:
>>>> On 13/10/2020 18:46, Michel Machado wrote:
>>>>> On 10/13/20 11:41 AM, Medvedkin, Vladimir wrote:
>>>>>> Hi Michel,
>>>>>>
>>>>>> Could you please describe a condition when LPM gets inconsistent?
>>>>>> As I can see if there is no free tbl8 it will return -ENOSPC.
>>>>>
>>>>>      Consider this simple example, we need to add the following two
>>>>> prefixes with different next hops: 10.99.0.0/16, 18.99.99.128/25. If
>>>>> the LPM table is out of tbl8s, the second prefix is not added and
>>>>> Gatekeeper will make decisions in violation of the policy. The data
>>>>> structure of the LPM table is consistent, but its content
>>>>> inconsistent with the policy.
> max_rules and number_tbl8s in 'struct rte_lpm' contain the config information. These 2 fields do not change based on the routes added and do not indicate the amount of space left. So, you cannot use this information to decide if there is enough space to add more routes.

    We are aware that those fields hold the config information not a 
status of the LPM table.

    Before updating a LPM table that holds network prefixes derived from 
threat intelligence, we compute the minimum values for max_rules and 
number_tbl8s. Here is an example of how we do it: 
https://github.com/AltraMayor/gatekeeper/blob/95d1d6e8201861a0d0c698bfd06ad606674f1e07/lua/examples/policy.lua#L135-L166

    Once these minimum values are available, we get the parameters of 
the LPM table to be updated and check if we can update it, or have to 
recreate it.

>>>> Aha, thanks. So do I understand correctly that you need to add a set
>>>> of routes atomically (either the entire set is installed or nothing)?
>>>
>>>      Yes.
>>>
>>>> If so, then I would suggest having 2 lpm and switching them
>>>> atomically after a successful addition. As for now, even if you have
>>>> enough tbl8's, routes are installed non atomically, i.e. there will
>>>> be a time gap between adding two routes, so in this time interval the
>>>> table will be inconsistent with the policy.
>>>> Also, if new lpm algorithms are added to the DPDK, they won't have
>>>> such a thing as tbl8.
>>>
>>>      Our code already deals with synchronization.
> If the application code already deals with synchronization, is it possible to revert back (i.e. delete the routes that got added so far) when the addition of the route-set fails?

    The way the code is structured, this would require a significant 
rewrite because the code assumes that it will succeed since the capacity 
of the LPM tables was already checked.

>>>>>> On 13/10/2020 15:58, Michel Machado wrote:
>>>>>>> Hi Kevin,
>>>>>>>
>>>>>>>      We do need fields max_rules and number_tbl8s of struct
>>>>>>> rte_lpm, so the removal would force us to have another patch to
>>>>>>> our local copy of DPDK. We'd rather avoid this new local patch
>>>>>>> because we wish to eventually be in sync with the stock DPDK.
>>>>>>>
>>>>>>>      Those fields are needed in Gatekeeper because we found a
>>>>>>> condition in an ongoing deployment in which the entries of some
>>>>>>> LPM tables may suddenly change a lot to reflect policy changes. To
>>>>>>> avoid getting into a state in which the LPM table is inconsistent
>>>>>>> because it cannot fit all the new entries, we compute the needed
>>>>>>> parameters to support the new entries, and compare with the
>>>>>>> current parameters. If the current table doesn't fit everything,
>>>>>>> we have to replace it with a new LPM table.
>>>>>>>
>>>>>>>      If there were a way to obtain the struct rte_lpm_config of a
>>>>>>> given LPM table, it would cleanly address our need. We have the
>>>>>>> same need in IPv6 and have a local patch to work around it (see
>>>>>>>
>> https://github.com/cjdoucette/dpdk/commit/3eaf124a781349b8ec8cd880db
>> 26a78115cb8c8f).
> I do not see why such an API is not possible, we could add one API that returns max_rules and number_tbl8s (essentially, the config that was passed to rte_lpm_create API).
> But, is there a possibility to store that info in the application as that data was passed to rte_lpm from the application?

    A suggestion for what this API could look like:

void rte_lpm_get_config(const struct rte_lpm *lpm, struct rte_lpm_config 
*config);
void rte_lpm6_get_config(const struct rte_lpm6 *lpm, struct 
rte_lpm6_config *config);

    If the final choice is for not supporting a way to retrieve the 
config information on the API, we'll look for a place to keep a copy of 
the parameters in our code.

Re: [dpdk-dev] [PATCH 2/2] lpm: hide internal data

[Honnappa Nagarahalli]

<snip>
> 
> On 10/14/20 7:57 PM, Honnappa Nagarahalli wrote:
> >>>> On 13/10/2020 18:46, Michel Machado wrote:
> >>>>> On 10/13/20 11:41 AM, Medvedkin, Vladimir wrote:
> >>>>>> Hi Michel,
> >>>>>>
> >>>>>> Could you please describe a condition when LPM gets inconsistent?
> >>>>>> As I can see if there is no free tbl8 it will return -ENOSPC.
> >>>>>
> >>>>>      Consider this simple example, we need to add the following
> >>>>> two prefixes with different next hops: 10.99.0.0/16,
> >>>>> 18.99.99.128/25. If the LPM table is out of tbl8s, the second
> >>>>> prefix is not added and Gatekeeper will make decisions in
> >>>>> violation of the policy. The data structure of the LPM table is
> >>>>> consistent, but its content inconsistent with the policy.
> > max_rules and number_tbl8s in 'struct rte_lpm' contain the config
> information. These 2 fields do not change based on the routes added and do
> not indicate the amount of space left. So, you cannot use this information to
> decide if there is enough space to add more routes.
> 
>     We are aware that those fields hold the config information not a status of
> the LPM table.
> 
>     Before updating a LPM table that holds network prefixes derived from
> threat intelligence, we compute the minimum values for max_rules and
> number_tbl8s. Here is an example of how we do it:
> https://github.com/AltraMayor/gatekeeper/blob/95d1d6e8201861a0d0c698
> bfd06ad606674f1e07/lua/examples/policy.lua#L135-L166
> 
>     Once these minimum values are available, we get the parameters of the
> LPM table to be updated and check if we can update it, or have to recreate it.
> 
> >>>> Aha, thanks. So do I understand correctly that you need to add a
> >>>> set of routes atomically (either the entire set is installed or nothing)?
> >>>
> >>>      Yes.
> >>>
> >>>> If so, then I would suggest having 2 lpm and switching them
> >>>> atomically after a successful addition. As for now, even if you
> >>>> have enough tbl8's, routes are installed non atomically, i.e. there
> >>>> will be a time gap between adding two routes, so in this time
> >>>> interval the table will be inconsistent with the policy.
> >>>> Also, if new lpm algorithms are added to the DPDK, they won't have
> >>>> such a thing as tbl8.
> >>>
> >>>      Our code already deals with synchronization.
> > If the application code already deals with synchronization, is it possible to
> revert back (i.e. delete the routes that got added so far) when the addition
> of the route-set fails?
> 
>     The way the code is structured, this would require a significant rewrite
> because the code assumes that it will succeed since the capacity of the LPM
> tables was already checked.
> 
> >>>>>> On 13/10/2020 15:58, Michel Machado wrote:
> >>>>>>> Hi Kevin,
> >>>>>>>
> >>>>>>>      We do need fields max_rules and number_tbl8s of struct
> >>>>>>> rte_lpm, so the removal would force us to have another patch to
> >>>>>>> our local copy of DPDK. We'd rather avoid this new local patch
> >>>>>>> because we wish to eventually be in sync with the stock DPDK.
> >>>>>>>
> >>>>>>>      Those fields are needed in Gatekeeper because we found a
> >>>>>>> condition in an ongoing deployment in which the entries of some
> >>>>>>> LPM tables may suddenly change a lot to reflect policy changes.
> >>>>>>> To avoid getting into a state in which the LPM table is
> >>>>>>> inconsistent because it cannot fit all the new entries, we
> >>>>>>> compute the needed parameters to support the new entries, and
> >>>>>>> compare with the current parameters. If the current table
> >>>>>>> doesn't fit everything, we have to replace it with a new LPM table.
> >>>>>>>
> >>>>>>>      If there were a way to obtain the struct rte_lpm_config of
> >>>>>>> a given LPM table, it would cleanly address our need. We have
> >>>>>>> the same need in IPv6 and have a local patch to work around it
> >>>>>>> (see
> >>>>>>>
> >>
> https://github.com/cjdoucette/dpdk/commit/3eaf124a781349b8ec8cd880db
> >> 26a78115cb8c8f).
> > I do not see why such an API is not possible, we could add one API that
> returns max_rules and number_tbl8s (essentially, the config that was passed
> to rte_lpm_create API).
> > But, is there a possibility to store that info in the application as that data
> was passed to rte_lpm from the application?
> 
>     A suggestion for what this API could look like:
> 
> void rte_lpm_get_config(const struct rte_lpm *lpm, struct rte_lpm_config
> *config); void rte_lpm6_get_config(const struct rte_lpm6 *lpm, struct
> rte_lpm6_config *config);
> 
>     If the final choice is for not supporting a way to retrieve the config
> information on the API, we'll look for a place to keep a copy of the
> parameters in our code.
IMO, this is not a performance critical path and it is not a difficult solution to store these values in the application. My suggestion is to skip adding the API and store the values in the application.
Vladimir, what's your opinion?

Re: [dpdk-dev] [PATCH 2/2] lpm: hide internal data

[Medvedkin, Vladimir]

Hello,

On 15/10/2020 18:38, Honnappa Nagarahalli wrote:
> <snip>
>>
>> On 10/14/20 7:57 PM, Honnappa Nagarahalli wrote:
>>>>>> On 13/10/2020 18:46, Michel Machado wrote:
>>>>>>> On 10/13/20 11:41 AM, Medvedkin, Vladimir wrote:
>>>>>>>> Hi Michel,
>>>>>>>>
>>>>>>>> Could you please describe a condition when LPM gets inconsistent?
>>>>>>>> As I can see if there is no free tbl8 it will return -ENOSPC.
>>>>>>>
>>>>>>>       Consider this simple example, we need to add the following
>>>>>>> two prefixes with different next hops: 10.99.0.0/16,
>>>>>>> 18.99.99.128/25. If the LPM table is out of tbl8s, the second
>>>>>>> prefix is not added and Gatekeeper will make decisions in
>>>>>>> violation of the policy. The data structure of the LPM table is
>>>>>>> consistent, but its content inconsistent with the policy.
>>> max_rules and number_tbl8s in 'struct rte_lpm' contain the config
>> information. These 2 fields do not change based on the routes added and do
>> not indicate the amount of space left. So, you cannot use this information to
>> decide if there is enough space to add more routes.

Thanks Honnappa, agree, these two fields are read only after LPM 
initialization, I confused them with rte_fib's "rsvd_tbl8s" and 
"cur_tbl8s", so there is no need to read them directly from LPM after 
initialization. I'd suggest just keeping them as external variables 
outside of the LPM library (in kind of a global configuration I suppose?).

>>
>>      We are aware that those fields hold the config information not a status of
>> the LPM table.
>>
>>      Before updating a LPM table that holds network prefixes derived from
>> threat intelligence, we compute the minimum values for max_rules and
>> number_tbl8s. Here is an example of how we do it:
>> https://github.com/AltraMayor/gatekeeper/blob/95d1d6e8201861a0d0c698
>> bfd06ad606674f1e07/lua/examples/policy.lua#L135-L166
>>
>>      Once these minimum values are available, we get the parameters of the
>> LPM table to be updated and check if we can update it, or have to recreate it.
>>
>>>>>> Aha, thanks. So do I understand correctly that you need to add a
>>>>>> set of routes atomically (either the entire set is installed or nothing)?
>>>>>
>>>>>       Yes.
>>>>>
>>>>>> If so, then I would suggest having 2 lpm and switching them
>>>>>> atomically after a successful addition. As for now, even if you
>>>>>> have enough tbl8's, routes are installed non atomically, i.e. there
>>>>>> will be a time gap between adding two routes, so in this time
>>>>>> interval the table will be inconsistent with the policy.
>>>>>> Also, if new lpm algorithms are added to the DPDK, they won't have
>>>>>> such a thing as tbl8.
>>>>>
>>>>>       Our code already deals with synchronization.
>>> If the application code already deals with synchronization, is it possible to
>> revert back (i.e. delete the routes that got added so far) when the addition
>> of the route-set fails?
>>
>>      The way the code is structured, this would require a significant rewrite
>> because the code assumes that it will succeed since the capacity of the LPM
>> tables was already checked.
>>
>>>>>>>> On 13/10/2020 15:58, Michel Machado wrote:
>>>>>>>>> Hi Kevin,
>>>>>>>>>
>>>>>>>>>       We do need fields max_rules and number_tbl8s of struct
>>>>>>>>> rte_lpm, so the removal would force us to have another patch to
>>>>>>>>> our local copy of DPDK. We'd rather avoid this new local patch
>>>>>>>>> because we wish to eventually be in sync with the stock DPDK.
>>>>>>>>>
>>>>>>>>>       Those fields are needed in Gatekeeper because we found a
>>>>>>>>> condition in an ongoing deployment in which the entries of some
>>>>>>>>> LPM tables may suddenly change a lot to reflect policy changes.
>>>>>>>>> To avoid getting into a state in which the LPM table is
>>>>>>>>> inconsistent because it cannot fit all the new entries, we
>>>>>>>>> compute the needed parameters to support the new entries, and
>>>>>>>>> compare with the current parameters. If the current table
>>>>>>>>> doesn't fit everything, we have to replace it with a new LPM table.
>>>>>>>>>
>>>>>>>>>       If there were a way to obtain the struct rte_lpm_config of
>>>>>>>>> a given LPM table, it would cleanly address our need. We have
>>>>>>>>> the same need in IPv6 and have a local patch to work around it
>>>>>>>>> (see
>>>>>>>>>
>>>>
>> https://github.com/cjdoucette/dpdk/commit/3eaf124a781349b8ec8cd880db
>>>> 26a78115cb8c8f).
>>> I do not see why such an API is not possible, we could add one API that
>> returns max_rules and number_tbl8s (essentially, the config that was passed
>> to rte_lpm_create API).
>>> But, is there a possibility to store that info in the application as that data
>> was passed to rte_lpm from the application?
>>
>>      A suggestion for what this API could look like:
>>
>> void rte_lpm_get_config(const struct rte_lpm *lpm, struct rte_lpm_config
>> *config); void rte_lpm6_get_config(const struct rte_lpm6 *lpm, struct
>> rte_lpm6_config *config);
>>
>>      If the final choice is for not supporting a way to retrieve the config
>> information on the API, we'll look for a place to keep a copy of the
>> parameters in our code.
> IMO, this is not a performance critical path and it is not a difficult solution to store these values in the application. My suggestion is to skip adding the API and store the values in the application.
> Vladimir, what's your opinion?

Agree. Global vars or part of a global configuration could be used here.

> 

-- 
Regards,
Vladimir

Re: [dpdk-dev] [PATCH 2/2] lpm: hide internal data

[Honnappa Nagarahalli]

<snip>
> 
> Hello,
> 
> On 15/10/2020 18:38, Honnappa Nagarahalli wrote:
> > <snip>
> >>
> >> On 10/14/20 7:57 PM, Honnappa Nagarahalli wrote:
> >>>>>> On 13/10/2020 18:46, Michel Machado wrote:
> >>>>>>> On 10/13/20 11:41 AM, Medvedkin, Vladimir wrote:
> >>>>>>>> Hi Michel,
> >>>>>>>>
> >>>>>>>> Could you please describe a condition when LPM gets inconsistent?
> >>>>>>>> As I can see if there is no free tbl8 it will return -ENOSPC.
> >>>>>>>
> >>>>>>>       Consider this simple example, we need to add the following
> >>>>>>> two prefixes with different next hops: 10.99.0.0/16,
> >>>>>>> 18.99.99.128/25. If the LPM table is out of tbl8s, the second
> >>>>>>> prefix is not added and Gatekeeper will make decisions in
> >>>>>>> violation of the policy. The data structure of the LPM table is
> >>>>>>> consistent, but its content inconsistent with the policy.
> >>> max_rules and number_tbl8s in 'struct rte_lpm' contain the config
> >> information. These 2 fields do not change based on the routes added
> >> and do not indicate the amount of space left. So, you cannot use this
> >> information to decide if there is enough space to add more routes.
> 
> Thanks Honnappa, agree, these two fields are read only after LPM
> initialization, I confused them with rte_fib's "rsvd_tbl8s" and "cur_tbl8s", so
> there is no need to read them directly from LPM after initialization. I'd
> suggest just keeping them as external variables outside of the LPM library (in
> kind of a global configuration I suppose?).
> 
> >>
> >>      We are aware that those fields hold the config information not a
> >> status of the LPM table.
> >>
> >>      Before updating a LPM table that holds network prefixes derived
> >> from threat intelligence, we compute the minimum values for max_rules
> >> and number_tbl8s. Here is an example of how we do it:
> >>
> https://github.com/AltraMayor/gatekeeper/blob/95d1d6e8201861a0d0c698
> >> bfd06ad606674f1e07/lua/examples/policy.lua#L135-L166
> >>
> >>      Once these minimum values are available, we get the parameters
> >> of the LPM table to be updated and check if we can update it, or have to
> recreate it.
> >>
> >>>>>> Aha, thanks. So do I understand correctly that you need to add a
> >>>>>> set of routes atomically (either the entire set is installed or nothing)?
> >>>>>
> >>>>>       Yes.
> >>>>>
> >>>>>> If so, then I would suggest having 2 lpm and switching them
> >>>>>> atomically after a successful addition. As for now, even if you
> >>>>>> have enough tbl8's, routes are installed non atomically, i.e.
> >>>>>> there will be a time gap between adding two routes, so in this
> >>>>>> time interval the table will be inconsistent with the policy.
> >>>>>> Also, if new lpm algorithms are added to the DPDK, they won't
> >>>>>> have such a thing as tbl8.
> >>>>>
> >>>>>       Our code already deals with synchronization.
> >>> If the application code already deals with synchronization, is it
> >>> possible to
> >> revert back (i.e. delete the routes that got added so far) when the
> >> addition of the route-set fails?
> >>
> >>      The way the code is structured, this would require a significant
> >> rewrite because the code assumes that it will succeed since the
> >> capacity of the LPM tables was already checked.
> >>
> >>>>>>>> On 13/10/2020 15:58, Michel Machado wrote:
> >>>>>>>>> Hi Kevin,
> >>>>>>>>>
> >>>>>>>>>       We do need fields max_rules and number_tbl8s of struct
> >>>>>>>>> rte_lpm, so the removal would force us to have another patch
> >>>>>>>>> to our local copy of DPDK. We'd rather avoid this new local
> >>>>>>>>> patch because we wish to eventually be in sync with the stock
> DPDK.
> >>>>>>>>>
> >>>>>>>>>       Those fields are needed in Gatekeeper because we found a
> >>>>>>>>> condition in an ongoing deployment in which the entries of
> >>>>>>>>> some LPM tables may suddenly change a lot to reflect policy
> changes.
> >>>>>>>>> To avoid getting into a state in which the LPM table is
> >>>>>>>>> inconsistent because it cannot fit all the new entries, we
> >>>>>>>>> compute the needed parameters to support the new entries,
> and
> >>>>>>>>> compare with the current parameters. If the current table
> >>>>>>>>> doesn't fit everything, we have to replace it with a new LPM
> table.
> >>>>>>>>>
> >>>>>>>>>       If there were a way to obtain the struct rte_lpm_config
> >>>>>>>>> of a given LPM table, it would cleanly address our need. We
> >>>>>>>>> have the same need in IPv6 and have a local patch to work
> >>>>>>>>> around it (see
> >>>>>>>>>
> >>>>
> >>
> https://github.com/cjdoucette/dpdk/commit/3eaf124a781349b8ec8cd880db
> >>>> 26a78115cb8c8f).
> >>> I do not see why such an API is not possible, we could add one API
> >>> that
> >> returns max_rules and number_tbl8s (essentially, the config that was
> >> passed to rte_lpm_create API).
> >>> But, is there a possibility to store that info in the application as
> >>> that data
> >> was passed to rte_lpm from the application?
> >>
> >>      A suggestion for what this API could look like:
> >>
> >> void rte_lpm_get_config(const struct rte_lpm *lpm, struct
> >> rte_lpm_config *config); void rte_lpm6_get_config(const struct
> >> rte_lpm6 *lpm, struct rte_lpm6_config *config);
> >>
> >>      If the final choice is for not supporting a way to retrieve the
> >> config information on the API, we'll look for a place to keep a copy
> >> of the parameters in our code.
> > IMO, this is not a performance critical path and it is not a difficult solution to
> store these values in the application. My suggestion is to skip adding the API
> and store the values in the application.
> > Vladimir, what's your opinion?
> 
> Agree. Global vars or part of a global configuration could be used here.
Thank you. I think we are fine to go ahead with merging this patch.

> 
> >
> 
> --
> Regards,
> Vladimir

Re: [dpdk-dev] [PATCH 2/2] lpm: hide internal data

[Kevin Traynor]

On 15/10/2020 23:54, Honnappa Nagarahalli wrote:
> <snip>
>>
>> Hello,
>>
>> On 15/10/2020 18:38, Honnappa Nagarahalli wrote:
>>> <snip>
>>>>
>>>> On 10/14/20 7:57 PM, Honnappa Nagarahalli wrote:
>>>>>>>> On 13/10/2020 18:46, Michel Machado wrote:
>>>>>>>>> On 10/13/20 11:41 AM, Medvedkin, Vladimir wrote:
>>>>>>>>>> Hi Michel,
>>>>>>>>>>
>>>>>>>>>> Could you please describe a condition when LPM gets inconsistent?
>>>>>>>>>> As I can see if there is no free tbl8 it will return -ENOSPC.
>>>>>>>>>
>>>>>>>>>       Consider this simple example, we need to add the following
>>>>>>>>> two prefixes with different next hops: 10.99.0.0/16,
>>>>>>>>> 18.99.99.128/25. If the LPM table is out of tbl8s, the second
>>>>>>>>> prefix is not added and Gatekeeper will make decisions in
>>>>>>>>> violation of the policy. The data structure of the LPM table is
>>>>>>>>> consistent, but its content inconsistent with the policy.
>>>>> max_rules and number_tbl8s in 'struct rte_lpm' contain the config
>>>> information. These 2 fields do not change based on the routes added
>>>> and do not indicate the amount of space left. So, you cannot use this
>>>> information to decide if there is enough space to add more routes.
>>
>> Thanks Honnappa, agree, these two fields are read only after LPM
>> initialization, I confused them with rte_fib's "rsvd_tbl8s" and "cur_tbl8s", so
>> there is no need to read them directly from LPM after initialization. I'd
>> suggest just keeping them as external variables outside of the LPM library (in
>> kind of a global configuration I suppose?).
>>
>>>>
>>>>      We are aware that those fields hold the config information not a
>>>> status of the LPM table.
>>>>
>>>>      Before updating a LPM table that holds network prefixes derived
>>>> from threat intelligence, we compute the minimum values for max_rules
>>>> and number_tbl8s. Here is an example of how we do it:
>>>>
>> https://github.com/AltraMayor/gatekeeper/blob/95d1d6e8201861a0d0c698
>>>> bfd06ad606674f1e07/lua/examples/policy.lua#L135-L166
>>>>
>>>>      Once these minimum values are available, we get the parameters
>>>> of the LPM table to be updated and check if we can update it, or have to
>> recreate it.
>>>>
>>>>>>>> Aha, thanks. So do I understand correctly that you need to add a
>>>>>>>> set of routes atomically (either the entire set is installed or nothing)?
>>>>>>>
>>>>>>>       Yes.
>>>>>>>
>>>>>>>> If so, then I would suggest having 2 lpm and switching them
>>>>>>>> atomically after a successful addition. As for now, even if you
>>>>>>>> have enough tbl8's, routes are installed non atomically, i.e.
>>>>>>>> there will be a time gap between adding two routes, so in this
>>>>>>>> time interval the table will be inconsistent with the policy.
>>>>>>>> Also, if new lpm algorithms are added to the DPDK, they won't
>>>>>>>> have such a thing as tbl8.
>>>>>>>
>>>>>>>       Our code already deals with synchronization.
>>>>> If the application code already deals with synchronization, is it
>>>>> possible to
>>>> revert back (i.e. delete the routes that got added so far) when the
>>>> addition of the route-set fails?
>>>>
>>>>      The way the code is structured, this would require a significant
>>>> rewrite because the code assumes that it will succeed since the
>>>> capacity of the LPM tables was already checked.
>>>>
>>>>>>>>>> On 13/10/2020 15:58, Michel Machado wrote:
>>>>>>>>>>> Hi Kevin,
>>>>>>>>>>>
>>>>>>>>>>>       We do need fields max_rules and number_tbl8s of struct
>>>>>>>>>>> rte_lpm, so the removal would force us to have another patch
>>>>>>>>>>> to our local copy of DPDK. We'd rather avoid this new local
>>>>>>>>>>> patch because we wish to eventually be in sync with the stock
>> DPDK.
>>>>>>>>>>>
>>>>>>>>>>>       Those fields are needed in Gatekeeper because we found a
>>>>>>>>>>> condition in an ongoing deployment in which the entries of
>>>>>>>>>>> some LPM tables may suddenly change a lot to reflect policy
>> changes.
>>>>>>>>>>> To avoid getting into a state in which the LPM table is
>>>>>>>>>>> inconsistent because it cannot fit all the new entries, we
>>>>>>>>>>> compute the needed parameters to support the new entries,
>> and
>>>>>>>>>>> compare with the current parameters. If the current table
>>>>>>>>>>> doesn't fit everything, we have to replace it with a new LPM
>> table.
>>>>>>>>>>>
>>>>>>>>>>>       If there were a way to obtain the struct rte_lpm_config
>>>>>>>>>>> of a given LPM table, it would cleanly address our need. We
>>>>>>>>>>> have the same need in IPv6 and have a local patch to work
>>>>>>>>>>> around it (see
>>>>>>>>>>>
>>>>>>
>>>>
>> https://github.com/cjdoucette/dpdk/commit/3eaf124a781349b8ec8cd880db
>>>>>> 26a78115cb8c8f).
>>>>> I do not see why such an API is not possible, we could add one API
>>>>> that
>>>> returns max_rules and number_tbl8s (essentially, the config that was
>>>> passed to rte_lpm_create API).
>>>>> But, is there a possibility to store that info in the application as
>>>>> that data
>>>> was passed to rte_lpm from the application?
>>>>
>>>>      A suggestion for what this API could look like:
>>>>
>>>> void rte_lpm_get_config(const struct rte_lpm *lpm, struct
>>>> rte_lpm_config *config); void rte_lpm6_get_config(const struct
>>>> rte_lpm6 *lpm, struct rte_lpm6_config *config);
>>>>
>>>>      If the final choice is for not supporting a way to retrieve the
>>>> config information on the API, we'll look for a place to keep a copy
>>>> of the parameters in our code.
>>> IMO, this is not a performance critical path and it is not a difficult solution to
>> store these values in the application. My suggestion is to skip adding the API
>> and store the values in the application.
>>> Vladimir, what's your opinion?
>>
>> Agree. Global vars or part of a global configuration could be used here.
> Thank you. I think we are fine to go ahead with merging this patch.
> 

Michel, I know it's a bit of churn so not zero effort, but is that
solution workable for you? The change in DPDK would not be backported,
so gatekeeper code would only need an update on updating to use DPDK 20.11.

Kevin.

P.S. As you are using DPDK 19.08, we should talk about DPDK LTS but
let's leave that for another thread :-)

>>
>>>
>>
>> --
>> Regards,
>> Vladimir

Re: [dpdk-dev] [PATCH 2/2] lpm: hide internal data

[Michel Machado]

>>>>>       If the final choice is for not supporting a way to retrieve the
>>>>> config information on the API, we'll look for a place to keep a copy
>>>>> of the parameters in our code.
>>>> IMO, this is not a performance critical path and it is not a difficult solution to
>>> store these values in the application. My suggestion is to skip adding the API
>>> and store the values in the application.
>>>> Vladimir, what's your opinion?
>>>
>>> Agree. Global vars or part of a global configuration could be used here.
>> Thank you. I think we are fine to go ahead with merging this patch.
>>
> 
> Michel, I know it's a bit of churn so not zero effort, but is that
> solution workable for you? The change in DPDK would not be backported,
> so gatekeeper code would only need an update on updating to use DPDK 20.11.

    It's workable.

    Thank you for organizing this discussion, Kevin.

> P.S. As you are using DPDK 19.08, we should talk about DPDK LTS but
> let's leave that for another thread :-)

    Sure.

Re: [dpdk-dev] [PATCH 2/2] lpm: hide internal data

[David Marchand]

On Fri, Oct 16, 2020 at 12:54 AM Honnappa Nagarahalli
<Honnappa.Nagarahalli@arm.com> wrote:
> > > IMO, this is not a performance critical path and it is not a difficult solution to
> > store these values in the application. My suggestion is to skip adding the API
> > and store the values in the application.
> > > Vladimir, what's your opinion?
> >
> > Agree. Global vars or part of a global configuration could be used here.
> Thank you. I think we are fine to go ahead with merging this patch.

I saw Honnappa and Kevin acks, are other techboard members aware and
okay with this change?
Thanks.

-- 
David Marchand

Re: [dpdk-dev] [PATCH 2/2] lpm: hide internal data

[Thomas Monjalon]

19/10/2020 16:53, David Marchand:
> On Fri, Oct 16, 2020 at 12:54 AM Honnappa Nagarahalli
> <Honnappa.Nagarahalli@arm.com> wrote:
> > > > IMO, this is not a performance critical path and it is not a difficult solution to
> > > store these values in the application. My suggestion is to skip adding the API
> > > and store the values in the application.
> > > > Vladimir, what's your opinion?
> > >
> > > Agree. Global vars or part of a global configuration could be used here.
> > Thank you. I think we are fine to go ahead with merging this patch.
> 
> I saw Honnappa and Kevin acks, are other techboard members aware and
> okay with this change?

This is an API change for better maintenance.
I'm not sure the benefit is big enough,
but the cost for the applications is said to be workable.
Better to do it now than later, so

Acked-by: Thomas Monjalon <thomas@monjalon.net>
(at the condition it is added in release notes)

What others think?

Re: [dpdk-dev] [PATCH 2/2] lpm: hide internal data

[Medvedkin, Vladimir]

Hi,

On 20/10/2020 15:22, Thomas Monjalon wrote:
> 19/10/2020 16:53, David Marchand:
>> On Fri, Oct 16, 2020 at 12:54 AM Honnappa Nagarahalli
>> <Honnappa.Nagarahalli@arm.com> wrote:
>>>>> IMO, this is not a performance critical path and it is not a difficult solution to
>>>> store these values in the application. My suggestion is to skip adding the API
>>>> and store the values in the application.
>>>>> Vladimir, what's your opinion?
>>>>
>>>> Agree. Global vars or part of a global configuration could be used here.
>>> Thank you. I think we are fine to go ahead with merging this patch.
>>
>> I saw Honnappa and Kevin acks, are other techboard members aware and
>> okay with this change?
> 
> This is an API change for better maintenance.
> I'm not sure the benefit is big enough,
> but the cost for the applications is said to be workable.
> Better to do it now than later, so
> 
> Acked-by: Thomas Monjalon <thomas@monjalon.net>
> (at the condition it is added in release notes)
> 
> What others think?
> 

This changes LGTM (agree with Thomas about release notes).
Acked-by: Vladimir Medvedkin <vladimir.medvedkin@intel.com>

> 

-- 
Regards,
Vladimir

Re: [dpdk-dev] [PATCH 2/2] lpm: hide internal data

[Honnappa Nagarahalli]

<snip>

> > >>
> > >> Hi Ruifeng,
> > >>
> > >> On 15/09/2020 17:02, Bruce Richardson wrote:
> > >>> On Mon, Sep 07, 2020 at 04:15:17PM +0800, Ruifeng Wang wrote:
> > >>>> Fields except tbl24 and tbl8 in rte_lpm structure have no need to
> > >>>> be exposed to the user.
> > >>>> Hide the unneeded exposure of structure fields for better ABI
> > >>>> maintainability.
> > >>>>
> > >>>> Suggested-by: David Marchand <david.marchand@redhat.com>
> > >>>> Signed-off-by: Ruifeng Wang <ruifeng.wang@arm.com>
> > >>>> Reviewed-by: Phil Yang <phil.yang@arm.com>
Reviewed-by: Honnappa Nagarahalli <honnappa.nagarahalli@arm.com>

<snip>

Re: [dpdk-dev] [PATCH 0/2] LPM changes

[David Marchand]

On Mon, Sep 7, 2020 at 10:15 AM Ruifeng Wang <ruifeng.wang@arm.com> wrote:
>
> The rte_lpm structure is exported because lookup API is inlined.
> But most of the structure can be hidden.
> Discussion at: http://patches.dpdk.org/patch/72403/
> This patch set aimed to hide the rte_lpm structure as much as possible.
>
> A data free issue was identified and fixed.
>
> Ruifeng Wang (2):
>   lpm: fix free of data structure
>   lpm: hide internal data
>
>  lib/librte_lpm/rte_lpm.c | 154 +++++++++++++++++++++++----------------
>  lib/librte_lpm/rte_lpm.h |   7 --
>  2 files changed, 92 insertions(+), 69 deletions(-)

Cc: maintainers.

Those changes look good to me.
Opinions?


Thanks.
-- 
David Marchand

Re: [dpdk-dev] [PATCH 0/2] LPM changes

[Kevin Traynor]

On 15/09/2020 15:41, David Marchand wrote:
> On Mon, Sep 7, 2020 at 10:15 AM Ruifeng Wang <ruifeng.wang@arm.com> wrote:
>>
>> The rte_lpm structure is exported because lookup API is inlined.
>> But most of the structure can be hidden.
>> Discussion at: http://patches.dpdk.org/patch/72403/
>> This patch set aimed to hide the rte_lpm structure as much as possible.
>>
>> A data free issue was identified and fixed.
>>
>> Ruifeng Wang (2):
>>   lpm: fix free of data structure
>>   lpm: hide internal data
>>
>>  lib/librte_lpm/rte_lpm.c | 154 +++++++++++++++++++++++----------------
>>  lib/librte_lpm/rte_lpm.h |   7 --
>>  2 files changed, 92 insertions(+), 69 deletions(-)
> 
> Cc: maintainers.
> 
> Those changes look good to me.
> Opinions?
> 
> 

LGTM, Gatekeeper confirmed they can handle this change.

Acked-by: Kevin Traynor <ktraynor@redhat.com>

> Thanks.
> 

[dpdk-dev] [PATCH v2 0/2] LPM changes

[Ruifeng Wang]

The rte_lpm structure is exported because lookup API is inlined.
But most of the structure can be hidden.
Discussion at: http://patches.dpdk.org/patch/72403/
This patch set aimed to hide the rte_lpm structure as much as possible.

A data free issue was identified and fixed.

Ruifeng Wang (2):
  lpm: fix free of data structure
  lpm: hide internal data

 doc/guides/rel_notes/release_20_11.rst |   2 +
 lib/librte_lpm/rte_lpm.c               | 154 +++++++++++++++----------
 lib/librte_lpm/rte_lpm.h               |   7 --
 3 files changed, 94 insertions(+), 69 deletions(-)

-- 
2.20.1

[dpdk-dev] [PATCH v2 1/2] lpm: fix free of data structure

[Ruifeng Wang]

The container structure should be freed instead of rte_lpm structure
after wrapping rte_lpm into internal structure __rte_lpm.

Fixes: 8a9f8564e9f9 ("lpm: implement RCU rule reclamation")
Cc: stable@dpdk.org

Signed-off-by: Ruifeng Wang <ruifeng.wang@arm.com>
Reviewed-by: Phil Yang <phil.yang@arm.com>
Acked-by: Bruce Richardson <bruce.richardson@intel.com>
Acked-by: Vladimir Medvedkin <vladimir.medvedkin@intel.com>
Acked-by: Kevin Traynor <ktraynor@redhat.com>
---
 lib/librte_lpm/rte_lpm.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/lib/librte_lpm/rte_lpm.c b/lib/librte_lpm/rte_lpm.c
index 757436f49..51a0ae578 100644
--- a/lib/librte_lpm/rte_lpm.c
+++ b/lib/librte_lpm/rte_lpm.c
@@ -268,7 +268,7 @@ rte_lpm_free(struct rte_lpm *lpm)
 		rte_rcu_qsbr_dq_delete(internal_lpm->dq);
 	rte_free(lpm->tbl8);
 	rte_free(lpm->rules_tbl);
-	rte_free(lpm);
+	rte_free(internal_lpm);
 	rte_free(te);
 }
 
-- 
2.20.1

[dpdk-dev] [PATCH v2 2/2] lpm: hide internal data

[Ruifeng Wang]

Fields except tbl24 and tbl8 in rte_lpm structure have no
need to be exposed to the user.
Hide the unneeded exposure of structure fields for better
ABI maintainability.

Suggested-by: David Marchand <david.marchand@redhat.com>
Signed-off-by: Ruifeng Wang <ruifeng.wang@arm.com>
Reviewed-by: Honnappa Nagarahalli <honnappa.nagarahalli@arm.com>
Acked-by: Kevin Traynor <ktraynor@redhat.com>
Acked-by: Thomas Monjalon <thomas@monjalon.net>
Acked-by: Vladimir Medvedkin <vladimir.medvedkin@intel.com>
---
v2:
Added release notes.

 doc/guides/rel_notes/release_20_11.rst |   2 +
 lib/librte_lpm/rte_lpm.c               | 152 +++++++++++++++----------
 lib/librte_lpm/rte_lpm.h               |   7 --
 3 files changed, 93 insertions(+), 68 deletions(-)

diff --git a/doc/guides/rel_notes/release_20_11.rst b/doc/guides/rel_notes/release_20_11.rst
index 0d45b5003..3b5034ce5 100644
--- a/doc/guides/rel_notes/release_20_11.rst
+++ b/doc/guides/rel_notes/release_20_11.rst
@@ -602,6 +602,8 @@ ABI Changes
 
 * sched: Added new fields to ``struct rte_sched_subport_port_params``.
 
+* lpm: Removed fields other than ``tbl24`` and ``tbl8`` from the struct ``rte_lpm``.
+  The removed fields were made internal.
 
 Known Issues
 ------------
diff --git a/lib/librte_lpm/rte_lpm.c b/lib/librte_lpm/rte_lpm.c
index 51a0ae578..88d31df6d 100644
--- a/lib/librte_lpm/rte_lpm.c
+++ b/lib/librte_lpm/rte_lpm.c
@@ -42,9 +42,17 @@ enum valid_flag {
 
 /** @internal LPM structure. */
 struct __rte_lpm {
-	/* LPM metadata. */
+	/* Exposed LPM data. */
 	struct rte_lpm lpm;
 
+	/* LPM metadata. */
+	char name[RTE_LPM_NAMESIZE];        /**< Name of the lpm. */
+	uint32_t max_rules; /**< Max. balanced rules per lpm. */
+	uint32_t number_tbl8s; /**< Number of tbl8s. */
+	/**< Rule info table. */
+	struct rte_lpm_rule_info rule_info[RTE_LPM_MAX_DEPTH];
+	struct rte_lpm_rule *rules_tbl; /**< LPM rules. */
+
 	/* RCU config. */
 	struct rte_rcu_qsbr *v;		/* RCU QSBR variable. */
 	enum rte_lpm_qsbr_mode rcu_mode;/* Blocking, defer queue. */
@@ -104,7 +112,7 @@ depth_to_range(uint8_t depth)
 struct rte_lpm *
 rte_lpm_find_existing(const char *name)
 {
-	struct rte_lpm *l = NULL;
+	struct __rte_lpm *l = NULL;
 	struct rte_tailq_entry *te;
 	struct rte_lpm_list *lpm_list;
 
@@ -123,7 +131,7 @@ rte_lpm_find_existing(const char *name)
 		return NULL;
 	}
 
-	return l;
+	return &l->lpm;
 }
 
 /*
@@ -157,8 +165,8 @@ rte_lpm_create(const char *name, int socket_id,
 
 	/* guarantee there's no existing */
 	TAILQ_FOREACH(te, lpm_list, next) {
-		lpm = te->data;
-		if (strncmp(name, lpm->name, RTE_LPM_NAMESIZE) == 0)
+		internal_lpm = te->data;
+		if (strncmp(name, internal_lpm->name, RTE_LPM_NAMESIZE) == 0)
 			break;
 	}
 
@@ -193,10 +201,10 @@ rte_lpm_create(const char *name, int socket_id,
 	}
 
 	lpm = &internal_lpm->lpm;
-	lpm->rules_tbl = rte_zmalloc_socket(NULL,
+	internal_lpm->rules_tbl = rte_zmalloc_socket(NULL,
 			(size_t)rules_size, RTE_CACHE_LINE_SIZE, socket_id);
 
-	if (lpm->rules_tbl == NULL) {
+	if (internal_lpm->rules_tbl == NULL) {
 		RTE_LOG(ERR, LPM, "LPM rules_tbl memory allocation failed\n");
 		rte_free(internal_lpm);
 		internal_lpm = NULL;
@@ -211,7 +219,7 @@ rte_lpm_create(const char *name, int socket_id,
 
 	if (lpm->tbl8 == NULL) {
 		RTE_LOG(ERR, LPM, "LPM tbl8 memory allocation failed\n");
-		rte_free(lpm->rules_tbl);
+		rte_free(internal_lpm->rules_tbl);
 		rte_free(internal_lpm);
 		internal_lpm = NULL;
 		lpm = NULL;
@@ -221,11 +229,11 @@ rte_lpm_create(const char *name, int socket_id,
 	}
 
 	/* Save user arguments. */
-	lpm->max_rules = config->max_rules;
-	lpm->number_tbl8s = config->number_tbl8s;
-	strlcpy(lpm->name, name, sizeof(lpm->name));
+	internal_lpm->max_rules = config->max_rules;
+	internal_lpm->number_tbl8s = config->number_tbl8s;
+	strlcpy(internal_lpm->name, name, sizeof(internal_lpm->name));
 
-	te->data = lpm;
+	te->data = internal_lpm;
 
 	TAILQ_INSERT_TAIL(lpm_list, te, next);
 
@@ -241,7 +249,7 @@ rte_lpm_create(const char *name, int socket_id,
 void
 rte_lpm_free(struct rte_lpm *lpm)
 {
-	struct __rte_lpm *internal_lpm;
+	struct __rte_lpm *internal_lpm = NULL;
 	struct rte_lpm_list *lpm_list;
 	struct rte_tailq_entry *te;
 
@@ -255,7 +263,8 @@ rte_lpm_free(struct rte_lpm *lpm)
 
 	/* find our tailq entry */
 	TAILQ_FOREACH(te, lpm_list, next) {
-		if (te->data == (void *) lpm)
+		internal_lpm = te->data;
+		if (&internal_lpm->lpm == lpm)
 			break;
 	}
 	if (te != NULL)
@@ -263,11 +272,10 @@ rte_lpm_free(struct rte_lpm *lpm)
 
 	rte_mcfg_tailq_write_unlock();
 
-	internal_lpm = container_of(lpm, struct __rte_lpm, lpm);
 	if (internal_lpm->dq != NULL)
 		rte_rcu_qsbr_dq_delete(internal_lpm->dq);
 	rte_free(lpm->tbl8);
-	rte_free(lpm->rules_tbl);
+	rte_free(internal_lpm->rules_tbl);
 	rte_free(internal_lpm);
 	rte_free(te);
 }
@@ -310,11 +318,11 @@ rte_lpm_rcu_qsbr_add(struct rte_lpm *lpm, struct rte_lpm_rcu_config *cfg)
 	} else if (cfg->mode == RTE_LPM_QSBR_MODE_DQ) {
 		/* Init QSBR defer queue. */
 		snprintf(rcu_dq_name, sizeof(rcu_dq_name),
-				"LPM_RCU_%s", lpm->name);
+				"LPM_RCU_%s", internal_lpm->name);
 		params.name = rcu_dq_name;
 		params.size = cfg->dq_size;
 		if (params.size == 0)
-			params.size = lpm->number_tbl8s;
+			params.size = internal_lpm->number_tbl8s;
 		params.trigger_reclaim_limit = cfg->reclaim_thd;
 		params.max_reclaim_size = cfg->reclaim_max;
 		if (params.max_reclaim_size == 0)
@@ -352,74 +360,79 @@ static int32_t
 rule_add(struct rte_lpm *lpm, uint32_t ip_masked, uint8_t depth,
 	uint32_t next_hop)
 {
-	uint32_t rule_gindex, rule_index, last_rule;
+	uint32_t rule_gindex, rule_index, last_rule, first_index;
+	struct __rte_lpm *i_lpm;
 	int i;
 
 	VERIFY_DEPTH(depth);
 
+	i_lpm = container_of(lpm, struct __rte_lpm, lpm);
 	/* Scan through rule group to see if rule already exists. */
-	if (lpm->rule_info[depth - 1].used_rules > 0) {
+	if (i_lpm->rule_info[depth - 1].used_rules > 0) {
 
 		/* rule_gindex stands for rule group index. */
-		rule_gindex = lpm->rule_info[depth - 1].first_rule;
+		rule_gindex = i_lpm->rule_info[depth - 1].first_rule;
 		/* Initialise rule_index to point to start of rule group. */
 		rule_index = rule_gindex;
 		/* Last rule = Last used rule in this rule group. */
-		last_rule = rule_gindex + lpm->rule_info[depth - 1].used_rules;
+		last_rule = rule_gindex
+				+ i_lpm->rule_info[depth - 1].used_rules;
 
 		for (; rule_index < last_rule; rule_index++) {
 
 			/* If rule already exists update next hop and return. */
-			if (lpm->rules_tbl[rule_index].ip == ip_masked) {
+			if (i_lpm->rules_tbl[rule_index].ip == ip_masked) {
 
-				if (lpm->rules_tbl[rule_index].next_hop
+				if (i_lpm->rules_tbl[rule_index].next_hop
 						== next_hop)
 					return -EEXIST;
-				lpm->rules_tbl[rule_index].next_hop = next_hop;
+				i_lpm->rules_tbl[rule_index].next_hop
+					= next_hop;
 
 				return rule_index;
 			}
 		}
 
-		if (rule_index == lpm->max_rules)
+		if (rule_index == i_lpm->max_rules)
 			return -ENOSPC;
 	} else {
 		/* Calculate the position in which the rule will be stored. */
 		rule_index = 0;
 
 		for (i = depth - 1; i > 0; i--) {
-			if (lpm->rule_info[i - 1].used_rules > 0) {
-				rule_index = lpm->rule_info[i - 1].first_rule
-						+ lpm->rule_info[i - 1].used_rules;
+			if (i_lpm->rule_info[i - 1].used_rules > 0) {
+				rule_index = i_lpm->rule_info[i - 1].first_rule
+					+ i_lpm->rule_info[i - 1].used_rules;
 				break;
 			}
 		}
-		if (rule_index == lpm->max_rules)
+		if (rule_index == i_lpm->max_rules)
 			return -ENOSPC;
 
-		lpm->rule_info[depth - 1].first_rule = rule_index;
+		i_lpm->rule_info[depth - 1].first_rule = rule_index;
 	}
 
 	/* Make room for the new rule in the array. */
 	for (i = RTE_LPM_MAX_DEPTH; i > depth; i--) {
-		if (lpm->rule_info[i - 1].first_rule
-				+ lpm->rule_info[i - 1].used_rules == lpm->max_rules)
+		first_index = i_lpm->rule_info[i - 1].first_rule;
+		if (first_index + i_lpm->rule_info[i - 1].used_rules
+				== i_lpm->max_rules)
 			return -ENOSPC;
 
-		if (lpm->rule_info[i - 1].used_rules > 0) {
-			lpm->rules_tbl[lpm->rule_info[i - 1].first_rule
-				+ lpm->rule_info[i - 1].used_rules]
-					= lpm->rules_tbl[lpm->rule_info[i - 1].first_rule];
-			lpm->rule_info[i - 1].first_rule++;
+		if (i_lpm->rule_info[i - 1].used_rules > 0) {
+			i_lpm->rules_tbl[first_index
+				+ i_lpm->rule_info[i - 1].used_rules]
+					= i_lpm->rules_tbl[first_index];
+			i_lpm->rule_info[i - 1].first_rule++;
 		}
 	}
 
 	/* Add the new rule. */
-	lpm->rules_tbl[rule_index].ip = ip_masked;
-	lpm->rules_tbl[rule_index].next_hop = next_hop;
+	i_lpm->rules_tbl[rule_index].ip = ip_masked;
+	i_lpm->rules_tbl[rule_index].next_hop = next_hop;
 
 	/* Increment the used rules counter for this rule group. */
-	lpm->rule_info[depth - 1].used_rules++;
+	i_lpm->rule_info[depth - 1].used_rules++;
 
 	return rule_index;
 }
@@ -432,23 +445,25 @@ static void
 rule_delete(struct rte_lpm *lpm, int32_t rule_index, uint8_t depth)
 {
 	int i;
+	struct __rte_lpm *i_lpm;
 
 	VERIFY_DEPTH(depth);
 
-	lpm->rules_tbl[rule_index] =
-			lpm->rules_tbl[lpm->rule_info[depth - 1].first_rule
-			+ lpm->rule_info[depth - 1].used_rules - 1];
+	i_lpm = container_of(lpm, struct __rte_lpm, lpm);
+	i_lpm->rules_tbl[rule_index] =
+			i_lpm->rules_tbl[i_lpm->rule_info[depth - 1].first_rule
+			+ i_lpm->rule_info[depth - 1].used_rules - 1];
 
 	for (i = depth; i < RTE_LPM_MAX_DEPTH; i++) {
-		if (lpm->rule_info[i].used_rules > 0) {
-			lpm->rules_tbl[lpm->rule_info[i].first_rule - 1] =
-					lpm->rules_tbl[lpm->rule_info[i].first_rule
-						+ lpm->rule_info[i].used_rules - 1];
-			lpm->rule_info[i].first_rule--;
+		if (i_lpm->rule_info[i].used_rules > 0) {
+			i_lpm->rules_tbl[i_lpm->rule_info[i].first_rule - 1] =
+				i_lpm->rules_tbl[i_lpm->rule_info[i].first_rule
+					+ i_lpm->rule_info[i].used_rules - 1];
+			i_lpm->rule_info[i].first_rule--;
 		}
 	}
 
-	lpm->rule_info[depth - 1].used_rules--;
+	i_lpm->rule_info[depth - 1].used_rules--;
 }
 
 /*
@@ -459,16 +474,18 @@ static int32_t
 rule_find(struct rte_lpm *lpm, uint32_t ip_masked, uint8_t depth)
 {
 	uint32_t rule_gindex, last_rule, rule_index;
+	struct __rte_lpm *internal_lpm;
 
 	VERIFY_DEPTH(depth);
 
-	rule_gindex = lpm->rule_info[depth - 1].first_rule;
-	last_rule = rule_gindex + lpm->rule_info[depth - 1].used_rules;
+	internal_lpm = container_of(lpm, struct __rte_lpm, lpm);
+	rule_gindex = internal_lpm->rule_info[depth - 1].first_rule;
+	last_rule = rule_gindex + internal_lpm->rule_info[depth - 1].used_rules;
 
 	/* Scan used rules at given depth to find rule. */
 	for (rule_index = rule_gindex; rule_index < last_rule; rule_index++) {
 		/* If rule is found return the rule index. */
-		if (lpm->rules_tbl[rule_index].ip == ip_masked)
+		if (internal_lpm->rules_tbl[rule_index].ip == ip_masked)
 			return rule_index;
 	}
 
@@ -484,9 +501,11 @@ _tbl8_alloc(struct rte_lpm *lpm)
 {
 	uint32_t group_idx; /* tbl8 group index. */
 	struct rte_lpm_tbl_entry *tbl8_entry;
+	struct __rte_lpm *i_lpm;
 
+	i_lpm = container_of(lpm, struct __rte_lpm, lpm);
 	/* Scan through tbl8 to find a free (i.e. INVALID) tbl8 group. */
-	for (group_idx = 0; group_idx < lpm->number_tbl8s; group_idx++) {
+	for (group_idx = 0; group_idx < i_lpm->number_tbl8s; group_idx++) {
 		tbl8_entry = &lpm->tbl8[group_idx *
 					RTE_LPM_TBL8_GROUP_NUM_ENTRIES];
 		/* If a free tbl8 group is found clean it and set as VALID. */
@@ -844,6 +863,7 @@ uint32_t *next_hop)
 {
 	uint32_t ip_masked;
 	int32_t rule_index;
+	struct __rte_lpm *internal_lpm;
 
 	/* Check user arguments. */
 	if ((lpm == NULL) ||
@@ -855,8 +875,9 @@ uint32_t *next_hop)
 	ip_masked = ip & depth_to_mask(depth);
 	rule_index = rule_find(lpm, ip_masked, depth);
 
+	internal_lpm = container_of(lpm, struct __rte_lpm, lpm);
 	if (rule_index >= 0) {
-		*next_hop = lpm->rules_tbl[rule_index].next_hop;
+		*next_hop = internal_lpm->rules_tbl[rule_index].next_hop;
 		return 1;
 	}
 
@@ -897,7 +918,9 @@ delete_depth_small(struct rte_lpm *lpm, uint32_t ip_masked,
 	tbl24_range = depth_to_range(depth);
 	tbl24_index = (ip_masked >> 8);
 	struct rte_lpm_tbl_entry zero_tbl24_entry = {0};
+	struct __rte_lpm *i_lpm;
 
+	i_lpm = container_of(lpm, struct __rte_lpm, lpm);
 	/*
 	 * Firstly check the sub_rule_index. A -1 indicates no replacement rule
 	 * and a positive number indicates a sub_rule_index.
@@ -939,7 +962,7 @@ delete_depth_small(struct rte_lpm *lpm, uint32_t ip_masked,
 		 */
 
 		struct rte_lpm_tbl_entry new_tbl24_entry = {
-			.next_hop = lpm->rules_tbl[sub_rule_index].next_hop,
+			.next_hop = i_lpm->rules_tbl[sub_rule_index].next_hop,
 			.valid = VALID,
 			.valid_group = 0,
 			.depth = sub_rule_depth,
@@ -949,7 +972,7 @@ delete_depth_small(struct rte_lpm *lpm, uint32_t ip_masked,
 			.valid = VALID,
 			.valid_group = VALID,
 			.depth = sub_rule_depth,
-			.next_hop = lpm->rules_tbl
+			.next_hop = i_lpm->rules_tbl
 			[sub_rule_index].next_hop,
 		};
 
@@ -1048,6 +1071,7 @@ delete_depth_big(struct rte_lpm *lpm, uint32_t ip_masked,
 	uint32_t tbl24_index, tbl8_group_index, tbl8_group_start, tbl8_index,
 			tbl8_range, i;
 	int32_t tbl8_recycle_index, status = 0;
+	struct __rte_lpm *i_lpm;
 
 	/*
 	 * Calculate the index into tbl24 and range. Note: All depths larger
@@ -1061,6 +1085,7 @@ delete_depth_big(struct rte_lpm *lpm, uint32_t ip_masked,
 	tbl8_index = tbl8_group_start + (ip_masked & 0xFF);
 	tbl8_range = depth_to_range(depth);
 
+	i_lpm = container_of(lpm, struct __rte_lpm, lpm);
 	if (sub_rule_index < 0) {
 		/*
 		 * Loop through the range of entries on tbl8 for which the
@@ -1076,7 +1101,7 @@ delete_depth_big(struct rte_lpm *lpm, uint32_t ip_masked,
 			.valid = VALID,
 			.depth = sub_rule_depth,
 			.valid_group = lpm->tbl8[tbl8_group_start].valid_group,
-			.next_hop = lpm->rules_tbl[sub_rule_index].next_hop,
+			.next_hop = i_lpm->rules_tbl[sub_rule_index].next_hop,
 		};
 
 		/*
@@ -1188,16 +1213,21 @@ rte_lpm_delete(struct rte_lpm *lpm, uint32_t ip, uint8_t depth)
 void
 rte_lpm_delete_all(struct rte_lpm *lpm)
 {
+	struct __rte_lpm *internal_lpm;
+
+	internal_lpm = container_of(lpm, struct __rte_lpm, lpm);
 	/* Zero rule information. */
-	memset(lpm->rule_info, 0, sizeof(lpm->rule_info));
+	memset(internal_lpm->rule_info, 0, sizeof(internal_lpm->rule_info));
 
 	/* Zero tbl24. */
 	memset(lpm->tbl24, 0, sizeof(lpm->tbl24));
 
 	/* Zero tbl8. */
 	memset(lpm->tbl8, 0, sizeof(lpm->tbl8[0])
-			* RTE_LPM_TBL8_GROUP_NUM_ENTRIES * lpm->number_tbl8s);
+			* RTE_LPM_TBL8_GROUP_NUM_ENTRIES
+			* internal_lpm->number_tbl8s);
 
 	/* Delete all rules form the rules table. */
-	memset(lpm->rules_tbl, 0, sizeof(lpm->rules_tbl[0]) * lpm->max_rules);
+	memset(internal_lpm->rules_tbl, 0,
+		sizeof(internal_lpm->rules_tbl[0]) * internal_lpm->max_rules);
 }
diff --git a/lib/librte_lpm/rte_lpm.h b/lib/librte_lpm/rte_lpm.h
index 5b3b7b5b5..9a0ac97ab 100644
--- a/lib/librte_lpm/rte_lpm.h
+++ b/lib/librte_lpm/rte_lpm.h
@@ -132,17 +132,10 @@ struct rte_lpm_rule_info {
 
 /** @internal LPM structure. */
 struct rte_lpm {
-	/* LPM metadata. */
-	char name[RTE_LPM_NAMESIZE];        /**< Name of the lpm. */
-	uint32_t max_rules; /**< Max. balanced rules per lpm. */
-	uint32_t number_tbl8s; /**< Number of tbl8s. */
-	struct rte_lpm_rule_info rule_info[RTE_LPM_MAX_DEPTH]; /**< Rule info table. */
-
 	/* LPM Tables. */
 	struct rte_lpm_tbl_entry tbl24[RTE_LPM_TBL24_NUM_ENTRIES]
 			__rte_cache_aligned; /**< LPM tbl24 table. */
 	struct rte_lpm_tbl_entry *tbl8; /**< LPM tbl8 table. */
-	struct rte_lpm_rule *rules_tbl; /**< LPM rules. */
 };
 
 /** LPM RCU QSBR configuration structure. */
-- 
2.20.1

Re: [dpdk-dev] [PATCH v2 2/2] lpm: hide internal data

[Thomas Monjalon]

21/10/2020 05:02, Ruifeng Wang:
> --- a/doc/guides/rel_notes/release_20_11.rst
> +++ b/doc/guides/rel_notes/release_20_11.rst
> @@ -602,6 +602,8 @@ ABI Changes
>  
>  * sched: Added new fields to ``struct rte_sched_subport_port_params``.
>  
> +* lpm: Removed fields other than ``tbl24`` and ``tbl8`` from the struct ``rte_lpm``.
> +  The removed fields were made internal.
>  
>  Known Issues
>  ------------

Can be changed on apply, but please note when adding a new paragraph,
that you should add a new blank line, keeping 2 blank lines
before the next section as it was before your patch.

PS: having this kind of minor comment means you are a regular
contributor, so we expect perfect patches :)

Thanks

Re: [dpdk-dev] [PATCH v2 2/2] lpm: hide internal data

[Ruifeng Wang]

> -----Original Message-----
> From: Thomas Monjalon <thomas@monjalon.net>
> Sent: Wednesday, October 21, 2020 3:59 PM
> To: Ruifeng Wang <Ruifeng.Wang@arm.com>
> Cc: Bruce Richardson <bruce.richardson@intel.com>; Vladimir Medvedkin
> <vladimir.medvedkin@intel.com>; dev@dpdk.org; Honnappa Nagarahalli
> <Honnappa.Nagarahalli@arm.com>; nd <nd@arm.com>; David Marchand
> <david.marchand@redhat.com>; Kevin Traynor <ktraynor@redhat.com>
> Subject: Re: [PATCH v2 2/2] lpm: hide internal data
> 
> 21/10/2020 05:02, Ruifeng Wang:
> > --- a/doc/guides/rel_notes/release_20_11.rst
> > +++ b/doc/guides/rel_notes/release_20_11.rst
> > @@ -602,6 +602,8 @@ ABI Changes
> >
> >  * sched: Added new fields to ``struct rte_sched_subport_port_params``.
> >
> > +* lpm: Removed fields other than ``tbl24`` and ``tbl8`` from the struct
> ``rte_lpm``.
> > +  The removed fields were made internal.
> >
> >  Known Issues
> >  ------------
> 
> Can be changed on apply, but please note when adding a new paragraph,
> that you should add a new blank line, keeping 2 blank lines before the next
> section as it was before your patch.
> 
> PS: having this kind of minor comment means you are a regular contributor,
> so we expect perfect patches :)
> 
Sorry for the extra burden added to merge. Will pay more attention on format and other details.

Thanks.
> Thanks
> 

Re: [dpdk-dev] [PATCH v2 2/2] lpm: hide internal data

[David Marchand]

On Wed, Oct 21, 2020 at 5:02 AM Ruifeng Wang <ruifeng.wang@arm.com> wrote:
> diff --git a/lib/librte_lpm/rte_lpm.c b/lib/librte_lpm/rte_lpm.c
> index 51a0ae578..88d31df6d 100644
> --- a/lib/librte_lpm/rte_lpm.c
> +++ b/lib/librte_lpm/rte_lpm.c
> @@ -42,9 +42,17 @@ enum valid_flag {
>
>  /** @internal LPM structure. */
>  struct __rte_lpm {
> -       /* LPM metadata. */
> +       /* Exposed LPM data. */
>         struct rte_lpm lpm;
>
> +       /* LPM metadata. */
> +       char name[RTE_LPM_NAMESIZE];        /**< Name of the lpm. */
> +       uint32_t max_rules; /**< Max. balanced rules per lpm. */
> +       uint32_t number_tbl8s; /**< Number of tbl8s. */
> +       /**< Rule info table. */
> +       struct rte_lpm_rule_info rule_info[RTE_LPM_MAX_DEPTH];
> +       struct rte_lpm_rule *rules_tbl; /**< LPM rules. */

- We hide the rules, is there a reason to keep struct
rte_lpm_rule_info and struct rte_lpm_rule exposed?

- Rather than have translations lpm -> i_lpm, in many places of this
library, we should translate only in the functions exposed to the
user.
Besides, it is a bit hard to read between internal_lpm and i_lpm, I
would adopt a single i_lpm convention for the whole file.
I went and tried to do it (big search and replace + build tests, no
runtime check though).
This results in:
https://github.com/david-marchand/dpdk/commit/4e61f0ce7cf2ac472565d3c6aa5bb78ffb8f70c9

What do you think?


-- 
David Marchand

Re: [dpdk-dev] [PATCH v2 2/2] lpm: hide internal data

[Ruifeng Wang]

> -----Original Message-----
> From: David Marchand <david.marchand@redhat.com>
> Sent: Thursday, October 22, 2020 11:14 PM
> To: Ruifeng Wang <Ruifeng.Wang@arm.com>
> Cc: Bruce Richardson <bruce.richardson@intel.com>; Vladimir Medvedkin
> <vladimir.medvedkin@intel.com>; dev <dev@dpdk.org>; Honnappa
> Nagarahalli <Honnappa.Nagarahalli@arm.com>; nd <nd@arm.com>; Kevin
> Traynor <ktraynor@redhat.com>; thomas@monjalon.net
> Subject: Re: [PATCH v2 2/2] lpm: hide internal data
> 
> On Wed, Oct 21, 2020 at 5:02 AM Ruifeng Wang <ruifeng.wang@arm.com>
> wrote:
> > diff --git a/lib/librte_lpm/rte_lpm.c b/lib/librte_lpm/rte_lpm.c index
> > 51a0ae578..88d31df6d 100644
> > --- a/lib/librte_lpm/rte_lpm.c
> > +++ b/lib/librte_lpm/rte_lpm.c
> > @@ -42,9 +42,17 @@ enum valid_flag {
> >
> >  /** @internal LPM structure. */
> >  struct __rte_lpm {
> > -       /* LPM metadata. */
> > +       /* Exposed LPM data. */
> >         struct rte_lpm lpm;
> >
> > +       /* LPM metadata. */
> > +       char name[RTE_LPM_NAMESIZE];        /**< Name of the lpm. */
> > +       uint32_t max_rules; /**< Max. balanced rules per lpm. */
> > +       uint32_t number_tbl8s; /**< Number of tbl8s. */
> > +       /**< Rule info table. */
> > +       struct rte_lpm_rule_info rule_info[RTE_LPM_MAX_DEPTH];
> > +       struct rte_lpm_rule *rules_tbl; /**< LPM rules. */
> 
> - We hide the rules, is there a reason to keep struct rte_lpm_rule_info and
> struct rte_lpm_rule exposed?
> 
These structs can be moved into rte_lpm.c and made internal too.

> - Rather than have translations lpm -> i_lpm, in many places of this library, we
> should translate only in the functions exposed to the user.
> Besides, it is a bit hard to read between internal_lpm and i_lpm, I would
> adopt a single i_lpm convention for the whole file.
> I went and tried to do it (big search and replace + build tests, no runtime
> check though).
> This results in:
> https://github.com/david-
> marchand/dpdk/commit/4e61f0ce7cf2ac472565d3c6aa5bb78ffb8f70c9
> 
> What do you think?
> 
I'm fine with the change. It looks good.
I checked unit test is passing.

Thanks.
/Ruifeng
> 
> --
> David Marchand

Re: [dpdk-dev] [PATCH v2 2/2] lpm: hide internal data

[Medvedkin, Vladimir]

Hello,

On 23/10/2020 07:13, Ruifeng Wang wrote:
> 
>> -----Original Message-----
>> From: David Marchand <david.marchand@redhat.com>
>> Sent: Thursday, October 22, 2020 11:14 PM
>> To: Ruifeng Wang <Ruifeng.Wang@arm.com>
>> Cc: Bruce Richardson <bruce.richardson@intel.com>; Vladimir Medvedkin
>> <vladimir.medvedkin@intel.com>; dev <dev@dpdk.org>; Honnappa
>> Nagarahalli <Honnappa.Nagarahalli@arm.com>; nd <nd@arm.com>; Kevin
>> Traynor <ktraynor@redhat.com>; thomas@monjalon.net
>> Subject: Re: [PATCH v2 2/2] lpm: hide internal data
>>
>> On Wed, Oct 21, 2020 at 5:02 AM Ruifeng Wang <ruifeng.wang@arm.com>
>> wrote:
>>> diff --git a/lib/librte_lpm/rte_lpm.c b/lib/librte_lpm/rte_lpm.c index
>>> 51a0ae578..88d31df6d 100644
>>> --- a/lib/librte_lpm/rte_lpm.c
>>> +++ b/lib/librte_lpm/rte_lpm.c
>>> @@ -42,9 +42,17 @@ enum valid_flag {
>>>
>>>   /** @internal LPM structure. */
>>>   struct __rte_lpm {
>>> -       /* LPM metadata. */
>>> +       /* Exposed LPM data. */
>>>          struct rte_lpm lpm;
>>>
>>> +       /* LPM metadata. */
>>> +       char name[RTE_LPM_NAMESIZE];        /**< Name of the lpm. */
>>> +       uint32_t max_rules; /**< Max. balanced rules per lpm. */
>>> +       uint32_t number_tbl8s; /**< Number of tbl8s. */
>>> +       /**< Rule info table. */
>>> +       struct rte_lpm_rule_info rule_info[RTE_LPM_MAX_DEPTH];
>>> +       struct rte_lpm_rule *rules_tbl; /**< LPM rules. */
>>
>> - We hide the rules, is there a reason to keep struct rte_lpm_rule_info and
>> struct rte_lpm_rule exposed?
>>
> These structs can be moved into rte_lpm.c and made internal too.

Agree, these structs are control plane related and could be moved from 
public struct.

> 
>> - Rather than have translations lpm -> i_lpm, in many places of this library, we
>> should translate only in the functions exposed to the user.
>> Besides, it is a bit hard to read between internal_lpm and i_lpm, I would
>> adopt a single i_lpm convention for the whole file.
>> I went and tried to do it (big search and replace + build tests, no runtime
>> check though).
>> This results in:
>> https://github.com/david-
>> marchand/dpdk/commit/4e61f0ce7cf2ac472565d3c6aa5bb78ffb8f70c9
>>
>> What do you think?
>>
> I'm fine with the change. It looks good.
> I checked unit test is passing.
> 

Also checked with testfib app with "-c -a" args, FIB and LPM lookup 
returns same values.

> Thanks.
> /Ruifeng
>>
>> --
>> David Marchand
> 

-- 
Regards,
Vladimir

[dpdk-dev] [PATCH v3 0/2] LPM changes

[David Marchand]

From Ruifeng Wang:

The rte_lpm structure is exported because lookup API is inlined.
But most of the structure can be hidden.
Discussion at: http://patches.dpdk.org/patch/72403/
This patch set aimed to hide the rte_lpm structure as much as possible.

A data free issue was identified and fixed.

Changelog since v2:
- hid now internal structures,
- changed code so that everything is done on internal representation of
  the lpm object and translation only happens at public API boundaries,

-- 
David Marchand

Ruifeng Wang (2):
  lpm: fix free of data structure
  lpm: hide internal data

 doc/guides/rel_notes/release_20_11.rst |   3 +
 lib/librte_lpm/rte_lpm.c               | 388 +++++++++++++------------
 lib/librte_lpm/rte_lpm.h               |  19 --
 3 files changed, 208 insertions(+), 202 deletions(-)

-- 
2.23.0

[dpdk-dev] [PATCH v3 1/2] lpm: fix free of data structure

[David Marchand]

From: Ruifeng Wang <ruifeng.wang@arm.com>

The container structure should be freed instead of rte_lpm structure
after wrapping rte_lpm into internal structure __rte_lpm.

Fixes: 8a9f8564e9f9 ("lpm: implement RCU rule reclamation")

Signed-off-by: Ruifeng Wang <ruifeng.wang@arm.com>
Reviewed-by: Phil Yang <phil.yang@arm.com>
Acked-by: Bruce Richardson <bruce.richardson@intel.com>
Acked-by: Vladimir Medvedkin <vladimir.medvedkin@intel.com>
Acked-by: Kevin Traynor <ktraynor@redhat.com>
---
 lib/librte_lpm/rte_lpm.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/lib/librte_lpm/rte_lpm.c b/lib/librte_lpm/rte_lpm.c
index 757436f492..51a0ae5780 100644
--- a/lib/librte_lpm/rte_lpm.c
+++ b/lib/librte_lpm/rte_lpm.c
@@ -268,7 +268,7 @@ rte_lpm_free(struct rte_lpm *lpm)
 		rte_rcu_qsbr_dq_delete(internal_lpm->dq);
 	rte_free(lpm->tbl8);
 	rte_free(lpm->rules_tbl);
-	rte_free(lpm);
+	rte_free(internal_lpm);
 	rte_free(te);
 }
 
-- 
2.23.0

[dpdk-dev] [PATCH v3 2/2] lpm: hide internal data

[David Marchand]

From: Ruifeng Wang <ruifeng.wang@arm.com>

Fields except tbl24 and tbl8 in rte_lpm structure have no
need to be exposed to the user.
Hide the unneeded exposure of structure fields for better
ABI maintainability.

Suggested-by: David Marchand <david.marchand@redhat.com>
Signed-off-by: Ruifeng Wang <ruifeng.wang@arm.com>
Reviewed-by: Honnappa Nagarahalli <honnappa.nagarahalli@arm.com>
Acked-by: Kevin Traynor <ktraynor@redhat.com>
Acked-by: Thomas Monjalon <thomas@monjalon.net>
Acked-by: Vladimir Medvedkin <vladimir.medvedkin@intel.com>
Signed-off-by: David Marchand <david.marchand@redhat.com>
---
Changes since v2:
- hid rte_lpm_rule and rte_lpm_rule_info,
- used i_lpm as the preferred variable name,
- moved lpm <-> i_lpm at public API boundaries, all internal functions
  deal with __rte_lpm object,

---
 doc/guides/rel_notes/release_20_11.rst |   3 +
 lib/librte_lpm/rte_lpm.c               | 388 +++++++++++++------------
 lib/librte_lpm/rte_lpm.h               |  19 --
 3 files changed, 208 insertions(+), 202 deletions(-)

diff --git a/doc/guides/rel_notes/release_20_11.rst b/doc/guides/rel_notes/release_20_11.rst
index d8ac359e51..dca8d41eb6 100644
--- a/doc/guides/rel_notes/release_20_11.rst
+++ b/doc/guides/rel_notes/release_20_11.rst
@@ -606,6 +606,9 @@ ABI Changes
 
 * sched: Added new fields to ``struct rte_sched_subport_port_params``.
 
+* lpm: Removed fields other than ``tbl24`` and ``tbl8`` from the struct
+  ``rte_lpm``. The removed fields were made internal.
+
 
 Known Issues
 ------------
diff --git a/lib/librte_lpm/rte_lpm.c b/lib/librte_lpm/rte_lpm.c
index 51a0ae5780..002811f4de 100644
--- a/lib/librte_lpm/rte_lpm.c
+++ b/lib/librte_lpm/rte_lpm.c
@@ -40,11 +40,31 @@ enum valid_flag {
 	VALID
 };
 
+/** @internal Rule structure. */
+struct rte_lpm_rule {
+	uint32_t ip; /**< Rule IP address. */
+	uint32_t next_hop; /**< Rule next hop. */
+};
+
+/** @internal Contains metadata about the rules table. */
+struct rte_lpm_rule_info {
+	uint32_t used_rules; /**< Used rules so far. */
+	uint32_t first_rule; /**< Indexes the first rule of a given depth. */
+};
+
 /** @internal LPM structure. */
 struct __rte_lpm {
-	/* LPM metadata. */
+	/* Exposed LPM data. */
 	struct rte_lpm lpm;
 
+	/* LPM metadata. */
+	char name[RTE_LPM_NAMESIZE];        /**< Name of the lpm. */
+	uint32_t max_rules; /**< Max. balanced rules per lpm. */
+	uint32_t number_tbl8s; /**< Number of tbl8s. */
+	/**< Rule info table. */
+	struct rte_lpm_rule_info rule_info[RTE_LPM_MAX_DEPTH];
+	struct rte_lpm_rule *rules_tbl; /**< LPM rules. */
+
 	/* RCU config. */
 	struct rte_rcu_qsbr *v;		/* RCU QSBR variable. */
 	enum rte_lpm_qsbr_mode rcu_mode;/* Blocking, defer queue. */
@@ -104,7 +124,7 @@ depth_to_range(uint8_t depth)
 struct rte_lpm *
 rte_lpm_find_existing(const char *name)
 {
-	struct rte_lpm *l = NULL;
+	struct __rte_lpm *i_lpm = NULL;
 	struct rte_tailq_entry *te;
 	struct rte_lpm_list *lpm_list;
 
@@ -112,8 +132,8 @@ rte_lpm_find_existing(const char *name)
 
 	rte_mcfg_tailq_read_lock();
 	TAILQ_FOREACH(te, lpm_list, next) {
-		l = te->data;
-		if (strncmp(name, l->name, RTE_LPM_NAMESIZE) == 0)
+		i_lpm = te->data;
+		if (strncmp(name, i_lpm->name, RTE_LPM_NAMESIZE) == 0)
 			break;
 	}
 	rte_mcfg_tailq_read_unlock();
@@ -123,7 +143,7 @@ rte_lpm_find_existing(const char *name)
 		return NULL;
 	}
 
-	return l;
+	return &i_lpm->lpm;
 }
 
 /*
@@ -134,7 +154,7 @@ rte_lpm_create(const char *name, int socket_id,
 		const struct rte_lpm_config *config)
 {
 	char mem_name[RTE_LPM_NAMESIZE];
-	struct __rte_lpm *internal_lpm;
+	struct __rte_lpm *i_lpm;
 	struct rte_lpm *lpm = NULL;
 	struct rte_tailq_entry *te;
 	uint32_t mem_size, rules_size, tbl8s_size;
@@ -157,19 +177,18 @@ rte_lpm_create(const char *name, int socket_id,
 
 	/* guarantee there's no existing */
 	TAILQ_FOREACH(te, lpm_list, next) {
-		lpm = te->data;
-		if (strncmp(name, lpm->name, RTE_LPM_NAMESIZE) == 0)
+		i_lpm = te->data;
+		if (strncmp(name, i_lpm->name, RTE_LPM_NAMESIZE) == 0)
 			break;
 	}
 
 	if (te != NULL) {
-		lpm = NULL;
 		rte_errno = EEXIST;
 		goto exit;
 	}
 
 	/* Determine the amount of memory to allocate. */
-	mem_size = sizeof(*internal_lpm);
+	mem_size = sizeof(*i_lpm);
 	rules_size = sizeof(struct rte_lpm_rule) * config->max_rules;
 	tbl8s_size = sizeof(struct rte_lpm_tbl_entry) *
 			RTE_LPM_TBL8_GROUP_NUM_ENTRIES * config->number_tbl8s;
@@ -183,49 +202,47 @@ rte_lpm_create(const char *name, int socket_id,
 	}
 
 	/* Allocate memory to store the LPM data structures. */
-	internal_lpm = rte_zmalloc_socket(mem_name, mem_size,
+	i_lpm = rte_zmalloc_socket(mem_name, mem_size,
 			RTE_CACHE_LINE_SIZE, socket_id);
-	if (internal_lpm == NULL) {
+	if (i_lpm == NULL) {
 		RTE_LOG(ERR, LPM, "LPM memory allocation failed\n");
 		rte_free(te);
 		rte_errno = ENOMEM;
 		goto exit;
 	}
 
-	lpm = &internal_lpm->lpm;
-	lpm->rules_tbl = rte_zmalloc_socket(NULL,
+	i_lpm->rules_tbl = rte_zmalloc_socket(NULL,
 			(size_t)rules_size, RTE_CACHE_LINE_SIZE, socket_id);
 
-	if (lpm->rules_tbl == NULL) {
+	if (i_lpm->rules_tbl == NULL) {
 		RTE_LOG(ERR, LPM, "LPM rules_tbl memory allocation failed\n");
-		rte_free(internal_lpm);
-		internal_lpm = NULL;
-		lpm = NULL;
+		rte_free(i_lpm);
+		i_lpm = NULL;
 		rte_free(te);
 		rte_errno = ENOMEM;
 		goto exit;
 	}
 
-	lpm->tbl8 = rte_zmalloc_socket(NULL,
+	i_lpm->lpm.tbl8 = rte_zmalloc_socket(NULL,
 			(size_t)tbl8s_size, RTE_CACHE_LINE_SIZE, socket_id);
 
-	if (lpm->tbl8 == NULL) {
+	if (i_lpm->lpm.tbl8 == NULL) {
 		RTE_LOG(ERR, LPM, "LPM tbl8 memory allocation failed\n");
-		rte_free(lpm->rules_tbl);
-		rte_free(internal_lpm);
-		internal_lpm = NULL;
-		lpm = NULL;
+		rte_free(i_lpm->rules_tbl);
+		rte_free(i_lpm);
+		i_lpm = NULL;
 		rte_free(te);
 		rte_errno = ENOMEM;
 		goto exit;
 	}
 
 	/* Save user arguments. */
-	lpm->max_rules = config->max_rules;
-	lpm->number_tbl8s = config->number_tbl8s;
-	strlcpy(lpm->name, name, sizeof(lpm->name));
+	i_lpm->max_rules = config->max_rules;
+	i_lpm->number_tbl8s = config->number_tbl8s;
+	strlcpy(i_lpm->name, name, sizeof(i_lpm->name));
 
-	te->data = lpm;
+	te->data = i_lpm;
+	lpm = &i_lpm->lpm;
 
 	TAILQ_INSERT_TAIL(lpm_list, te, next);
 
@@ -241,13 +258,14 @@ rte_lpm_create(const char *name, int socket_id,
 void
 rte_lpm_free(struct rte_lpm *lpm)
 {
-	struct __rte_lpm *internal_lpm;
 	struct rte_lpm_list *lpm_list;
 	struct rte_tailq_entry *te;
+	struct __rte_lpm *i_lpm;
 
 	/* Check user arguments. */
 	if (lpm == NULL)
 		return;
+	i_lpm = container_of(lpm, struct __rte_lpm, lpm);
 
 	lpm_list = RTE_TAILQ_CAST(rte_lpm_tailq.head, rte_lpm_list);
 
@@ -255,7 +273,7 @@ rte_lpm_free(struct rte_lpm *lpm)
 
 	/* find our tailq entry */
 	TAILQ_FOREACH(te, lpm_list, next) {
-		if (te->data == (void *) lpm)
+		if (te->data == (void *)i_lpm)
 			break;
 	}
 	if (te != NULL)
@@ -263,19 +281,18 @@ rte_lpm_free(struct rte_lpm *lpm)
 
 	rte_mcfg_tailq_write_unlock();
 
-	internal_lpm = container_of(lpm, struct __rte_lpm, lpm);
-	if (internal_lpm->dq != NULL)
-		rte_rcu_qsbr_dq_delete(internal_lpm->dq);
-	rte_free(lpm->tbl8);
-	rte_free(lpm->rules_tbl);
-	rte_free(internal_lpm);
+	if (i_lpm->dq != NULL)
+		rte_rcu_qsbr_dq_delete(i_lpm->dq);
+	rte_free(i_lpm->lpm.tbl8);
+	rte_free(i_lpm->rules_tbl);
+	rte_free(i_lpm);
 	rte_free(te);
 }
 
 static void
 __lpm_rcu_qsbr_free_resource(void *p, void *data, unsigned int n)
 {
-	struct rte_lpm_tbl_entry *tbl8 = ((struct rte_lpm *)p)->tbl8;
+	struct rte_lpm_tbl_entry *tbl8 = ((struct __rte_lpm *)p)->lpm.tbl8;
 	struct rte_lpm_tbl_entry zero_tbl8_entry = {0};
 	uint32_t tbl8_group_index = *(uint32_t *)data;
 
@@ -292,15 +309,15 @@ rte_lpm_rcu_qsbr_add(struct rte_lpm *lpm, struct rte_lpm_rcu_config *cfg)
 {
 	struct rte_rcu_qsbr_dq_parameters params = {0};
 	char rcu_dq_name[RTE_RCU_QSBR_DQ_NAMESIZE];
-	struct __rte_lpm *internal_lpm;
+	struct __rte_lpm *i_lpm;
 
 	if (lpm == NULL || cfg == NULL) {
 		rte_errno = EINVAL;
 		return 1;
 	}
 
-	internal_lpm = container_of(lpm, struct __rte_lpm, lpm);
-	if (internal_lpm->v != NULL) {
+	i_lpm = container_of(lpm, struct __rte_lpm, lpm);
+	if (i_lpm->v != NULL) {
 		rte_errno = EEXIST;
 		return 1;
 	}
@@ -310,21 +327,21 @@ rte_lpm_rcu_qsbr_add(struct rte_lpm *lpm, struct rte_lpm_rcu_config *cfg)
 	} else if (cfg->mode == RTE_LPM_QSBR_MODE_DQ) {
 		/* Init QSBR defer queue. */
 		snprintf(rcu_dq_name, sizeof(rcu_dq_name),
-				"LPM_RCU_%s", lpm->name);
+				"LPM_RCU_%s", i_lpm->name);
 		params.name = rcu_dq_name;
 		params.size = cfg->dq_size;
 		if (params.size == 0)
-			params.size = lpm->number_tbl8s;
+			params.size = i_lpm->number_tbl8s;
 		params.trigger_reclaim_limit = cfg->reclaim_thd;
 		params.max_reclaim_size = cfg->reclaim_max;
 		if (params.max_reclaim_size == 0)
 			params.max_reclaim_size = RTE_LPM_RCU_DQ_RECLAIM_MAX;
 		params.esize = sizeof(uint32_t);	/* tbl8 group index */
 		params.free_fn = __lpm_rcu_qsbr_free_resource;
-		params.p = lpm;
+		params.p = i_lpm;
 		params.v = cfg->v;
-		internal_lpm->dq = rte_rcu_qsbr_dq_create(&params);
-		if (internal_lpm->dq == NULL) {
+		i_lpm->dq = rte_rcu_qsbr_dq_create(&params);
+		if (i_lpm->dq == NULL) {
 			RTE_LOG(ERR, LPM, "LPM defer queue creation failed\n");
 			return 1;
 		}
@@ -332,8 +349,8 @@ rte_lpm_rcu_qsbr_add(struct rte_lpm *lpm, struct rte_lpm_rcu_config *cfg)
 		rte_errno = EINVAL;
 		return 1;
 	}
-	internal_lpm->rcu_mode = cfg->mode;
-	internal_lpm->v = cfg->v;
+	i_lpm->rcu_mode = cfg->mode;
+	i_lpm->v = cfg->v;
 
 	return 0;
 }
@@ -349,7 +366,7 @@ rte_lpm_rcu_qsbr_add(struct rte_lpm *lpm, struct rte_lpm_rcu_config *cfg)
  * NOTE: Valid range for depth parameter is 1 .. 32 inclusive.
  */
 static int32_t
-rule_add(struct rte_lpm *lpm, uint32_t ip_masked, uint8_t depth,
+rule_add(struct __rte_lpm *i_lpm, uint32_t ip_masked, uint8_t depth,
 	uint32_t next_hop)
 {
 	uint32_t rule_gindex, rule_index, last_rule;
@@ -358,68 +375,68 @@ rule_add(struct rte_lpm *lpm, uint32_t ip_masked, uint8_t depth,
 	VERIFY_DEPTH(depth);
 
 	/* Scan through rule group to see if rule already exists. */
-	if (lpm->rule_info[depth - 1].used_rules > 0) {
+	if (i_lpm->rule_info[depth - 1].used_rules > 0) {
 
 		/* rule_gindex stands for rule group index. */
-		rule_gindex = lpm->rule_info[depth - 1].first_rule;
+		rule_gindex = i_lpm->rule_info[depth - 1].first_rule;
 		/* Initialise rule_index to point to start of rule group. */
 		rule_index = rule_gindex;
 		/* Last rule = Last used rule in this rule group. */
-		last_rule = rule_gindex + lpm->rule_info[depth - 1].used_rules;
+		last_rule = rule_gindex + i_lpm->rule_info[depth - 1].used_rules;
 
 		for (; rule_index < last_rule; rule_index++) {
 
 			/* If rule already exists update next hop and return. */
-			if (lpm->rules_tbl[rule_index].ip == ip_masked) {
+			if (i_lpm->rules_tbl[rule_index].ip == ip_masked) {
 
-				if (lpm->rules_tbl[rule_index].next_hop
+				if (i_lpm->rules_tbl[rule_index].next_hop
 						== next_hop)
 					return -EEXIST;
-				lpm->rules_tbl[rule_index].next_hop = next_hop;
+				i_lpm->rules_tbl[rule_index].next_hop = next_hop;
 
 				return rule_index;
 			}
 		}
 
-		if (rule_index == lpm->max_rules)
+		if (rule_index == i_lpm->max_rules)
 			return -ENOSPC;
 	} else {
 		/* Calculate the position in which the rule will be stored. */
 		rule_index = 0;
 
 		for (i = depth - 1; i > 0; i--) {
-			if (lpm->rule_info[i - 1].used_rules > 0) {
-				rule_index = lpm->rule_info[i - 1].first_rule
-						+ lpm->rule_info[i - 1].used_rules;
+			if (i_lpm->rule_info[i - 1].used_rules > 0) {
+				rule_index = i_lpm->rule_info[i - 1].first_rule
+						+ i_lpm->rule_info[i - 1].used_rules;
 				break;
 			}
 		}
-		if (rule_index == lpm->max_rules)
+		if (rule_index == i_lpm->max_rules)
 			return -ENOSPC;
 
-		lpm->rule_info[depth - 1].first_rule = rule_index;
+		i_lpm->rule_info[depth - 1].first_rule = rule_index;
 	}
 
 	/* Make room for the new rule in the array. */
 	for (i = RTE_LPM_MAX_DEPTH; i > depth; i--) {
-		if (lpm->rule_info[i - 1].first_rule
-				+ lpm->rule_info[i - 1].used_rules == lpm->max_rules)
+		if (i_lpm->rule_info[i - 1].first_rule
+				+ i_lpm->rule_info[i - 1].used_rules == i_lpm->max_rules)
 			return -ENOSPC;
 
-		if (lpm->rule_info[i - 1].used_rules > 0) {
-			lpm->rules_tbl[lpm->rule_info[i - 1].first_rule
-				+ lpm->rule_info[i - 1].used_rules]
-					= lpm->rules_tbl[lpm->rule_info[i - 1].first_rule];
-			lpm->rule_info[i - 1].first_rule++;
+		if (i_lpm->rule_info[i - 1].used_rules > 0) {
+			i_lpm->rules_tbl[i_lpm->rule_info[i - 1].first_rule
+				+ i_lpm->rule_info[i - 1].used_rules]
+					= i_lpm->rules_tbl[i_lpm->rule_info[i - 1].first_rule];
+			i_lpm->rule_info[i - 1].first_rule++;
 		}
 	}
 
 	/* Add the new rule. */
-	lpm->rules_tbl[rule_index].ip = ip_masked;
-	lpm->rules_tbl[rule_index].next_hop = next_hop;
+	i_lpm->rules_tbl[rule_index].ip = ip_masked;
+	i_lpm->rules_tbl[rule_index].next_hop = next_hop;
 
 	/* Increment the used rules counter for this rule group. */
-	lpm->rule_info[depth - 1].used_rules++;
+	i_lpm->rule_info[depth - 1].used_rules++;
 
 	return rule_index;
 }
@@ -429,26 +446,26 @@ rule_add(struct rte_lpm *lpm, uint32_t ip_masked, uint8_t depth,
  * NOTE: Valid range for depth parameter is 1 .. 32 inclusive.
  */
 static void
-rule_delete(struct rte_lpm *lpm, int32_t rule_index, uint8_t depth)
+rule_delete(struct __rte_lpm *i_lpm, int32_t rule_index, uint8_t depth)
 {
 	int i;
 
 	VERIFY_DEPTH(depth);
 
-	lpm->rules_tbl[rule_index] =
-			lpm->rules_tbl[lpm->rule_info[depth - 1].first_rule
-			+ lpm->rule_info[depth - 1].used_rules - 1];
+	i_lpm->rules_tbl[rule_index] =
+			i_lpm->rules_tbl[i_lpm->rule_info[depth - 1].first_rule
+			+ i_lpm->rule_info[depth - 1].used_rules - 1];
 
 	for (i = depth; i < RTE_LPM_MAX_DEPTH; i++) {
-		if (lpm->rule_info[i].used_rules > 0) {
-			lpm->rules_tbl[lpm->rule_info[i].first_rule - 1] =
-					lpm->rules_tbl[lpm->rule_info[i].first_rule
-						+ lpm->rule_info[i].used_rules - 1];
-			lpm->rule_info[i].first_rule--;
+		if (i_lpm->rule_info[i].used_rules > 0) {
+			i_lpm->rules_tbl[i_lpm->rule_info[i].first_rule - 1] =
+					i_lpm->rules_tbl[i_lpm->rule_info[i].first_rule
+						+ i_lpm->rule_info[i].used_rules - 1];
+			i_lpm->rule_info[i].first_rule--;
 		}
 	}
 
-	lpm->rule_info[depth - 1].used_rules--;
+	i_lpm->rule_info[depth - 1].used_rules--;
 }
 
 /*
@@ -456,19 +473,19 @@ rule_delete(struct rte_lpm *lpm, int32_t rule_index, uint8_t depth)
  * NOTE: Valid range for depth parameter is 1 .. 32 inclusive.
  */
 static int32_t
-rule_find(struct rte_lpm *lpm, uint32_t ip_masked, uint8_t depth)
+rule_find(struct __rte_lpm *i_lpm, uint32_t ip_masked, uint8_t depth)
 {
 	uint32_t rule_gindex, last_rule, rule_index;
 
 	VERIFY_DEPTH(depth);
 
-	rule_gindex = lpm->rule_info[depth - 1].first_rule;
-	last_rule = rule_gindex + lpm->rule_info[depth - 1].used_rules;
+	rule_gindex = i_lpm->rule_info[depth - 1].first_rule;
+	last_rule = rule_gindex + i_lpm->rule_info[depth - 1].used_rules;
 
 	/* Scan used rules at given depth to find rule. */
 	for (rule_index = rule_gindex; rule_index < last_rule; rule_index++) {
 		/* If rule is found return the rule index. */
-		if (lpm->rules_tbl[rule_index].ip == ip_masked)
+		if (i_lpm->rules_tbl[rule_index].ip == ip_masked)
 			return rule_index;
 	}
 
@@ -480,14 +497,14 @@ rule_find(struct rte_lpm *lpm, uint32_t ip_masked, uint8_t depth)
  * Find, clean and allocate a tbl8.
  */
 static int32_t
-_tbl8_alloc(struct rte_lpm *lpm)
+_tbl8_alloc(struct __rte_lpm *i_lpm)
 {
 	uint32_t group_idx; /* tbl8 group index. */
 	struct rte_lpm_tbl_entry *tbl8_entry;
 
 	/* Scan through tbl8 to find a free (i.e. INVALID) tbl8 group. */
-	for (group_idx = 0; group_idx < lpm->number_tbl8s; group_idx++) {
-		tbl8_entry = &lpm->tbl8[group_idx *
+	for (group_idx = 0; group_idx < i_lpm->number_tbl8s; group_idx++) {
+		tbl8_entry = &i_lpm->lpm.tbl8[group_idx *
 					RTE_LPM_TBL8_GROUP_NUM_ENTRIES];
 		/* If a free tbl8 group is found clean it and set as VALID. */
 		if (!tbl8_entry->valid_group) {
@@ -515,45 +532,41 @@ _tbl8_alloc(struct rte_lpm *lpm)
 }
 
 static int32_t
-tbl8_alloc(struct rte_lpm *lpm)
+tbl8_alloc(struct __rte_lpm *i_lpm)
 {
 	int32_t group_idx; /* tbl8 group index. */
-	struct __rte_lpm *internal_lpm;
 
-	internal_lpm = container_of(lpm, struct __rte_lpm, lpm);
-	group_idx = _tbl8_alloc(lpm);
-	if (group_idx == -ENOSPC && internal_lpm->dq != NULL) {
+	group_idx = _tbl8_alloc(i_lpm);
+	if (group_idx == -ENOSPC && i_lpm->dq != NULL) {
 		/* If there are no tbl8 groups try to reclaim one. */
-		if (rte_rcu_qsbr_dq_reclaim(internal_lpm->dq, 1,
+		if (rte_rcu_qsbr_dq_reclaim(i_lpm->dq, 1,
 				NULL, NULL, NULL) == 0)
-			group_idx = _tbl8_alloc(lpm);
+			group_idx = _tbl8_alloc(i_lpm);
 	}
 
 	return group_idx;
 }
 
 static int32_t
-tbl8_free(struct rte_lpm *lpm, uint32_t tbl8_group_start)
+tbl8_free(struct __rte_lpm *i_lpm, uint32_t tbl8_group_start)
 {
 	struct rte_lpm_tbl_entry zero_tbl8_entry = {0};
-	struct __rte_lpm *internal_lpm;
 	int status;
 
-	internal_lpm = container_of(lpm, struct __rte_lpm, lpm);
-	if (internal_lpm->v == NULL) {
+	if (i_lpm->v == NULL) {
 		/* Set tbl8 group invalid*/
-		__atomic_store(&lpm->tbl8[tbl8_group_start], &zero_tbl8_entry,
+		__atomic_store(&i_lpm->lpm.tbl8[tbl8_group_start], &zero_tbl8_entry,
 				__ATOMIC_RELAXED);
-	} else if (internal_lpm->rcu_mode == RTE_LPM_QSBR_MODE_SYNC) {
+	} else if (i_lpm->rcu_mode == RTE_LPM_QSBR_MODE_SYNC) {
 		/* Wait for quiescent state change. */
-		rte_rcu_qsbr_synchronize(internal_lpm->v,
+		rte_rcu_qsbr_synchronize(i_lpm->v,
 			RTE_QSBR_THRID_INVALID);
 		/* Set tbl8 group invalid*/
-		__atomic_store(&lpm->tbl8[tbl8_group_start], &zero_tbl8_entry,
+		__atomic_store(&i_lpm->lpm.tbl8[tbl8_group_start], &zero_tbl8_entry,
 				__ATOMIC_RELAXED);
-	} else if (internal_lpm->rcu_mode == RTE_LPM_QSBR_MODE_DQ) {
+	} else if (i_lpm->rcu_mode == RTE_LPM_QSBR_MODE_DQ) {
 		/* Push into QSBR defer queue. */
-		status = rte_rcu_qsbr_dq_enqueue(internal_lpm->dq,
+		status = rte_rcu_qsbr_dq_enqueue(i_lpm->dq,
 				(void *)&tbl8_group_start);
 		if (status == 1) {
 			RTE_LOG(ERR, LPM, "Failed to push QSBR FIFO\n");
@@ -565,7 +578,7 @@ tbl8_free(struct rte_lpm *lpm, uint32_t tbl8_group_start)
 }
 
 static __rte_noinline int32_t
-add_depth_small(struct rte_lpm *lpm, uint32_t ip, uint8_t depth,
+add_depth_small(struct __rte_lpm *i_lpm, uint32_t ip, uint8_t depth,
 		uint32_t next_hop)
 {
 #define group_idx next_hop
@@ -580,8 +593,8 @@ add_depth_small(struct rte_lpm *lpm, uint32_t ip, uint8_t depth,
 		 * For invalid OR valid and non-extended tbl 24 entries set
 		 * entry.
 		 */
-		if (!lpm->tbl24[i].valid || (lpm->tbl24[i].valid_group == 0 &&
-				lpm->tbl24[i].depth <= depth)) {
+		if (!i_lpm->lpm.tbl24[i].valid || (i_lpm->lpm.tbl24[i].valid_group == 0 &&
+				i_lpm->lpm.tbl24[i].depth <= depth)) {
 
 			struct rte_lpm_tbl_entry new_tbl24_entry = {
 				.next_hop = next_hop,
@@ -593,24 +606,24 @@ add_depth_small(struct rte_lpm *lpm, uint32_t ip, uint8_t depth,
 			/* Setting tbl24 entry in one go to avoid race
 			 * conditions
 			 */
-			__atomic_store(&lpm->tbl24[i], &new_tbl24_entry,
+			__atomic_store(&i_lpm->lpm.tbl24[i], &new_tbl24_entry,
 					__ATOMIC_RELEASE);
 
 			continue;
 		}
 
-		if (lpm->tbl24[i].valid_group == 1) {
+		if (i_lpm->lpm.tbl24[i].valid_group == 1) {
 			/* If tbl24 entry is valid and extended calculate the
 			 *  index into tbl8.
 			 */
-			tbl8_index = lpm->tbl24[i].group_idx *
+			tbl8_index = i_lpm->lpm.tbl24[i].group_idx *
 					RTE_LPM_TBL8_GROUP_NUM_ENTRIES;
 			tbl8_group_end = tbl8_index +
 					RTE_LPM_TBL8_GROUP_NUM_ENTRIES;
 
 			for (j = tbl8_index; j < tbl8_group_end; j++) {
-				if (!lpm->tbl8[j].valid ||
-						lpm->tbl8[j].depth <= depth) {
+				if (!i_lpm->lpm.tbl8[j].valid ||
+						i_lpm->lpm.tbl8[j].depth <= depth) {
 					struct rte_lpm_tbl_entry
 						new_tbl8_entry = {
 						.valid = VALID,
@@ -623,7 +636,7 @@ add_depth_small(struct rte_lpm *lpm, uint32_t ip, uint8_t depth,
 					 * Setting tbl8 entry in one go to avoid
 					 * race conditions
 					 */
-					__atomic_store(&lpm->tbl8[j],
+					__atomic_store(&i_lpm->lpm.tbl8[j],
 						&new_tbl8_entry,
 						__ATOMIC_RELAXED);
 
@@ -637,7 +650,7 @@ add_depth_small(struct rte_lpm *lpm, uint32_t ip, uint8_t depth,
 }
 
 static __rte_noinline int32_t
-add_depth_big(struct rte_lpm *lpm, uint32_t ip_masked, uint8_t depth,
+add_depth_big(struct __rte_lpm *i_lpm, uint32_t ip_masked, uint8_t depth,
 		uint32_t next_hop)
 {
 #define group_idx next_hop
@@ -648,9 +661,9 @@ add_depth_big(struct rte_lpm *lpm, uint32_t ip_masked, uint8_t depth,
 	tbl24_index = (ip_masked >> 8);
 	tbl8_range = depth_to_range(depth);
 
-	if (!lpm->tbl24[tbl24_index].valid) {
+	if (!i_lpm->lpm.tbl24[tbl24_index].valid) {
 		/* Search for a free tbl8 group. */
-		tbl8_group_index = tbl8_alloc(lpm);
+		tbl8_group_index = tbl8_alloc(i_lpm);
 
 		/* Check tbl8 allocation was successful. */
 		if (tbl8_group_index < 0) {
@@ -667,10 +680,10 @@ add_depth_big(struct rte_lpm *lpm, uint32_t ip_masked, uint8_t depth,
 			struct rte_lpm_tbl_entry new_tbl8_entry = {
 				.valid = VALID,
 				.depth = depth,
-				.valid_group = lpm->tbl8[i].valid_group,
+				.valid_group = i_lpm->lpm.tbl8[i].valid_group,
 				.next_hop = next_hop,
 			};
-			__atomic_store(&lpm->tbl8[i], &new_tbl8_entry,
+			__atomic_store(&i_lpm->lpm.tbl8[i], &new_tbl8_entry,
 					__ATOMIC_RELAXED);
 		}
 
@@ -690,13 +703,13 @@ add_depth_big(struct rte_lpm *lpm, uint32_t ip_masked, uint8_t depth,
 		/* The tbl24 entry must be written only after the
 		 * tbl8 entries are written.
 		 */
-		__atomic_store(&lpm->tbl24[tbl24_index], &new_tbl24_entry,
+		__atomic_store(&i_lpm->lpm.tbl24[tbl24_index], &new_tbl24_entry,
 				__ATOMIC_RELEASE);
 
 	} /* If valid entry but not extended calculate the index into Table8. */
-	else if (lpm->tbl24[tbl24_index].valid_group == 0) {
+	else if (i_lpm->lpm.tbl24[tbl24_index].valid_group == 0) {
 		/* Search for free tbl8 group. */
-		tbl8_group_index = tbl8_alloc(lpm);
+		tbl8_group_index = tbl8_alloc(i_lpm);
 
 		if (tbl8_group_index < 0) {
 			return tbl8_group_index;
@@ -711,11 +724,11 @@ add_depth_big(struct rte_lpm *lpm, uint32_t ip_masked, uint8_t depth,
 		for (i = tbl8_group_start; i < tbl8_group_end; i++) {
 			struct rte_lpm_tbl_entry new_tbl8_entry = {
 				.valid = VALID,
-				.depth = lpm->tbl24[tbl24_index].depth,
-				.valid_group = lpm->tbl8[i].valid_group,
-				.next_hop = lpm->tbl24[tbl24_index].next_hop,
+				.depth = i_lpm->lpm.tbl24[tbl24_index].depth,
+				.valid_group = i_lpm->lpm.tbl8[i].valid_group,
+				.next_hop = i_lpm->lpm.tbl24[tbl24_index].next_hop,
 			};
-			__atomic_store(&lpm->tbl8[i], &new_tbl8_entry,
+			__atomic_store(&i_lpm->lpm.tbl8[i], &new_tbl8_entry,
 					__ATOMIC_RELAXED);
 		}
 
@@ -726,10 +739,10 @@ add_depth_big(struct rte_lpm *lpm, uint32_t ip_masked, uint8_t depth,
 			struct rte_lpm_tbl_entry new_tbl8_entry = {
 				.valid = VALID,
 				.depth = depth,
-				.valid_group = lpm->tbl8[i].valid_group,
+				.valid_group = i_lpm->lpm.tbl8[i].valid_group,
 				.next_hop = next_hop,
 			};
-			__atomic_store(&lpm->tbl8[i], &new_tbl8_entry,
+			__atomic_store(&i_lpm->lpm.tbl8[i], &new_tbl8_entry,
 					__ATOMIC_RELAXED);
 		}
 
@@ -749,33 +762,33 @@ add_depth_big(struct rte_lpm *lpm, uint32_t ip_masked, uint8_t depth,
 		/* The tbl24 entry must be written only after the
 		 * tbl8 entries are written.
 		 */
-		__atomic_store(&lpm->tbl24[tbl24_index], &new_tbl24_entry,
+		__atomic_store(&i_lpm->lpm.tbl24[tbl24_index], &new_tbl24_entry,
 				__ATOMIC_RELEASE);
 
 	} else { /*
 		* If it is valid, extended entry calculate the index into tbl8.
 		*/
-		tbl8_group_index = lpm->tbl24[tbl24_index].group_idx;
+		tbl8_group_index = i_lpm->lpm.tbl24[tbl24_index].group_idx;
 		tbl8_group_start = tbl8_group_index *
 				RTE_LPM_TBL8_GROUP_NUM_ENTRIES;
 		tbl8_index = tbl8_group_start + (ip_masked & 0xFF);
 
 		for (i = tbl8_index; i < (tbl8_index + tbl8_range); i++) {
 
-			if (!lpm->tbl8[i].valid ||
-					lpm->tbl8[i].depth <= depth) {
+			if (!i_lpm->lpm.tbl8[i].valid ||
+					i_lpm->lpm.tbl8[i].depth <= depth) {
 				struct rte_lpm_tbl_entry new_tbl8_entry = {
 					.valid = VALID,
 					.depth = depth,
 					.next_hop = next_hop,
-					.valid_group = lpm->tbl8[i].valid_group,
+					.valid_group = i_lpm->lpm.tbl8[i].valid_group,
 				};
 
 				/*
 				 * Setting tbl8 entry in one go to avoid race
 				 * condition
 				 */
-				__atomic_store(&lpm->tbl8[i], &new_tbl8_entry,
+				__atomic_store(&i_lpm->lpm.tbl8[i], &new_tbl8_entry,
 						__ATOMIC_RELAXED);
 
 				continue;
@@ -794,16 +807,18 @@ rte_lpm_add(struct rte_lpm *lpm, uint32_t ip, uint8_t depth,
 		uint32_t next_hop)
 {
 	int32_t rule_index, status = 0;
+	struct __rte_lpm *i_lpm;
 	uint32_t ip_masked;
 
 	/* Check user arguments. */
 	if ((lpm == NULL) || (depth < 1) || (depth > RTE_LPM_MAX_DEPTH))
 		return -EINVAL;
 
+	i_lpm = container_of(lpm, struct __rte_lpm, lpm);
 	ip_masked = ip & depth_to_mask(depth);
 
 	/* Add the rule to the rule table. */
-	rule_index = rule_add(lpm, ip_masked, depth, next_hop);
+	rule_index = rule_add(i_lpm, ip_masked, depth, next_hop);
 
 	/* Skip table entries update if The rule is the same as
 	 * the rule in the rules table.
@@ -817,16 +832,16 @@ rte_lpm_add(struct rte_lpm *lpm, uint32_t ip, uint8_t depth,
 	}
 
 	if (depth <= MAX_DEPTH_TBL24) {
-		status = add_depth_small(lpm, ip_masked, depth, next_hop);
+		status = add_depth_small(i_lpm, ip_masked, depth, next_hop);
 	} else { /* If depth > RTE_LPM_MAX_DEPTH_TBL24 */
-		status = add_depth_big(lpm, ip_masked, depth, next_hop);
+		status = add_depth_big(i_lpm, ip_masked, depth, next_hop);
 
 		/*
 		 * If add fails due to exhaustion of tbl8 extensions delete
 		 * rule that was added to rule table.
 		 */
 		if (status < 0) {
-			rule_delete(lpm, rule_index, depth);
+			rule_delete(i_lpm, rule_index, depth);
 
 			return status;
 		}
@@ -842,6 +857,7 @@ int
 rte_lpm_is_rule_present(struct rte_lpm *lpm, uint32_t ip, uint8_t depth,
 uint32_t *next_hop)
 {
+	struct __rte_lpm *i_lpm;
 	uint32_t ip_masked;
 	int32_t rule_index;
 
@@ -852,11 +868,12 @@ uint32_t *next_hop)
 		return -EINVAL;
 
 	/* Look for the rule using rule_find. */
+	i_lpm = container_of(lpm, struct __rte_lpm, lpm);
 	ip_masked = ip & depth_to_mask(depth);
-	rule_index = rule_find(lpm, ip_masked, depth);
+	rule_index = rule_find(i_lpm, ip_masked, depth);
 
 	if (rule_index >= 0) {
-		*next_hop = lpm->rules_tbl[rule_index].next_hop;
+		*next_hop = i_lpm->rules_tbl[rule_index].next_hop;
 		return 1;
 	}
 
@@ -865,7 +882,7 @@ uint32_t *next_hop)
 }
 
 static int32_t
-find_previous_rule(struct rte_lpm *lpm, uint32_t ip, uint8_t depth,
+find_previous_rule(struct __rte_lpm *i_lpm, uint32_t ip, uint8_t depth,
 		uint8_t *sub_rule_depth)
 {
 	int32_t rule_index;
@@ -875,7 +892,7 @@ find_previous_rule(struct rte_lpm *lpm, uint32_t ip, uint8_t depth,
 	for (prev_depth = (uint8_t)(depth - 1); prev_depth > 0; prev_depth--) {
 		ip_masked = ip & depth_to_mask(prev_depth);
 
-		rule_index = rule_find(lpm, ip_masked, prev_depth);
+		rule_index = rule_find(i_lpm, ip_masked, prev_depth);
 
 		if (rule_index >= 0) {
 			*sub_rule_depth = prev_depth;
@@ -887,7 +904,7 @@ find_previous_rule(struct rte_lpm *lpm, uint32_t ip, uint8_t depth,
 }
 
 static int32_t
-delete_depth_small(struct rte_lpm *lpm, uint32_t ip_masked,
+delete_depth_small(struct __rte_lpm *i_lpm, uint32_t ip_masked,
 	uint8_t depth, int32_t sub_rule_index, uint8_t sub_rule_depth)
 {
 #define group_idx next_hop
@@ -909,26 +926,26 @@ delete_depth_small(struct rte_lpm *lpm, uint32_t ip_masked,
 		 */
 		for (i = tbl24_index; i < (tbl24_index + tbl24_range); i++) {
 
-			if (lpm->tbl24[i].valid_group == 0 &&
-					lpm->tbl24[i].depth <= depth) {
-				__atomic_store(&lpm->tbl24[i],
+			if (i_lpm->lpm.tbl24[i].valid_group == 0 &&
+					i_lpm->lpm.tbl24[i].depth <= depth) {
+				__atomic_store(&i_lpm->lpm.tbl24[i],
 					&zero_tbl24_entry, __ATOMIC_RELEASE);
-			} else if (lpm->tbl24[i].valid_group == 1) {
+			} else if (i_lpm->lpm.tbl24[i].valid_group == 1) {
 				/*
 				 * If TBL24 entry is extended, then there has
 				 * to be a rule with depth >= 25 in the
 				 * associated TBL8 group.
 				 */
 
-				tbl8_group_index = lpm->tbl24[i].group_idx;
+				tbl8_group_index = i_lpm->lpm.tbl24[i].group_idx;
 				tbl8_index = tbl8_group_index *
 						RTE_LPM_TBL8_GROUP_NUM_ENTRIES;
 
 				for (j = tbl8_index; j < (tbl8_index +
 					RTE_LPM_TBL8_GROUP_NUM_ENTRIES); j++) {
 
-					if (lpm->tbl8[j].depth <= depth)
-						lpm->tbl8[j].valid = INVALID;
+					if (i_lpm->lpm.tbl8[j].depth <= depth)
+						i_lpm->lpm.tbl8[j].valid = INVALID;
 				}
 			}
 		}
@@ -939,7 +956,7 @@ delete_depth_small(struct rte_lpm *lpm, uint32_t ip_masked,
 		 */
 
 		struct rte_lpm_tbl_entry new_tbl24_entry = {
-			.next_hop = lpm->rules_tbl[sub_rule_index].next_hop,
+			.next_hop = i_lpm->rules_tbl[sub_rule_index].next_hop,
 			.valid = VALID,
 			.valid_group = 0,
 			.depth = sub_rule_depth,
@@ -949,32 +966,32 @@ delete_depth_small(struct rte_lpm *lpm, uint32_t ip_masked,
 			.valid = VALID,
 			.valid_group = VALID,
 			.depth = sub_rule_depth,
-			.next_hop = lpm->rules_tbl
+			.next_hop = i_lpm->rules_tbl
 			[sub_rule_index].next_hop,
 		};
 
 		for (i = tbl24_index; i < (tbl24_index + tbl24_range); i++) {
 
-			if (lpm->tbl24[i].valid_group == 0 &&
-					lpm->tbl24[i].depth <= depth) {
-				__atomic_store(&lpm->tbl24[i], &new_tbl24_entry,
+			if (i_lpm->lpm.tbl24[i].valid_group == 0 &&
+					i_lpm->lpm.tbl24[i].depth <= depth) {
+				__atomic_store(&i_lpm->lpm.tbl24[i], &new_tbl24_entry,
 						__ATOMIC_RELEASE);
-			} else  if (lpm->tbl24[i].valid_group == 1) {
+			} else  if (i_lpm->lpm.tbl24[i].valid_group == 1) {
 				/*
 				 * If TBL24 entry is extended, then there has
 				 * to be a rule with depth >= 25 in the
 				 * associated TBL8 group.
 				 */
 
-				tbl8_group_index = lpm->tbl24[i].group_idx;
+				tbl8_group_index = i_lpm->lpm.tbl24[i].group_idx;
 				tbl8_index = tbl8_group_index *
 						RTE_LPM_TBL8_GROUP_NUM_ENTRIES;
 
 				for (j = tbl8_index; j < (tbl8_index +
 					RTE_LPM_TBL8_GROUP_NUM_ENTRIES); j++) {
 
-					if (lpm->tbl8[j].depth <= depth)
-						__atomic_store(&lpm->tbl8[j],
+					if (i_lpm->lpm.tbl8[j].depth <= depth)
+						__atomic_store(&i_lpm->lpm.tbl8[j],
 							&new_tbl8_entry,
 							__ATOMIC_RELAXED);
 				}
@@ -1041,7 +1058,7 @@ tbl8_recycle_check(struct rte_lpm_tbl_entry *tbl8,
 }
 
 static int32_t
-delete_depth_big(struct rte_lpm *lpm, uint32_t ip_masked,
+delete_depth_big(struct __rte_lpm *i_lpm, uint32_t ip_masked,
 	uint8_t depth, int32_t sub_rule_index, uint8_t sub_rule_depth)
 {
 #define group_idx next_hop
@@ -1056,7 +1073,7 @@ delete_depth_big(struct rte_lpm *lpm, uint32_t ip_masked,
 	tbl24_index = ip_masked >> 8;
 
 	/* Calculate the index into tbl8 and range. */
-	tbl8_group_index = lpm->tbl24[tbl24_index].group_idx;
+	tbl8_group_index = i_lpm->lpm.tbl24[tbl24_index].group_idx;
 	tbl8_group_start = tbl8_group_index * RTE_LPM_TBL8_GROUP_NUM_ENTRIES;
 	tbl8_index = tbl8_group_start + (ip_masked & 0xFF);
 	tbl8_range = depth_to_range(depth);
@@ -1067,16 +1084,16 @@ delete_depth_big(struct rte_lpm *lpm, uint32_t ip_masked,
 		 * rule_to_delete must be removed or modified.
 		 */
 		for (i = tbl8_index; i < (tbl8_index + tbl8_range); i++) {
-			if (lpm->tbl8[i].depth <= depth)
-				lpm->tbl8[i].valid = INVALID;
+			if (i_lpm->lpm.tbl8[i].depth <= depth)
+				i_lpm->lpm.tbl8[i].valid = INVALID;
 		}
 	} else {
 		/* Set new tbl8 entry. */
 		struct rte_lpm_tbl_entry new_tbl8_entry = {
 			.valid = VALID,
 			.depth = sub_rule_depth,
-			.valid_group = lpm->tbl8[tbl8_group_start].valid_group,
-			.next_hop = lpm->rules_tbl[sub_rule_index].next_hop,
+			.valid_group = i_lpm->lpm.tbl8[tbl8_group_start].valid_group,
+			.next_hop = i_lpm->rules_tbl[sub_rule_index].next_hop,
 		};
 
 		/*
@@ -1084,8 +1101,8 @@ delete_depth_big(struct rte_lpm *lpm, uint32_t ip_masked,
 		 * rule_to_delete must be modified.
 		 */
 		for (i = tbl8_index; i < (tbl8_index + tbl8_range); i++) {
-			if (lpm->tbl8[i].depth <= depth)
-				__atomic_store(&lpm->tbl8[i], &new_tbl8_entry,
+			if (i_lpm->lpm.tbl8[i].depth <= depth)
+				__atomic_store(&i_lpm->lpm.tbl8[i], &new_tbl8_entry,
 						__ATOMIC_RELAXED);
 		}
 	}
@@ -1096,31 +1113,31 @@ delete_depth_big(struct rte_lpm *lpm, uint32_t ip_masked,
 	 * associated tbl24 entry.
 	 */
 
-	tbl8_recycle_index = tbl8_recycle_check(lpm->tbl8, tbl8_group_start);
+	tbl8_recycle_index = tbl8_recycle_check(i_lpm->lpm.tbl8, tbl8_group_start);
 
 	if (tbl8_recycle_index == -EINVAL) {
 		/* Set tbl24 before freeing tbl8 to avoid race condition.
 		 * Prevent the free of the tbl8 group from hoisting.
 		 */
-		lpm->tbl24[tbl24_index].valid = 0;
+		i_lpm->lpm.tbl24[tbl24_index].valid = 0;
 		__atomic_thread_fence(__ATOMIC_RELEASE);
-		status = tbl8_free(lpm, tbl8_group_start);
+		status = tbl8_free(i_lpm, tbl8_group_start);
 	} else if (tbl8_recycle_index > -1) {
 		/* Update tbl24 entry. */
 		struct rte_lpm_tbl_entry new_tbl24_entry = {
-			.next_hop = lpm->tbl8[tbl8_recycle_index].next_hop,
+			.next_hop = i_lpm->lpm.tbl8[tbl8_recycle_index].next_hop,
 			.valid = VALID,
 			.valid_group = 0,
-			.depth = lpm->tbl8[tbl8_recycle_index].depth,
+			.depth = i_lpm->lpm.tbl8[tbl8_recycle_index].depth,
 		};
 
 		/* Set tbl24 before freeing tbl8 to avoid race condition.
 		 * Prevent the free of the tbl8 group from hoisting.
 		 */
-		__atomic_store(&lpm->tbl24[tbl24_index], &new_tbl24_entry,
+		__atomic_store(&i_lpm->lpm.tbl24[tbl24_index], &new_tbl24_entry,
 				__ATOMIC_RELAXED);
 		__atomic_thread_fence(__ATOMIC_RELEASE);
-		status = tbl8_free(lpm, tbl8_group_start);
+		status = tbl8_free(i_lpm, tbl8_group_start);
 	}
 #undef group_idx
 	return status;
@@ -1133,6 +1150,7 @@ int
 rte_lpm_delete(struct rte_lpm *lpm, uint32_t ip, uint8_t depth)
 {
 	int32_t rule_to_delete_index, sub_rule_index;
+	struct __rte_lpm *i_lpm;
 	uint32_t ip_masked;
 	uint8_t sub_rule_depth;
 	/*
@@ -1143,13 +1161,14 @@ rte_lpm_delete(struct rte_lpm *lpm, uint32_t ip, uint8_t depth)
 		return -EINVAL;
 	}
 
+	i_lpm = container_of(lpm, struct __rte_lpm, lpm);
 	ip_masked = ip & depth_to_mask(depth);
 
 	/*
 	 * Find the index of the input rule, that needs to be deleted, in the
 	 * rule table.
 	 */
-	rule_to_delete_index = rule_find(lpm, ip_masked, depth);
+	rule_to_delete_index = rule_find(i_lpm, ip_masked, depth);
 
 	/*
 	 * Check if rule_to_delete_index was found. If no rule was found the
@@ -1159,7 +1178,7 @@ rte_lpm_delete(struct rte_lpm *lpm, uint32_t ip, uint8_t depth)
 		return -EINVAL;
 
 	/* Delete the rule from the rule table. */
-	rule_delete(lpm, rule_to_delete_index, depth);
+	rule_delete(i_lpm, rule_to_delete_index, depth);
 
 	/*
 	 * Find rule to replace the rule_to_delete. If there is no rule to
@@ -1167,17 +1186,17 @@ rte_lpm_delete(struct rte_lpm *lpm, uint32_t ip, uint8_t depth)
 	 * entries associated with this rule.
 	 */
 	sub_rule_depth = 0;
-	sub_rule_index = find_previous_rule(lpm, ip, depth, &sub_rule_depth);
+	sub_rule_index = find_previous_rule(i_lpm, ip, depth, &sub_rule_depth);
 
 	/*
 	 * If the input depth value is less than 25 use function
 	 * delete_depth_small otherwise use delete_depth_big.
 	 */
 	if (depth <= MAX_DEPTH_TBL24) {
-		return delete_depth_small(lpm, ip_masked, depth,
+		return delete_depth_small(i_lpm, ip_masked, depth,
 				sub_rule_index, sub_rule_depth);
 	} else { /* If depth > MAX_DEPTH_TBL24 */
-		return delete_depth_big(lpm, ip_masked, depth, sub_rule_index,
+		return delete_depth_big(i_lpm, ip_masked, depth, sub_rule_index,
 				sub_rule_depth);
 	}
 }
@@ -1188,16 +1207,19 @@ rte_lpm_delete(struct rte_lpm *lpm, uint32_t ip, uint8_t depth)
 void
 rte_lpm_delete_all(struct rte_lpm *lpm)
 {
+	struct __rte_lpm *i_lpm;
+
+	i_lpm = container_of(lpm, struct __rte_lpm, lpm);
 	/* Zero rule information. */
-	memset(lpm->rule_info, 0, sizeof(lpm->rule_info));
+	memset(i_lpm->rule_info, 0, sizeof(i_lpm->rule_info));
 
 	/* Zero tbl24. */
-	memset(lpm->tbl24, 0, sizeof(lpm->tbl24));
+	memset(i_lpm->lpm.tbl24, 0, sizeof(i_lpm->lpm.tbl24));
 
 	/* Zero tbl8. */
-	memset(lpm->tbl8, 0, sizeof(lpm->tbl8[0])
-			* RTE_LPM_TBL8_GROUP_NUM_ENTRIES * lpm->number_tbl8s);
+	memset(i_lpm->lpm.tbl8, 0, sizeof(i_lpm->lpm.tbl8[0])
+			* RTE_LPM_TBL8_GROUP_NUM_ENTRIES * i_lpm->number_tbl8s);
 
 	/* Delete all rules form the rules table. */
-	memset(lpm->rules_tbl, 0, sizeof(lpm->rules_tbl[0]) * lpm->max_rules);
+	memset(i_lpm->rules_tbl, 0, sizeof(i_lpm->rules_tbl[0]) * i_lpm->max_rules);
 }
diff --git a/lib/librte_lpm/rte_lpm.h b/lib/librte_lpm/rte_lpm.h
index 5b3b7b5b58..1afe55cdcb 100644
--- a/lib/librte_lpm/rte_lpm.h
+++ b/lib/librte_lpm/rte_lpm.h
@@ -118,31 +118,12 @@ struct rte_lpm_config {
 	int flags;               /**< This field is currently unused. */
 };
 
-/** @internal Rule structure. */
-struct rte_lpm_rule {
-	uint32_t ip; /**< Rule IP address. */
-	uint32_t next_hop; /**< Rule next hop. */
-};
-
-/** @internal Contains metadata about the rules table. */
-struct rte_lpm_rule_info {
-	uint32_t used_rules; /**< Used rules so far. */
-	uint32_t first_rule; /**< Indexes the first rule of a given depth. */
-};
-
 /** @internal LPM structure. */
 struct rte_lpm {
-	/* LPM metadata. */
-	char name[RTE_LPM_NAMESIZE];        /**< Name of the lpm. */
-	uint32_t max_rules; /**< Max. balanced rules per lpm. */
-	uint32_t number_tbl8s; /**< Number of tbl8s. */
-	struct rte_lpm_rule_info rule_info[RTE_LPM_MAX_DEPTH]; /**< Rule info table. */
-
 	/* LPM Tables. */
 	struct rte_lpm_tbl_entry tbl24[RTE_LPM_TBL24_NUM_ENTRIES]
 			__rte_cache_aligned; /**< LPM tbl24 table. */
 	struct rte_lpm_tbl_entry *tbl8; /**< LPM tbl8 table. */
-	struct rte_lpm_rule *rules_tbl; /**< LPM rules. */
 };
 
 /** LPM RCU QSBR configuration structure. */
-- 
2.23.0

Re: [dpdk-dev] [PATCH v3 0/2] LPM changes

[David Marchand]

On Fri, Oct 23, 2020 at 11:39 AM David Marchand
<david.marchand@redhat.com> wrote:
>
> From Ruifeng Wang:
>
> The rte_lpm structure is exported because lookup API is inlined.
> But most of the structure can be hidden.
> Discussion at: http://patches.dpdk.org/patch/72403/
> This patch set aimed to hide the rte_lpm structure as much as possible.
>
> A data free issue was identified and fixed.

Series applied.
Thanks Ruifeng.


-- 
David Marchand