| Message ID | 1539325918-125438-4-git-send-email-honnappa.nagarahalli@arm.com |
|---|---|
| State | Superseded |
| Headers | show |
| Series | Address reader-writer concurrency in rte_hash | expand |
>-----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>
diff --git a/lib/librte_hash/rte_cuckoo_hash.c b/lib/librte_hash/rte_cuckoo_hash.c index 8de0de3..13646d0 100644 --- a/lib/librte_hash/rte_cuckoo_hash.c +++ b/lib/librte_hash/rte_cuckoo_hash.c @@ -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, diff --git a/lib/librte_hash/rte_cuckoo_hash.h b/lib/librte_hash/rte_cuckoo_hash.h index 5225402..b823d37 100644 --- a/lib/librte_hash/rte_cuckoo_hash.h +++ b/lib/librte_hash/rte_cuckoo_hash.h @@ -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 {