DPDK patches and discussions
 help / color / mirror / Atom feed
From: Yerden Zhumabekov <e_zhumabekov@sts.kz>
To: Bruce Richardson <bruce.richardson@intel.com>
Cc: dev@dpdk.org
Subject: Re: [dpdk-dev] [PATCH] hash: fix breaking strict-aliasing rules
Date: Fri, 20 Mar 2015 09:29:47 +0600
Message-ID: <550B942B.3030606@sts.kz> (raw)
In-Reply-To: <20150319163123.GB5996@bricha3-MOBL3>

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 <e_zhumabekov@sts.kz>
> 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

  reply	other threads:[~2015-03-20  3:31 UTC|newest]

Thread overview: 10+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2015-03-18 16:51 Yerden Zhumabekov
2015-03-19 16:25 ` Bruce Richardson
2015-03-19 16:31   ` Bruce Richardson
2015-03-20  3:29     ` Yerden Zhumabekov [this message]
2015-03-20 12:41 ` Pawel Wodkowski
2015-03-20 12:47 ` Pawel Wodkowski
2015-03-21  6:57   ` Жумабеков Ерден Мирзагулович
2015-03-24 13:31 ` [dpdk-dev] [PATCH v2] " Yerden Zhumabekov
2015-03-26 17:47   ` De Lara Guarch, Pablo
2015-03-27  9:26     ` Thomas Monjalon

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=550B942B.3030606@sts.kz \
    --to=e_zhumabekov@sts.kz \
    --cc=bruce.richardson@intel.com \
    --cc=dev@dpdk.org \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link

DPDK patches and discussions

This inbox may be cloned and mirrored by anyone:

	git clone --mirror https://inbox.dpdk.org/dev/0 dev/git/0.git

	# If you have public-inbox 1.1+ installed, you may
	# initialize and index your mirror using the following commands:
	public-inbox-init -V2 dev dev/ https://inbox.dpdk.org/dev \
		dev@dpdk.org
	public-inbox-index dev

Example config snippet for mirrors.
Newsgroup available over NNTP:
	nntp://inbox.dpdk.org/inbox.dpdk.dev


AGPL code for this site: git clone https://public-inbox.org/public-inbox.git