From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from mgw.gov.kz (mgw.gov.kz [89.218.88.242]) by dpdk.org (Postfix) with ESMTP id B141058F1 for ; Fri, 20 Mar 2015 04:31:43 +0100 (CET) Received: from mgw.gov.kz (mx.ctsat.kz [178.89.4.95]) by mgw.gov.kz with ESMTP id t2K3VgxY019583-t2K3VgxZ019583; Fri, 20 Mar 2015 09:31:42 +0600 Received: from EXCASHUB1.rgp.local (192.168.40.51) by EdgeForefront.rgp.local (192.168.40.59) with Microsoft SMTP Server (TLS) id 14.2.247.3; Fri, 20 Mar 2015 09:29:51 +0600 Received: from [192.168.35.15] (192.168.35.15) by excashub1.rgp.local (192.168.40.48) with Microsoft SMTP Server (TLS) id 14.2.247.3; Fri, 20 Mar 2015 09:31:37 +0600 Message-ID: <550B942B.3030606@sts.kz> Date: Fri, 20 Mar 2015 09:29:47 +0600 From: Yerden Zhumabekov User-Agent: Mozilla/5.0 (Windows NT 6.3; WOW64; rv:31.0) Gecko/20100101 Thunderbird/31.5.0 MIME-Version: 1.0 To: Bruce Richardson References: <20150319162547.GA5996@bricha3-MOBL3> <20150319163123.GB5996@bricha3-MOBL3> In-Reply-To: <20150319163123.GB5996@bricha3-MOBL3> Content-Type: text/plain; charset="UTF-8" Content-Transfer-Encoding: 8bit X-Originating-IP: [192.168.35.15] X-FEAS-SYSTEM-WL: e_zhumabekov@sts.kz Cc: dev@dpdk.org Subject: Re: [dpdk-dev] [PATCH] hash: fix breaking strict-aliasing rules 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: Fri, 20 Mar 2015 03:31:44 -0000 Hi Bruce, Answers below. 19.03.2015 22:25, Bruce Richardson пишет: > On Wed, Mar 18, 2015 at 10:51:12PM +0600, Yerden Zhumabekov wrote: >> Fix rte_hash_crc() function. Casting uint64_t pointer to uin32_t >> may trigger a compiler warning about breaking strict-aliasing rules. >> To avoid that, introduce a lookup table which is used to mask out >> a remainder of data. >> >> See issue #1, http://dpdk.org/ml/archives/dev/2015-March/015174.html >> >> Signed-off-by: Yerden Zhumabekov > Looks ok to me. Couple of minor suggestions below. > > /Bruce > >> --- >> lib/librte_hash/rte_hash_crc.h | 31 +++++++++++++++---------------- >> 1 file changed, 15 insertions(+), 16 deletions(-) >> >> diff --git a/lib/librte_hash/rte_hash_crc.h b/lib/librte_hash/rte_hash_crc.h >> index 3dcd362..e81920f 100644 >> --- a/lib/librte_hash/rte_hash_crc.h >> +++ b/lib/librte_hash/rte_hash_crc.h >> @@ -323,6 +323,16 @@ static const uint32_t crc32c_tables[8][256] = {{ >> 0xE54C35A1, 0xAC704886, 0x7734CFEF, 0x3E08B2C8, 0xC451B7CC, 0x8D6DCAEB, 0x56294D82, 0x1F1530A5 >> }}; >> >> +static const uint64_t odd_8byte_mask[] = { > Where does the name of this variable come from, it seems unclear to me? If the number of bytes in data for CRC hashing cannot be evenly divided by 8, the remainder is extracted with these masks. Hence, we have 'odd' bytes to mask out. Maybe my poor english. :) Suggestions are welcome. What about remainder_8byte_mask? >> + 0x00FFFFFFFFFFFFFF, >> + 0x0000FFFFFFFFFFFF, >> + 0x000000FFFFFFFFFF, >> + 0x00000000FFFFFFFF, >> + 0x0000000000FFFFFF, >> + 0x000000000000FFFF, >> + 0x00000000000000FF, >> +}; >> + >> #define CRC32_UPD(crc, n) \ >> (crc32c_tables[(n)][(crc) & 0xFF] ^ \ >> crc32c_tables[(n)-1][((crc) >> 8) & 0xFF]) >> @@ -535,38 +545,27 @@ static inline uint32_t >> rte_hash_crc(const void *data, uint32_t data_len, uint32_t init_val) >> { >> unsigned i; >> - uint64_t temp = 0; >> + uint64_t temp; > It is worth keeping variable "temp" at all, it looks to me like it could be done > away with without seriously affecting readability. Noted. >> const uint64_t *p64 = (const uint64_t *)data; >> >> for (i = 0; i < data_len / 8; i++) { >> init_val = rte_hash_crc_8byte(*p64++, init_val); >> } >> >> - switch (7 - (data_len & 0x07)) { >> + i = 7 - (data_len & 0x07); > i is not a terribly meaningful variable name, perhaps a slightly longer, more > meaningful name might improve readability. Noted, I'll declare a new one. -- Sincerely, Yerden Zhumabekov State Technical Service Astana, KZ