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

[Stephen Hemminger <stephen@networkplumber.org>]

On Tue, 16 Apr 2019 05:29:21 +0000
Honnappa Nagarahalli <Honnappa.Nagarahalli@arm.com> wrote:

> > > > > > > > On Fri, 12 Apr 2019 15:20:37 -0500 Honnappa Nagarahalli
> > > > > > > > <honnappa.nagarahalli@arm.com> 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>  
> > > > > > > >
> > > > > > > > After evaluating long term API/ABI issues, I think you need
> > > > > > > > to get rid of almost all use of inline and visible
> > > > > > > > structures. Yes it might be marginally slower, but you thank me  
> > the first time you have to fix something.  
> > > > > > > >  
> > > > > > > Agree, I was planning on another version to address this (I am yet  
> > to take a look at your patch addressing the ABI).  
> > > > > > > The structure visibility definitely needs to be addressed.
> > > > > > > For the inline functions, is the plan to convert all the
> > > > > > > inline functions in DPDK? If yes, I think we need to consider
> > > > > > > the performance  
> > > > > > difference. May be consider L3-fwd application, change all the  
> > inline functions in its path and run a test?  
> > > > > >
> > > > > > Every function that is not in the direct datapath should not be  
> > inline.  
> > > > > > Exceptions or things like rx/tx burst, ring enqueue/dequeue, and
> > > > > > packet alloc/free  
> I do not understand how DPDK can claim ABI compatibility if we have inline functions (unless we freeze any development in these inline functions forever).
> 
> > > > >
> > > > > Plus synchronization routines: spin/rwlock/barrier, etc.
> > > > > I think rcu should be one of such exceptions - it is just another
> > > > > synchronization mechanism after all (just a bit more sophisticated).
> > > > > Konstantin  
> > > >
> > > > If you look at the other userspace RCU, you wil see that the only
> > > > inlines are the rcu_read_lock,rcu_read_unlock and  
> > rcu_reference/rcu_assign_pointer.  
> > > >
> > > > The synchronization logic is all real functions.  
> > >
> > > In fact, I think urcu provides both flavors:
> > > https://github.com/urcu/userspace-  
> > rcu/blob/master/include/urcu/static/  
> > > urcu-qsbr.h I still don't understand why we have to treat it
> > > differently then let say spin-lock/ticket-lock or rwlock.
> > > If we gone all the way to create our own version of rcu, we probably
> > > want it to be as fast as possible (I know that main speedup should
> > > come from the fact that readers don't have to wait for writer to
> > > finish, but still...)
> > >
> > > Konstantin
> > >  
> > 
> > Having locking functions inline is already a problem in current releases.
> > The implementation can not be improved without breaking ABI (or doing
> > special workarounds like lock v2)  
> I think ABI and inline function discussion needs to be taken up in a different thread.
> 
> Currently, I am looking to hide the structure visibility. I looked at your patch [1], it is a different case than what I have in this patch. It is a pretty generic use case as well (similar situation exists in other libraries). I think a generic solution should be agreed upon.
> 
> If we have to hide the structure content, the handle to QS variable returned to the application needs to be opaque. I suggest using 'void *' behind which any structure can be used.
> 
> typedef void * rte_rcu_qsbr_t;
> typedef void * rte_hash_t;
> 
> But it requires typecasting.
> 
> [1] http://patchwork.dpdk.org/cover/52609/

C allows structure to be defined without knowing what is in it
therefore.

typedef struct rte_rcu_qsbr rte_rcu_qsbr_t;

is preferred (or do it without typedef)

struct rte_rcu_qsbr;