From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from smtp-fw-33001.amazon.com (smtp-fw-33001.amazon.com [207.171.189.228]) by dpdk.org (Postfix) with ESMTP id 0E0256A89 for ; Tue, 30 Sep 2014 20:08:11 +0200 (CEST) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=amazon.com; i=@amazon.com; q=dns/txt; s=amazon201209; t=1412100894; x=1443636894; h=from:to:cc:subject:date:message-id:references: in-reply-to:content-transfer-encoding:mime-version; bh=oDIZj7xGh8nVeK146RSq+yA0j4y0w7cbr50TICjYlVA=; b=dBAlMelEiK+Sf2iZ7rCRTvf7wgr0+Ov6sN+iG9ATVxTxgQ8G5Lc66gsp YgiDgzNc3o1MYQQCIeqehrUdM912/VsiGWSlgH+goHUJJ7Pti8nXCgDSA c5aSIYgB0tkcospZY/tyZddKEhD1vAC1bIfWVadRpF9bbJXQOT6jCYqpT 0=; X-IronPort-AV: E=Sophos;i="5.04,628,1406592000"; d="scan'208";a="109345150" Received: from email-inbound-relay-7002.iad7.amazon.com ([10.55.235.150]) by smtp-border-fw-out-33001.sea14.amazon.com with ESMTP/TLS/DHE-RSA-AES256-SHA; 30 Sep 2014 18:14:51 +0000 Received: from ex10-hub-9001.ant.amazon.com (iad1-ws-svc-lb91-vlan3.amazon.com [10.0.103.150]) by email-inbound-relay-7002.iad7.amazon.com (8.14.7/8.14.7) with ESMTP id s8UIEhYG004036 (version=TLSv1/SSLv3 cipher=AES128-SHA bits=128 verify=OK); Tue, 30 Sep 2014 18:14:50 GMT Received: from EX10-MBX-31003.ant.amazon.com ([fe80::5833:b3e6:f957:e42a]) by ex10-hub-9001.ant.amazon.com ([::1]) with mapi id 14.03.0181.006; Tue, 30 Sep 2014 11:14:47 -0700 From: "Saha, Avik (AWS)" To: Neil Horman Thread-Topic: [dpdk-dev] [PATCH] Fix for LRU corrupted returns Thread-Index: AQHP2KqLB1I3H3BZREmu6z+y0SVEIpwZPMLQgADhvwD//+Q+wA== Date: Tue, 30 Sep 2014 18:14:46 +0000 Message-ID: References: <20140925102155.GB32725@hmsreliant.think-freely.org> <20140930125126.GC2193@hmsreliant.think-freely.org> In-Reply-To: <20140930125126.GC2193@hmsreliant.think-freely.org> Accept-Language: en-US Content-Language: en-US X-MS-Has-Attach: X-MS-TNEF-Correlator: x-originating-ip: [10.184.49.70] Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: quoted-printable MIME-Version: 1.0 Precedence: Bulk Cc: "dev@dpdk.org" Subject: Re: [dpdk-dev] [PATCH] Fix for LRU corrupted returns X-BeenThere: dev@dpdk.org X-Mailman-Version: 2.1.15 List-Id: patches and discussions about DPDK List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Tue, 30 Sep 2014 18:08:12 -0000 I have to point out that I am commenting out the the power_of_2 check on en= try_size. I am not sure if this is the right way but I don't know why this = soft assumption is important (since I cannot find the power of 2 constraint= in the documentation). I agree with the 0 check but the only reason I did = not put that in is because entry size would at least be sizeof(struct rte_p= ipeline_table_entry) =3D 8 bytes (to which the action_data_size is added) Avik -----Original Message----- From: Neil Horman [mailto:nhorman@tuxdriver.com]=20 Sent: Tuesday, September 30, 2014 5:51 AM To: Saha, Avik (AWS) Cc: dev@dpdk.org Subject: Re: [dpdk-dev] [PATCH] Fix for LRU corrupted returns On Tue, Sep 30, 2014 at 06:26:23AM +0000, Saha, Avik (AWS) wrote: > Sorry about the delay. The number 32 is not really a CACHE_LINE_SIZE but = since __builtin_clz returns the number of leading 0's before the most signi= ficant set bit in a 32 bit number (entry_size is uint32_t), I subtract that= number from 32 to get the number of trailing bits after the most significa= nt set bit. This will be the separation in my data_mem regions. >=20 Ah, ok, then change that 32 to sizeof(t->data_size_shl) to protect you agai= nst type changes and to avoid having magic values running around in your co= de. Also, you might want to do some sanity checking of entry_size as it se= ems like theres a soft assumption that entry size is non-zero and a power o= f two. while the latter is checked higher in the function, the former isn't and __= builtin_clz has undefined behavior if its passed a zero value. Neil > -----Original Message----- > From: Neil Horman [mailto:nhorman@tuxdriver.com] > Sent: Thursday, September 25, 2014 3:22 AM > To: Saha, Avik (AWS) > Cc: dev@dpdk.org > Subject: Re: [dpdk-dev] [PATCH] Fix for LRU corrupted returns >=20 > On Thu, Sep 25, 2014 at 07:46:16AM +0000, Saha, Avik (AWS) wrote: > > This is a patch to a problem that I have faced (described in the threa= d) and this works for me. > >=20 > > 1) Since the data_size_shl was getting its value from the key_size= , the table data entries were being corrupted when the calculation to shift= the number of bits was being made based on the key_size (according to the = document the key_size and entry_size are independently configurable) - With= this fix, we get the MSB that is set in entry_size (also removes the const= raint of this having to be a power of 2 - not entirely sure if this was the= reason the constraint was kept though) > > 2) The document does not say that the entry_size needs to be a pow= er of 2 and this was failing silently when I was trying to bring my applica= tion up. > >=20 > > diff --git a/DPDK/lib/librte_table/rte_table_hash_lru.c > > b/DPDK/lib/librte_table/rte_table_hash_lru.c > > index d1a4984..4ec9aa4 100644 > > --- a/DPDK/lib/librte_table/rte_table_hash_lru.c > > +++ b/DPDK/lib/librte_table/rte_table_hash_lru.c > > @@ -153,8 +153,10 @@ rte_table_hash_lru_create(void *params, int socket= _id, uint32_t entry_size) > > uint32_t i; > >=20 > > /* Check input parameters */ > > - if ((check_params_create(p) !=3D 0) || > > - (!rte_is_power_of_2(entry_size)) || > > + // Commenting out the power of 2 check on the entry_size since = the > > + // Programmers Guide does not call this out and we are going to= handle > > + // the data_size_shl of the table later on (Line 197) > Please remove the reference to Line 197 here. Thats not going to remain = accurate for very long. >=20 > > + if ((check_params_create(p) !=3D 0) || > > ((sizeof(struct rte_table_hash) % CACHE_LINE_SIZE) !=3D= 0) || > > (sizeof(struct bucket) !=3D (CACHE_LINE_SIZE / 2))) { > > return NULL; > > @@ -192,7 +194,7 @@ rte_table_hash_lru_create(void *params, int socket_= id, uint32_t entry_size) > > /* Internal */ > > t->bucket_mask =3D t->n_buckets - 1; > > t->key_size_shl =3D __builtin_ctzl(p->key_size); > > - t->data_size_shl =3D __builtin_ctzl(p->key_size); > > + t->data_size_shl =3D 32 - (__builtin_clz(entry_size)); > I presume the 32 value here is a cache line size? That should be replace= d with CACHE_LINE_SIZE...Though looking at it, that doesn't seem sufficient= . Seems like we need a eal abstraction to dynamically tell us what the cac= he line size is (we can read it from /proc/cpuinfo in linux, not sure about= bsd). >=20 > Neil >=20 >=20