DPDK patches and discussions
 help / color / mirror / Atom feed
From: Bruce Richardson <bruce.richardson@intel.com>
To: Yerden Zhumabekov <e_zhumabekov@sts.kz>
Cc: dev@dpdk.org
Subject: Re: [dpdk-dev] [PATCH] hash: fix breaking strict-aliasing rules
Date: Thu, 19 Mar 2015 16:31:24 +0000
Message-ID: <20150319163123.GB5996@bricha3-MOBL3> (raw)
In-Reply-To: <20150319162547.GA5996@bricha3-MOBL3>

On Thu, Mar 19, 2015 at 04:25:47PM +0000, Bruce Richardson wrote:
> 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

Other than the below suggestions:

Acked-by: Bruce Richardson <bruce.richardson@intel.com>
> 
> > ---
> >  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?
> 
> > +	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.
> 
> >  	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.
> 
> > +	switch (i) {
> >  	case 0:
> > -		temp |= (uint64_t) *((const uint8_t *)p64 + 6) << 48;
> > -		/* Fallthrough */
> >  	case 1:
> > -		temp |= (uint64_t) *((const uint8_t *)p64 + 5) << 40;
> > -		/* Fallthrough */
> >  	case 2:
> > -		temp |= (uint64_t) *((const uint8_t *)p64 + 4) << 32;
> > -		temp |= *((const uint32_t *)p64);
> > +		temp = odd_8byte_mask[i] & *p64;
> >  		init_val = rte_hash_crc_8byte(temp, init_val);
> >  		break;
> >  	case 3:
> > -		init_val = rte_hash_crc_4byte(*(const uint32_t *)p64, init_val);
> > -		break;
> >  	case 4:
> > -		temp |= *((const uint8_t *)p64 + 2) << 16;
> > -		/* Fallthrough */
> >  	case 5:
> > -		temp |= *((const uint8_t *)p64 + 1) << 8;
> > -		/* Fallthrough */
> >  	case 6:
> > -		temp |= *((const uint8_t *)p64);
> > +		temp = odd_8byte_mask[i] & *p64;
> >  		init_val = rte_hash_crc_4byte(temp, init_val);
> > -		/* Fallthrough */
> >  	default:
> >  		break;
> >  	}
> > -- 
> > 1.7.9.5
> > 

  reply	other threads:[~2015-03-19 16: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 [this message]
2015-03-20  3:29     ` Yerden Zhumabekov
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=20150319163123.GB5996@bricha3-MOBL3 \
    --to=bruce.richardson@intel.com \
    --cc=dev@dpdk.org \
    --cc=e_zhumabekov@sts.kz \
    /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