Re: [dpdk-dev] [RFC v2 1/2] rcu: add RCU library supporting QSBR mechanism

["Ananyev, Konstantin" <konstantin.ananyev@intel.com>]

> > > > > diff --git a/lib/librte_rcu/rte_rcu_qsbr.h
> > > > > b/lib/librte_rcu/rte_rcu_qsbr.h new file mode 100644 index
> > > > > 000000000..c818e77fd
> > > > > --- /dev/null
> > > > > +++ b/lib/librte_rcu/rte_rcu_qsbr.h
> > > > > @@ -0,0 +1,321 @@
> > > > > +/* SPDX-License-Identifier: BSD-3-Clause
> > > > > + * Copyright (c) 2018 Arm Limited  */
> > > > > +
> > > > > +#ifndef _RTE_RCU_QSBR_H_
> > > > > +#define _RTE_RCU_QSBR_H_
> > > > > +
> > > > > +/**
> > > > > + * @file
> > > > > + * RTE Quiescent State Based Reclamation (QSBR)
> > > > > + *
> > > > > + * Quiescent State (QS) is any point in the thread execution
> > > > > + * where the thread does not hold a reference to shared memory.
> > > > > + * A critical section for a data structure can be a quiescent
> > > > > +state for
> > > > > + * another data structure.
> > > > > + *
> > > > > + * This library provides the ability to identify quiescent state.
> > > > > + */
> > > > > +
> > > > > +#ifdef __cplusplus
> > > > > +extern "C" {
> > > > > +#endif
> > > > > +
> > > > > +#include <stdio.h>
> > > > > +#include <stdint.h>
> > > > > +#include <errno.h>
> > > > > +#include <rte_common.h>
> > > > > +#include <rte_memory.h>
> > > > > +#include <rte_lcore.h>
> > > > > +#include <rte_debug.h>
> > > > > +
> > > > > +/**< Maximum number of reader threads supported. */ #define
> > > > > +RTE_RCU_MAX_THREADS 128
> > > > > +
> > > > > +#if !RTE_IS_POWER_OF_2(RTE_RCU_MAX_THREADS)
> > > > > +#error RTE_RCU_MAX_THREADS must be a power of 2 #endif
> > > > > +
> > > > > +/**< Number of array elements required for the bit-map */ #define
> > > > > +RTE_QSBR_BIT_MAP_ELEMS
> > (RTE_RCU_MAX_THREADS/(sizeof(uint64_t)
> > > > * 8))
> > > > > +
> > > > > +/* Thread IDs are stored as a bitmap of 64b element array. Given
> > > > > +thread id
> > > > > + * needs to be converted to index into the array and the id
> > > > > +within
> > > > > + * the array element.
> > > > > + */
> > > > > +#define RTE_QSBR_THR_INDEX_SHIFT 6 #define
> > RTE_QSBR_THR_ID_MASK
> > > > > +0x3f
> > > > > +
> > > > > +/* Worker thread counter */
> > > > > +struct rte_rcu_qsbr_cnt {
> > > > > +	uint64_t cnt; /**< Quiescent state counter. */ }
> > > > > +__rte_cache_aligned;
> > > > > +
> > > > > +/**
> > > > > + * RTE thread Quiescent State structure.
> > > > > + */
> > > > > +struct rte_rcu_qsbr {
> > > > > +	uint64_t reg_thread_id[RTE_QSBR_BIT_MAP_ELEMS]
> > > > __rte_cache_aligned;
> > > > > +	/**< Registered reader thread IDs - reader threads reporting
> > > > > +	 * on this QS variable represented in a bit map.
> > > > > +	 */
> > > > > +
> > > > > +	uint64_t token __rte_cache_aligned;
> > > > > +	/**< Counter to allow for multiple simultaneous QS queries */
> > > > > +
> > > > > +	struct rte_rcu_qsbr_cnt w[RTE_RCU_MAX_THREADS]
> > > > __rte_cache_aligned;
> > > > > +	/**< QS counter for each reader thread, counts upto
> > > > > +	 * current value of token.
> > > >
> > > > As I understand you decided to stick with neutral thread_id and let
> > > > user define what exactly thread_id is (lcore, syste, thread id, something
> > else)?
> > > Yes, that is correct. I will reply to the other thread to continue the discussion.
> > >
> > > > If so, can you probably get rid of RTE_RCU_MAX_THREADS limitation?
> > > I am not seeing this as a limitation. The user can change this if required. May
> > be I should change it as follows:
> > > #ifndef RTE_RCU_MAX_THREADS
> > > #define RTE_RCU_MAX_THREADS 128
> > > #endif
> >
> > Yep, that's better, though it would still require user to rebuild the code if he
> > would like to increase total number of threads supported.
> Agree
> 
> > Though it seems relatively simply to extend current code to support dynamic
> > max thread num here (2 variable arrays plus shift value plus mask).
> Agree, supporting dynamic 'max thread num' is simple. But this means memory needs to be allocated to the arrays. The API
> 'rte_rcu_qsbr_init' has to take max thread num as the parameter. We also have to introduce another API to free this memory. This will
> become very similar to alloc/free APIs I had in the v1.
> I hope I am following you well, please correct me if not.

I think we can still leave alloc/free tasks to the user.
We probabply just need extra function rte_rcu_qsbr_size(uint32_ max_threads)
to help user calculate required size.
rte_rcu_qsbr_init() might take as an additional parameter 'size' to make checks.
Thought about something like that:

size_t sz = rte_rcu_qsbr_size(max_threads);
struct rte_rcu_qsbr *qsbr = alloc_aligned(CACHE_LINE, sz);
rte_rcu_qsbr_init(qsbr, max_threads, sz);
...

Konstantin

> 
> >
> > >
> > > > I.E. struct rte_rcu_qsbr_cnt w[] and allow user at init time to
> > > > define max number of threads allowed.
> > > > Or something like:
> > > > #define RTE_RCU_QSBR_DEF(name, max_thread) struct name { \
> > > >   uint64_t reg_thread_id[ALIGN_CEIL(max_thread, 64)  >> 6]; \
> > > >    ...
> > > >    struct rte_rcu_qsbr_cnt w[max_thread]; \ }
> > > I am trying to understand this. I am not following why 'name' is
> > > required? Would the user call 'RTE_RCU_QSBR_DEF' in the application
> > header file?
> >
> > My thought here was to allow user to define his own structures, depending on
> > the number of max threads he needs/wants:
> > RTE_RCU_QSBR_DEF(rte_rcu_qsbr_128, 128);
> > RTE_RCU_QSBR_DEF(rte_rcu_qsbr_64, 64); ...
> Thank you for the clarification, I follow you now. However, it will not solve the problem of dynamic max thread num. Changes to the max
> number of threads will require recompilation.
> 
> > Konstantin