From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from mga03.intel.com (mga03.intel.com [134.134.136.65]) by dpdk.org (Postfix) with ESMTP id 11016ADA7 for ; Mon, 18 May 2015 18:14:12 +0200 (CEST) Received: from orsmga001.jf.intel.com ([10.7.209.18]) by orsmga103.jf.intel.com with ESMTP; 18 May 2015 09:14:04 -0700 X-ExtLoop1: 1 X-IronPort-AV: E=Sophos;i="5.13,453,1427785200"; d="scan'208";a="696652212" Received: from bricha3-mobl3.ger.corp.intel.com ([10.237.221.63]) by orsmga001.jf.intel.com with SMTP; 18 May 2015 09:14:03 -0700 Received: by (sSMTP sendmail emulation); Mon, 18 May 2015 17:14:02 +0025 Date: Mon, 18 May 2015 17:14:02 +0100 From: Bruce Richardson To: Pablo de Lara Message-ID: <20150518161401.GA3508@bricha3-MOBL3> References: <1430837034-21031-1-git-send-email-pablo.de.lara.guarch@intel.com> <1431428560-25426-1-git-send-email-pablo.de.lara.guarch@intel.com> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: <1431428560-25426-1-git-send-email-pablo.de.lara.guarch@intel.com> Organization: Intel Shannon Ltd. User-Agent: Mutt/1.5.23 (2014-03-12) Cc: dev@dpdk.org Subject: Re: [dpdk-dev] [PATCH v4 0/6] update jhash function 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: , X-List-Received-Date: Mon, 18 May 2015 16:14:13 -0000 On Tue, May 12, 2015 at 12:02:32PM +0100, Pablo de Lara wrote: > Jenkins hash function was developed originally in 1996, > and was integrated in first versions of DPDK. > The function has been improved in 2006, > achieving up to 60% better performance, compared to the original one. > > This patchset updates the current jhash in DPDK, > including two new functions that generate two hashes from a single key. > > It also separates the existing hash function performance tests to > another file, to make it quicker to run. > > changes in v4: > - Simplify key alignment checks > - Include missing x86 arch check > > changes in v3: > > - Update rte_jhash_1word, rte_jhash_2words and rte_jhash_3words > functions > > changes in v2: > > - Split single commit in three commits, one that updates the existing functions > and another that adds two new functions and use one of those functions > as a base to be called by the other ones. > - Remove some unnecessary ifdefs in the code. > - Add new macros to help on the reutilization of constants > - Separate hash function performance tests to another file > and improve cycle measurements. > - Rename existing function rte_jhash2 to rte_jhash_32b > (something more meaninful) and mark rte_jhash2 as > deprecated > Hi Pablo, Patchset looks good to me, and unit tests all pass across the set. Some general comments or suggestions though - particularly about testing. 1. The set of lengths used when testing the functions looks strange and rather arbitrary. Perhaps we could have a set of key lengths which are documented. E.g. lengths[] = { 4, 8, 16, 48, 64, /* standard key sizes */ 9, /* IPv4 SRC + DST + protocol, unpadded */ 13, /* IPv4 5-tuple, unpadded */ 37, /* IPv6 5-tuple, unpadded */ 40, /* IPv6 5-tuple, padded to 8-byte boundary */ } 2. When testing multiple algorithms, it might be nice to change the order of the loops so that we test all algorithms with the same key lengths first, and then change length, rather than running the same algorithm with multiple lengths and then changing algorithm. The output would be clearer and easier to see which algorithm performs best for a given key-length. 3. For sanity checking across the patches making changes to the jhash functions, I think it would be nice to have an initial sanity test with a set of known keys and hash results in it. That way we can verify that the actual calculation result never changes as the functions are modified. This would also be a big help for future work changing the code. [As far as I can see, we don't ever check in the algorithm checks that we are ever getting the right answer :-)] All the above suggestions could perhaps go in a patch (or 2/3 patches) after the first two, which splits out the algorithm tests, and before the actual changes to the jhash implementation. Regards, /Bruce