Re: [dpdk-dev] [PATCH v3 12/12] service: relax barriers with C11 atomic operations

["Van Haaren, Harry" <harry.van.haaren@intel.com>]

> -----Original Message-----
> From: Honnappa Nagarahalli <Honnappa.Nagarahalli@arm.com>
> Sent: Monday, April 6, 2020 6:06 PM
> To: Van Haaren, Harry <harry.van.haaren@intel.com>; Phil Yang
> <Phil.Yang@arm.com>; thomas@monjalon.net; Ananyev, Konstantin
> <konstantin.ananyev@intel.com>; stephen@networkplumber.org;
> maxime.coquelin@redhat.com; dev@dpdk.org
> Cc: david.marchand@redhat.com; jerinj@marvell.com; hemant.agrawal@nxp.com;
> Gavin Hu <Gavin.Hu@arm.com>; Ruifeng Wang <Ruifeng.Wang@arm.com>; Joyce Kong
> <Joyce.Kong@arm.com>; nd <nd@arm.com>; Honnappa Nagarahalli
> <Honnappa.Nagarahalli@arm.com>; nd <nd@arm.com>
> Subject: RE: [PATCH v3 12/12] service: relax barriers with C11 atomic
> operations
> 
> <snip>
> Just to get us on the same page on 'techniques to communicate data from writer
> to reader' (apologies if it is too trivial)
> 
> Let us say that the writer has 512B (key point is - cannot be written
> atomically) that needs to be communicated to the reader.
> 
> Since the data cannot be written atomically, we need a guard variable (which
> can be written atomically, can be a flag or pointer to data). So, the writer
> will store 512B in non-atomic way and write to guard variable with release
> memory order. So, if the guard variable is valid (set in the case of flag or
> not null in the case of pointer), it guarantees that 512B is written.
> 
> The reader will read the guard variable with acquire memory order and read the
> 512B data only if the guard variable is valid. So, the acquire memory order on
> the guard variable guarantees that the load of 512B does not happen before the
> guard variable is read. The validity check on the guard variable guarantees
> that 512B was written before it was read.
> 
> The store(guard_variable, RELEASE) on the writer and the load(guard_variable,
> ACQUIRE) can be said as synchronizing with each other.
> 
> (the guard variable technique applies even if we are not using C11 atomics)

Yep agreed on the above.


> Let us say that the writer has 4B (key point is - can be written atomically)
> that needs to be communicated to the reader. The writer is free to write this
> atomically with no constraints on memory ordering as long as this data is not
> acting as a guard variable for any other data.
> 
> In my understanding, the sequence of APIs to call to start a service (writer)
> are as follows:
> 1) rte_service_init
> 2) rte_service_component_register
> 3) <possible configuration of the service>
> 4) rte_service_component_runstate_set (the reader is allowed at this point to
> read the information about the service - written by
> rte_service_component_register API. This API should not be called before
> rte_service_component_register)
> 5) <possible configuration of the service>
> 6) rte_service_runstate_set (the reader is allowed at this point to read the
> information about the service - written by rte_service_component_register API
> and run the service. This API can be called anytime. But, the reader should
> not attempt to run the service before this API is called)
> 7) rte_lcore_service_add (multiple of these probably, can be called before
> this, can't be called later)
> 8) rte_service_map_lcore_set (this can be called anytime. Can be called even
> if the service is not registered)
> 9) rte_service_lcore_start (again, this can be called anytime, even before the
> service is registered)

I think this can be simplified, if we look at calling threads:
 - one thread is the writer/config thread, and is allowed to call anything
 --- Any updates/changes must be atomically correct in how the other threads can read state.

 - service lcores, which fundamentally spin in run(), and call services mapped to it
 --- here we need to ensure any service mapped to it is atomic, and the service is valid to run.

 - other application threads using "run on app lcore" function
 --- similar to service lcore, check for service in valid state, and allow to run.


Services are not allowed to be unregistered e.g. while running.
I'd like to avoid the "explosion of combinations" by enforcing
simple limitations (and documenting them more clearly if/where required).


> So, there are 2 guard variables - 'comp_runstate' and 'app_runstate'. Only
> these 2 need to have RELEASE ordering in writer and ACQUIRE ordering in
> reader.
> 
> We can write test cases with different orders of these API calls to prove that
> the memory orders we use are sufficient.
> 
> Few comments are inline based on this assessment.

Sure. As per other email thread, splitting changes into bugfix/cleanup/C11
would likely help to try keep track of changes etc required per patch, its
getting hard to follow the various topics being discussed in parallel.


> > Subject: RE: [PATCH v3 12/12] service: relax barriers with C11 atomic
> > operations
> >
> > > From: Phil Yang <phil.yang@arm.com>
> > > Sent: Tuesday, March 17, 2020 1:18 AM
> > > To: thomas@monjalon.net; Van Haaren, Harry
> > > <harry.van.haaren@intel.com>; Ananyev, Konstantin
> > > <konstantin.ananyev@intel.com>; stephen@networkplumber.org;
> > > maxime.coquelin@redhat.com; dev@dpdk.org
> > > Cc: david.marchand@redhat.com; jerinj@marvell.com;
> > > hemant.agrawal@nxp.com; Honnappa.Nagarahalli@arm.com;
> > > gavin.hu@arm.com; ruifeng.wang@arm.com; joyce.kong@arm.com;
> > nd@arm.com
> > > Subject: [PATCH v3 12/12] service: relax barriers with C11 atomic
> > > operations
> > >
> > > To guarantee the inter-threads visibility of the shareable domain, it
> > > uses a lot of rte_smp_r/wmb in the service library. This patch relaxed
> > > these barriers for service by using c11 atomic one-way barrier operations.
> > >
> > > Signed-off-by: Phil Yang <phil.yang@arm.com>
> > > Reviewed-by: Ruifeng Wang <ruifeng.wang@arm.com>
> > > Reviewed-by: Gavin Hu <gavin.hu@arm.com>
> > > ---
> > >  lib/librte_eal/common/rte_service.c | 45

<snip patch review comments>