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

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

> <snip>
> 
> > > > > > > +/**
> > > > > > > + * 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.
> The size is returned by an API provided by the library. Why does it need to be validated again? If 'size' is required for rte_rcu_qsbr_init, it
> could calculate it again.

Just as extra-safety check.
I don't have strong opinion here - if you think it is overkill, let's drop it.


> 
> > 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); ...
> >
> Do you see any advantage for allowing the user to allocate the memory?
So user can choose where to allocate the memory (eal malloc, normal malloc, stack, something else).
Again user might decide to make rcu part of some complex data structure - 
in that case he probably would like to allocate one big chunk of memory at once and then
provide part of it for rcu.
Or some other usage scenario that I can't predict.

> This approach requires the user to call 3 APIs (including memory allocation). These 3 can be abstracted in a rte_rcu_qsbr_alloc API, user has
> to call just 1 API.
> 
> > 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