[prev in list] [next in list] [prev in thread] [next in thread] 

List:       dpdk-dev
Subject:    Re: [dpdk-dev] [PATCH 2/4] hash: add memory ordering to avoid race	conditions
From:       Honnappa Nagarahalli <Honnappa.Nagarahalli () arm ! com>
Date:       2018-09-30 22:20:59
Message-ID: AM6PR08MB3672DF53627F2EBE0000CF7398EE0 () AM6PR08MB3672 ! eurprd08 ! prod ! outlook ! com
[Download RAW message or body]

> 
> Some general comments for  the various __atomic_store/load added,
> 
> 1. Although it passes the compiler check, but I just want to confirm that if we
> should use GCC/clang builtins, or if There are higher level APIs in DPDK to do
> atomic operations?
> 
I have used gcc builtins (just like rte_ring does)

> 2. We believe compiler will translate the atomic_store/load to regular MOV
> instruction on Total Store Order architecture (e.g. X86_64). But we run the
> perf test on x86 and here is the relative slowdown on lookup comparing to
> master head. I am not sure if the performance drop comes from the atomic
> buitins.
> 
C11 atomics also block compiler reordering. Other than this, the retry loop is an \
addition to lookup. The patch also has the alignment corrected. I am not sure how is \
that affecting the perf numbers.

> Keysize | single lookup | bulk lookup
> 4 | 0.93 | 0.95
> 8 | 0.95 | 0.96
> 16 | 0.97 | 0.96
> 32 | 0.97 | 1.00
> 48 | 1.03 | 0.99
> 64 | 1.04 | 0.98
> 9 | 0.91 | 0.96
> 13 | 0.97 | 0.98
> 37 | 1.04 | 1.03
> 40 | 1.02 | 0.98
> 
I assume this is the data from the test cases in test_hash_perf.c file. I tried to \
reproduce this data, but my data is worse. Can you specify the actual test from \
test_hash_perf.c you are using (With locks/Pre-computed hash/With data/Elements in \
primary)? IMO, the differences you have provided are not high.

> Other comments inlined.
> 
> > -----Original Message-----
> > Only race condition that can occur is -  using the key store element
> > before the key write is completed. Hence, while inserting the element
> > the release memory order is used. Any other race condition is caught by
> > the key comparison. Memory orderings are added only where needed.
> > For ex: reads in the writer's context do not need memory ordering as
> > there is a single writer.
> > 
> [Wang, Yipeng] I assume this commit works together with the next one to
> enable read-write concurrency on relaxed memory ordering machine. This
> commit by itself does not do that, right?
> If this is the case, should we merge the two commits, or maybe just explain it
> a little bit more in the commit message?
This commit works only with the next commit. I kept it separated to make it easy for \
review. I will merge it in the final patch.

> 
> > key_idx in the bucket entry and pdata in the key store element are used
> > for synchronisation. key_idx is used to release an inserted entry in
> > the bucket to the reader. Use of pdata for synchronisation is required
> > due to updation of an existing entry where-in only the pdata is updated
> > without updating key_idx.
> > 
> > 
> > -/* Search a key from bucket and update its data */
> > +/* Search a key from bucket and update its data.
> > + * Writer holds the lock before calling this.
> > + */
> > static inline int32_t
> > search_and_update(const struct rte_hash *h, void *data, const void *key,
> > 	struct rte_hash_bucket *bkt, hash_sig_t sig, hash_sig_t alt_hash) @@
> > -499,8 +501,13 @@ search_and_update(const struct rte_hash *h, void *data,
> const void *key,
> > 			k = (struct rte_hash_key *) ((char *)keys +
> > 					bkt->key_idx[i] * h->key_entry_size);
> > 			if (rte_hash_cmp_eq(key, k->key, h) == 0) {
> > -				/* Update data */
> > -				k->pdata = data;
> > +				/* 'pdata' acts as the synchronization point
> > +				 * when an existing hash entry is updated.
> > +				 * Key is not updated in this case.
> > +				 */
> [Wang, Yipeng] I did not quite understand why do we need synchronization
> for hash data update.
> Since pdata write is already atomic, the lookup will either read out the stale
> data or the new data, which should be fine without synchronization.
> Is it to ensure the order of multiple reads in lookup threads?
Yes, it is to ensure the order of reads in the lookup threads. There might be reads \
in the application and we want to make sure they do not get reordered with this.

> 
> > @@ -1243,11 +1289,19 @@ __rte_hash_lookup_bulk(const struct rte_hash
> *h, const void **keys,
> > 		while (sec_hitmask[i]) {
> > 			uint32_t hit_index = __builtin_ctzl(sec_hitmask[i]);
> > 
> > -			uint32_t key_idx = secondary_bkt[i]-
> > key_idx[hit_index];
> > +			uint32_t key_idx =
> > +			__atomic_load_n(
> > +				&secondary_bkt[i]->key_idx[hit_index],
> > +				__ATOMIC_ACQUIRE);
> > 			const struct rte_hash_key *key_slot =
> > 				(const struct rte_hash_key *)(
> > 				(const char *)h->key_store +
> > 				key_idx * h->key_entry_size);
> > +
> > +			if (key_idx != EMPTY_SLOT)
> [Wang, Yipeng] I think even for current code, we need to check empty_slot.
> Could you export this as a bug fix commit?
> 
In the existing code, there is check 'if (!!key_idx & !rte_hash....)'. Are you \
referring to '!!key_idx'? I think this should be changed to '(key_idx != \
EMPTY_SLOT)'.


[prev in list] [next in list] [prev in thread] [next in thread]