From patchwork Tue Dec 16 19:13:55 2014 Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: 7bit X-Patchwork-Submitter: Cristian Dumitrescu X-Patchwork-Id: 2037 Return-Path: X-Original-To: patchwork@dpdk.org Delivered-To: patchwork@dpdk.org Received: from [92.243.14.124] (localhost [IPv6:::1]) by dpdk.org (Postfix) with ESMTP id 7DCB68075; Tue, 16 Dec 2014 20:36:16 +0100 (CET) Received: from mga03.intel.com (mga03.intel.com [134.134.136.65]) by dpdk.org (Postfix) with ESMTP id E58EC805F for ; Tue, 16 Dec 2014 20:36:13 +0100 (CET) Received: from orsmga003.jf.intel.com ([10.7.209.27]) by orsmga103.jf.intel.com with ESMTP; 16 Dec 2014 11:11:41 -0800 X-ExtLoop1: 1 X-IronPort-AV: E=Sophos;i="5.04,691,1406617200"; d="scan'208";a="499716796" Received: from irsmsx103.ger.corp.intel.com ([163.33.3.157]) by orsmga003.jf.intel.com with ESMTP; 16 Dec 2014 11:09:43 -0800 Received: from irsmsx108.ger.corp.intel.com ([169.254.11.150]) by IRSMSX103.ger.corp.intel.com ([169.254.3.113]) with mapi id 14.03.0195.001; Tue, 16 Dec 2014 19:13:55 +0000 From: "Dumitrescu, Cristian" To: "Wunderlich, Mark W" , "dev@dpdk.org" Thread-Topic: [dpdk-dev] [PATCH] lib/librte_table: Fix table array lookup Thread-Index: AQHQFnIwG3+qW2QW10q8mdY0wgOlIJySmnSg Date: Tue, 16 Dec 2014 19:13:55 +0000 Message-ID: <3EB4FA525960D640B5BDFFD6A3D89126322DF189@IRSMSX108.ger.corp.intel.com> References: <20141213010638.28002.63966.stgit@localhost.localdomain> In-Reply-To: <20141213010638.28002.63966.stgit@localhost.localdomain> Accept-Language: en-US Content-Language: en-US X-MS-Has-Attach: X-MS-TNEF-Correlator: x-originating-ip: [163.33.239.181] MIME-Version: 1.0 Subject: Re: [dpdk-dev] [PATCH] lib/librte_table: Fix table array lookup X-BeenThere: dev@dpdk.org X-Mailman-Version: 2.1.15 Precedence: list List-Id: patches and discussions about DPDK List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Errors-To: dev-bounces@dpdk.org Sender: "dev" 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 --- lib/librte_table/rte_table_array.c | 25 +++++++++++++++---------- 1 file changed, 15 insertions(+), 10 deletions(-) -------------------------------------------------------------- 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. 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; }