[v3,3/7] hash: correct key store element alignment
Checks
Commit Message
Correct the key store array element alignment. This is required to
make 'pdata' in 'struct rte_hash_key' align on the correct boundary.
Signed-off-by: Honnappa Nagarahalli <honnappa.nagarahalli@arm.com>
Reviewed-by: Gavin Hu <gavin.hu@arm.com>
Reviewed-by: Ola Liljedahl <ola.liljedahl@arm.com>
Reviewed-by: Steve Capper <steve.capper@arm.com>
---
lib/librte_hash/rte_cuckoo_hash.c | 4 +++-
lib/librte_hash/rte_cuckoo_hash.h | 2 +-
2 files changed, 4 insertions(+), 2 deletions(-)
Comments
>-----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"?
>
>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.
Otherwise
Reviewed-by: Yipeng Wang <yipeng1.wang@intel.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>
@@ -279,7 +279,9 @@ rte_hash_create(const struct rte_hash_parameters *params)
rte_ring_sp_enqueue(r_ext, (void *)((uintptr_t) i));
}
- const uint32_t key_entry_size = sizeof(struct rte_hash_key) + params->key_len;
+ const uint32_t key_entry_size =
+ RTE_ALIGN(sizeof(struct rte_hash_key) + params->key_len,
+ KEY_ALIGNMENT);
const uint64_t key_tbl_size = (uint64_t) key_entry_size * num_key_slots;
k = rte_zmalloc_socket(NULL, key_tbl_size,
@@ -123,7 +123,7 @@ struct rte_hash_key {
};
/* Variable key size */
char key[0];
-} __attribute__((aligned(KEY_ALIGNMENT)));
+};
/* All different signature compare functions */
enum rte_hash_sig_compare_function {