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:25:47 +0000 Message-ID: <20150319162547.GA5996@bricha3-MOBL3> (raw) In-Reply-To: <b441d020bd7830b1c66b6f4d2aa8d776a7f0bffd.1426697208.git.e_zhumabekov@sts.kz> 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? > + 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 >
next prev parent reply other threads:[~2015-03-19 16:25 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 [this message] 2015-03-19 16:31 ` Bruce Richardson 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=20150319162547.GA5996@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