Re: [dpdk-dev] [PATCH v7 1/3] rcu: add RCU library supporting QSBR mechanism

["Ruifeng Wang (Arm Technology China)" <Ruifeng.Wang@arm.com>]

Hi Honnappa,

> -----Original Message-----
> From: dev <dev-bounces@dpdk.org> On Behalf Of Honnappa Nagarahalli
> Sent: Tuesday, April 23, 2019 12:31
> To: konstantin.ananyev@intel.com; stephen@networkplumber.org;
> paulmck@linux.ibm.com; marko.kovacevic@intel.com; dev@dpdk.org
> Cc: Honnappa Nagarahalli <Honnappa.Nagarahalli@arm.com>; Gavin Hu (Arm
> Technology China) <Gavin.Hu@arm.com>; Dharmik Thakkar
> <Dharmik.Thakkar@arm.com>; Malvika Gupta <Malvika.Gupta@arm.com>
> Subject: [dpdk-dev] [PATCH v7 1/3] rcu: add RCU library supporting QSBR
> mechanism
> 

*snip*

> +int __rte_experimental
> +rte_rcu_qsbr_thread_unregister(struct rte_rcu_qsbr *v, unsigned int
> +thread_id) {
> +	unsigned int i, id, success;
> +	uint64_t old_bmap, new_bmap;
> +
> +	if (v == NULL || thread_id >= v->max_threads) {
> +		rte_log(RTE_LOG_ERR, rcu_log_type,
> +			"%s(): Invalid input parameter\n", __func__);
> +		rte_errno = EINVAL;
> +
> +		return 1;
> +	}
> +
> +	RCU_IS_LOCK_CNT_ZERO(v, thread_id, ERR, "Lock counter %u\n",
> +				v->qsbr_cnt[thread_id].lock_cnt);
> +
> +	id = thread_id & RTE_QSBR_THRID_MASK;
> +	i = thread_id >> RTE_QSBR_THRID_INDEX_SHIFT;
> +
> +	/* Make sure that the counter for registered threads does not
> +	 * go out of sync. Hence, additional checks are required.
> +	 */
> +	/* Check if the thread is already unregistered */
> +	old_bmap = __atomic_load_n(RTE_QSBR_THRID_ARRAY_ELM(v, i),
> +					__ATOMIC_RELAXED);
> +	if (old_bmap & ~(1UL << id))

If I understand correctly, here should be (!(old_bmap & 1UL << id))
Can you please check?

> +		return 0;
> +
> +	do {
> +		new_bmap = old_bmap & ~(1UL << id);
> +		/* Make sure any loads of the shared data structure are
> +		 * completed before removal of the thread from the list of
> +		 * reporting threads.
> +		 */
> +		success = __atomic_compare_exchange(
> +					RTE_QSBR_THRID_ARRAY_ELM(v, i),
> +					&old_bmap, &new_bmap, 0,
> +					__ATOMIC_RELEASE,
> __ATOMIC_RELAXED);
> +
> +		if (success)
> +			__atomic_fetch_sub(&v->num_threads,
> +						1, __ATOMIC_RELAXED);
> +		else if (old_bmap & ~(1UL << id))

Same comment as previous.

> +			/* Someone else unregistered this thread.
> +			 * Counter should not be incremented.
> +			 */
> +			return 0;
> +	} while (success == 0);
> +
> +	return 0;
> +}

*snip*