[dpdk-dev] [PATCH] lib/librte_table: Fix table array lookup

[dpdk-dev] [PATCH] lib/librte_table: Fix table array lookup

[Mark Wunderlich]

The existing lookup function was returning an unmodified
pkts_mask bitmask into lookup_hit_mask.  This effectively
assumes that all packets would index correctly into one
of the array table entries.

Also, there was no check that the metadata provided index
value was within range of the table max entries.  By using
using table index bitmask on the metadata provided index
the resulting entry position may falsely indicate a hit
for index values provided that happen to be greter than
the number of table entries.

Like other table type lookup functions it would seem that
the possibility exists that some of the packets provided
to the function would not result in a hit.  It is assumed
that the metadata provided should be a direct index into
the array table.  So, code was added to build and return
a bitmask for only those packets that correctly index
directly into the table array.

If the original intent for this table type was to accept
any 32-bit value, then by applying the table index bitmask
as a modulo index for distribution across table entries,
then this patch would be invalid and should be rejected.

Signed-off-by: Mark Wunderlich <mark.w.wunderlich@intel.com>
---
 lib/librte_table/rte_table_array.c |   25 +++++++++++++++----------
 1 file changed, 15 insertions(+), 10 deletions(-)

diff --git a/lib/librte_table/rte_table_array.c b/lib/librte_table/rte_table_array.c
index c031070..0164d18 100644
--- a/lib/librte_table/rte_table_array.c
+++ b/lib/librte_table/rte_table_array.c
@@ -164,8 +164,7 @@ rte_table_array_lookup(
 	void **entries)
 {
 	struct rte_table_array *t = (struct rte_table_array *) table;
-
-	*lookup_hit_mask = pkts_mask;
+	uint64_t pkts_out_mask = 0;
 
 	if ((pkts_mask & (pkts_mask + 1)) == 0) {
 		uint64_t n_pkts = __builtin_popcountll(pkts_mask);
@@ -173,26 +172,32 @@ rte_table_array_lookup(
 
 		for (i = 0; i < n_pkts; i++) {
 			struct rte_mbuf *pkt = pkts[i];
-			uint32_t entry_pos = RTE_MBUF_METADATA_UINT32(pkt,
-				t->offset) & t->entry_pos_mask;
+			uint32_t entry_pos = RTE_MBUF_METADATA_UINT32(pkt,t->offset);
 
-			entries[i] = (void *) &t->array[entry_pos *
-				t->entry_size];
+			if (entry_pos < t->n_entries) {
+				entries[i] = (void *) &t->array[entry_pos *
+						t->entry_size];
+				pkts_out_mask |= (1LLU << i);
+			}
 		}
 	} else {
 		for ( ; pkts_mask; ) {
 			uint32_t pkt_index = __builtin_ctzll(pkts_mask);
 			uint64_t pkt_mask = 1LLU << pkt_index;
 			struct rte_mbuf *pkt = pkts[pkt_index];
-			uint32_t entry_pos = RTE_MBUF_METADATA_UINT32(pkt,
-				t->offset) & t->entry_pos_mask;
+			uint32_t entry_pos = RTE_MBUF_METADATA_UINT32(pkt,t->offset);
 
-			entries[pkt_index] = (void *) &t->array[entry_pos *
-				t->entry_size];
+			if (entry_pos < t->n_entries) {
+				entries[pkt_index] = (void *) &t->array[entry_pos *
+							t->entry_size];
+				pkts_out_mask |= pkt_mask;
+			}
 			pkts_mask &= ~pkt_mask;
 		}
 	}
 
+	*lookup_hit_mask = pkts_out_mask;
+
 	return 0;
 }
 

Re: [dpdk-dev] [PATCH] lib/librte_table: Fix table array lookup

[Thomas Monjalon]

Cristian, this patch is about packet framework.
Could you review it please?

2014-12-12 17:06, Mark Wunderlich:
> The existing lookup function was returning an unmodified
> pkts_mask bitmask into lookup_hit_mask.  This effectively
> assumes that all packets would index correctly into one
> of the array table entries.
> 
> Also, there was no check that the metadata provided index
> value was within range of the table max entries.  By using
> using table index bitmask on the metadata provided index
> the resulting entry position may falsely indicate a hit
> for index values provided that happen to be greter than
> the number of table entries.
> 
> Like other table type lookup functions it would seem that
> the possibility exists that some of the packets provided
> to the function would not result in a hit.  It is assumed
> that the metadata provided should be a direct index into
> the array table.  So, code was added to build and return
> a bitmask for only those packets that correctly index
> directly into the table array.
> 
> If the original intent for this table type was to accept
> any 32-bit value, then by applying the table index bitmask
> as a modulo index for distribution across table entries,
> then this patch would be invalid and should be rejected.
> 
> Signed-off-by: Mark Wunderlich <mark.w.wunderlich@intel.com>
> ---
>  lib/librte_table/rte_table_array.c |   25 +++++++++++++++----------
>  1 file changed, 15 insertions(+), 10 deletions(-)

Re: [dpdk-dev] [PATCH] lib/librte_table: Fix table array lookup

[Dumitrescu, Cristian]

Hi Mark,

NACK

>If the original intent for this table type was to accept
>any 32-bit value, then by applying the table index bitmask
>as a modulo index for distribution across table entries,
>then this patch would be invalid and should be rejected.

I think you got it exactly right, this was the intention. The reason is favouring lookup speed instead of checks.
For all tables, we do run-time checks for create/entry add/entry delete and we never do run-time checks for lookup.
If there is any inconvenience about this, let you and I talk.

We are planning to have a documentation enhancement for 2.0 release, this is one thing to clearly mention about array tables.

Thanks!

Regards,
Cristian

-----Original Message-----
From: dev [mailto:dev-bounces@dpdk.org] On Behalf Of Mark Wunderlich
Sent: Saturday, December 13, 2014 1:07 AM
To: dev@dpdk.org
Subject: [dpdk-dev] [PATCH] lib/librte_table: Fix table array lookup

The existing lookup function was returning an unmodified
pkts_mask bitmask into lookup_hit_mask.  This effectively
assumes that all packets would index correctly into one
of the array table entries.

Also, there was no check that the metadata provided index
value was within range of the table max entries.  By using
using table index bitmask on the metadata provided index
the resulting entry position may falsely indicate a hit
for index values provided that happen to be greter than
the number of table entries.

Like other table type lookup functions it would seem that
the possibility exists that some of the packets provided
to the function would not result in a hit.  It is assumed
that the metadata provided should be a direct index into
the array table.  So, code was added to build and return
a bitmask for only those packets that correctly index
directly into the table array.

If the original intent for this table type was to accept
any 32-bit value, then by applying the table index bitmask
as a modulo index for distribution across table entries,
then this patch would be invalid and should be rejected.

Signed-off-by: Mark Wunderlich <mark.w.wunderlich@intel.com>
---
 lib/librte_table/rte_table_array.c |   25 +++++++++++++++----------
 1 file changed, 15 insertions(+), 10 deletions(-)

diff --git a/lib/librte_table/rte_table_array.c b/lib/librte_table/rte_table_array.c
index c031070..0164d18 100644
--- a/lib/librte_table/rte_table_array.c
+++ b/lib/librte_table/rte_table_array.c
@@ -164,8 +164,7 @@ rte_table_array_lookup(
 	void **entries)
 {
 	struct rte_table_array *t = (struct rte_table_array *) table;
-
-	*lookup_hit_mask = pkts_mask;
+	uint64_t pkts_out_mask = 0;
 
 	if ((pkts_mask & (pkts_mask + 1)) == 0) {
 		uint64_t n_pkts = __builtin_popcountll(pkts_mask);
@@ -173,26 +172,32 @@ rte_table_array_lookup(
 
 		for (i = 0; i < n_pkts; i++) {
 			struct rte_mbuf *pkt = pkts[i];
-			uint32_t entry_pos = RTE_MBUF_METADATA_UINT32(pkt,
-				t->offset) & t->entry_pos_mask;
+			uint32_t entry_pos = RTE_MBUF_METADATA_UINT32(pkt,t->offset);
 
-			entries[i] = (void *) &t->array[entry_pos *
-				t->entry_size];
+			if (entry_pos < t->n_entries) {
+				entries[i] = (void *) &t->array[entry_pos *
+						t->entry_size];
+				pkts_out_mask |= (1LLU << i);
+			}
 		}
 	} else {
 		for ( ; pkts_mask; ) {
 			uint32_t pkt_index = __builtin_ctzll(pkts_mask);
 			uint64_t pkt_mask = 1LLU << pkt_index;
 			struct rte_mbuf *pkt = pkts[pkt_index];
-			uint32_t entry_pos = RTE_MBUF_METADATA_UINT32(pkt,
-				t->offset) & t->entry_pos_mask;
+			uint32_t entry_pos = RTE_MBUF_METADATA_UINT32(pkt,t->offset);
 
-			entries[pkt_index] = (void *) &t->array[entry_pos *
-				t->entry_size];
+			if (entry_pos < t->n_entries) {
+				entries[pkt_index] = (void *) &t->array[entry_pos *
+							t->entry_size];
+				pkts_out_mask |= pkt_mask;
+			}
 			pkts_mask &= ~pkt_mask;
 		}
 	}
 
+	*lookup_hit_mask = pkts_out_mask;
+
 	return 0;
 }
 

--------------------------------------------------------------
Intel Shannon Limited
Registered in Ireland
Registered Office: Collinstown Industrial Park, Leixlip, County Kildare
Registered Number: 308263
Business address: Dromore House, East Park, Shannon, Co. Clare

This e-mail and any attachments may contain confidential material for the sole use of the intended recipient(s). Any review or distribution by others is strictly prohibited. If you are not the intended recipient, please contact the sender and delete all copies.