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

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

> 
> On Tue, Apr 16, 2019 at 11:13:57PM -0500, Honnappa Nagarahalli wrote:
> > Add RCU library supporting quiescent state based memory reclamation
> method.
> > This library helps identify the quiescent state of the reader threads
> > so that the writers can free the memory associated with the lock less
> > data structures.
> >
> > Signed-off-by: Honnappa Nagarahalli <honnappa.nagarahalli@arm.com>
> > Reviewed-by: Steve Capper <steve.capper@arm.com>
> > Reviewed-by: Gavin Hu <gavin.hu@arm.com>
> > Reviewed-by: Ola Liljedahl <ola.liljedahl@arm.com>
> > Acked-by: Konstantin Ananyev <konstantin.ananyev@intel.com>
> 
> Looks much better!
> 
> One more suggestion below, on rte_rcu_qsbr_thread_offline().
> 
> 						Thanx, Paul
> 

<snip>

> > diff --git a/lib/librte_rcu/rte_rcu_qsbr.h
> > b/lib/librte_rcu/rte_rcu_qsbr.h new file mode 100644 index
> > 000000000..73fa3354e
> > --- /dev/null
> > +++ b/lib/librte_rcu/rte_rcu_qsbr.h
> > @@ -0,0 +1,629 @@
> > +/* SPDX-License-Identifier: BSD-3-Clause
> > + * Copyright (c) 2018 Arm Limited
> > + */
> > +
> > +#ifndef _RTE_RCU_QSBR_H_
> > +#define _RTE_RCU_QSBR_H_
> > +

<snip>

> > +
> > +/**
> > + * @warning
> > + * @b EXPERIMENTAL: this API may change without prior notice
> > + *
> > + * Add a registered reader thread, to the list of threads reporting
> > +their
> > + * quiescent state on a QS variable.
> > + *
> > + * This is implemented as a lock-free function. It is multi-thread
> > + * safe.
> > + *
> > + * Any registered reader thread that wants to report its quiescent
> > +state must
> > + * call this API before calling rte_rcu_qsbr_quiescent. This can be
> > +called
> > + * during initialization or as part of the packet processing loop.
> > + *
> > + * The reader thread must call rte_rcu_thread_offline API, before
> > + * calling any functions that block, to ensure that
> > +rte_rcu_qsbr_check
> > + * API does not wait indefinitely for the reader thread to update its QS.
> > + *
> > + * The reader thread must call rte_rcu_thread_online API, after the
> > +blocking
> > + * function call returns, to ensure that rte_rcu_qsbr_check API
> > + * waits for the reader thread to update its quiescent state.
> > + *
> > + * @param v
> > + *   QS variable
> > + * @param thread_id
> > + *   Reader thread with this thread ID will report its quiescent state on
> > + *   the QS variable.
> > + */
> > +static __rte_always_inline void __rte_experimental
> > +rte_rcu_qsbr_thread_online(struct rte_rcu_qsbr *v, unsigned int
> > +thread_id) {
> > +	uint64_t t;
> > +
> > +	RTE_ASSERT(v != NULL && thread_id < v->max_threads);
> > +
> > +	/* Copy the current value of token.
> > +	 * The fence at the end of the function will ensure that
> > +	 * the following will not move down after the load of any shared
> > +	 * data structure.
> > +	 */
> > +	t = __atomic_load_n(&v->token, __ATOMIC_RELAXED);
> > +
> > +	/* __atomic_store_n(cnt, __ATOMIC_RELAXED) is used to ensure
> > +	 * 'cnt' (64b) is accessed atomically.
> > +	 */
> > +	__atomic_store_n(&v->qsbr_cnt[thread_id].cnt,
> > +		t, __ATOMIC_RELAXED);
> > +
> > +	/* The subsequent load of the data structure should not
> > +	 * move above the store. Hence a store-load barrier
> > +	 * is required.
> > +	 * If the load of the data structure moves above the store,
> > +	 * writer might not see that the reader is online, even though
> > +	 * the reader is referencing the shared data structure.
> > +	 */
> > +#ifdef RTE_ARCH_X86_64
> > +	/* rte_smp_mb() for x86 is lighter */
> > +	rte_smp_mb();
> > +#else
> > +	__atomic_thread_fence(__ATOMIC_SEQ_CST);
> > +#endif
> > +}
> > +
> > +/**
> > + * @warning
> > + * @b EXPERIMENTAL: this API may change without prior notice
> > + *
> > + * Remove a registered reader thread from the list of threads
> > +reporting their
> > + * quiescent state on a QS variable.
> > + *
> > + * This is implemented as a lock-free function. It is multi-thread
> > + * safe.
> > + *
> > + * This can be called during initialization or as part of the packet
> > + * processing loop.
> > + *
> > + * The reader thread must call rte_rcu_thread_offline API, before
> > + * calling any functions that block, to ensure that
> > +rte_rcu_qsbr_check
> > + * API does not wait indefinitely for the reader thread to update its QS.
> > + *
> > + * @param v
> > + *   QS variable
> > + * @param thread_id
> > + *   rte_rcu_qsbr_check API will not wait for the reader thread with
> > + *   this thread ID to report its quiescent state on the QS variable.
> > + */
> > +static __rte_always_inline void __rte_experimental
> > +rte_rcu_qsbr_thread_offline(struct rte_rcu_qsbr *v, unsigned int
> > +thread_id) {
> > +	RTE_ASSERT(v != NULL && thread_id < v->max_threads);
> 
> I suggest adding an assertion that v->qsbr_cnt[thread_id].lock_cnt is equal to
> zero.  This makes it easier to find a misplaced rte_rcu_qsbr_thread_offline().
> Similar situation as the assertion that you added to rte_rcu_qsbr_quiescent().
> 
Agree, will add that. I think there is value in adding similar check to rte_rcu_qsbr_thread_online and rte_rcu_qsbr_thread_unregister as well.
Adding a check to rte_rcu_qsbr_thread_register does not hurt.

> > +
> > +	/* The reader can go offline only after the load of the
> > +	 * data structure is completed. i.e. any load of the
> > +	 * data strcture can not move after this store.
> > +	 */
> > +
> > +	__atomic_store_n(&v->qsbr_cnt[thread_id].cnt,
> > +		RTE_QSBR_CNT_THR_OFFLINE, __ATOMIC_RELEASE); }
> > +

<snip>