Re: [dpdk-dev] [PATCH v3 3/7] hash: correct key store element alignment

[Honnappa Nagarahalli <Honnappa.Nagarahalli@arm.com>]

> >-----Original Message-----
> >From: Honnappa Nagarahalli [mailto:honnappa.nagarahalli@arm.com]
> >Sent: Thursday, October 11, 2018 11:32 PM
> >To: Richardson, Bruce <bruce.richardson@intel.com>; De Lara Guarch,
> >Pablo <pablo.de.lara.guarch@intel.com>
> >Cc: dev@dpdk.org; Wang, Yipeng1 <yipeng1.wang@intel.com>;
> >honnappa.nagarahalli@arm.com; dharmik.thakkar@arm.com;
> >gavin.hu@arm.com; nd@arm.com
> >Subject: [PATCH v3 3/7] hash: correct key store element alignment
> [Wang, Yipeng] "correct" -> "improve"?
I think 'fix' is the right word to use. If we look at the existing code, original author seems to have tried to align it on certain boundary:
struct rte_hash_key {
        union {
                uintptr_t idata;
                void *pdata;
        };
        /* Variable key size */
        char key[0];
} __attribute__((aligned(KEY_ALIGNMENT)));

But, this does not align every element of the key-store on the alignment boundary. This patch fixes it.
I think what is missing is the "Fixes" tag. I will add that.

I found this bug because I made the store/load of 'pdata' atomic.

> >
> >Correct the key store array element alignment. This is required to make
> >'pdata' in 'struct rte_hash_key' align on the correct boundary.
> [Wang, Yipeng]
> More explanation in commit message is appreciated, because people may not
> understand what is "correct" boundary.
> e.g. Previously pdata could spread across multiple cache lines, which makes
> the access of pdata non-atomic which may have performance implications.
> 
I will add more explanation related to atomic access.

> Otherwise
> Reviewed-by: Yipeng Wang <yipeng1.wang@intel.com>