Re: [dpdk-dev] [PATCH v2 1/2] service: add API to retrieve service core active

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

> -----Original Message-----
> From: Honnappa Nagarahalli <Honnappa.Nagarahalli@arm.com>
> Sent: Tuesday, July 21, 2020 9:24 PM
> To: David Marchand <david.marchand@redhat.com>
> Cc: Van Haaren, Harry <harry.van.haaren@intel.com>; dev@dpdk.org;
> igor.romanov@oktetlabs.ru; Yigit, Ferruh <ferruh.yigit@intel.com>; nd
> <nd@arm.com>; aconole@redhat.com; l.wojciechow@partner.samsung.com; Phil
> Yang <Phil.Yang@arm.com>; Honnappa Nagarahalli
> <Honnappa.Nagarahalli@arm.com>; nd <nd@arm.com>
> Subject: RE: [PATCH v2 1/2] service: add API to retrieve service core active
> 
> > Subject: Re: [PATCH v2 1/2] service: add API to retrieve service core active
> >
> > On Tue, Jul 21, 2020 at 9:43 PM Honnappa Nagarahalli
> > <Honnappa.Nagarahalli@arm.com> wrote:
> > > > @@ -457,6 +458,8 @@ service_runner_func(void *arg)
> > > >       const int lcore = rte_lcore_id();
> > > >       struct core_state *cs = &lcore_states[lcore];
> > > >
> > > > +     __atomic_store_n(&cs->thread_active, 1, __ATOMIC_RELAXED);
> > > Essentially, we have to ensure that all the memory operations are contained
> > within both stores (this one and the one below) to 'thread_active'.
> > > We should use __ATOMIC_SEQ_CST, which will avoid any memory
> > operations from getting hoisted above this line.
> > > Performance should not be an issue as it will get executed only when the
> > service core is started.
> > > It would be good to add comment reasoning the memory order used.

OK, will update to SEQ_CST in v3, and add comment.

> > > Also, what happens if the user calls 'rte_service_lcore_active' just before the
> > above statement is executed? It might not be a valid use case, but it is good
> > to document the race conditions and correct sequence of API calls.
> > >
> > > > +
> > > >       /* runstate act as the guard variable. Use load-acquire
> > > >        * memory order here to synchronize with store-release
> > > >        * in runstate update functions.
> > > > @@ -475,9 +478,20 @@ service_runner_func(void *arg)
> > > >               cs->loops++;
> > > >       }
> > > >
> > > > +     __atomic_store_n(&cs->thread_active, 0, __ATOMIC_RELAXED);
> > > __ATOMIC_RELEASE would be enough. But, __ATOMIC_SEQ_CST should not
> > cause any performance issues.
> >
> > But then are we missing a ACQUIRE barrier in rte_service_lcore_active?
> +1 (see below)

OK, will update to SEQ_CST in v3, with comment.


> > > > +int32_t
> > > > +rte_service_lcore_active(uint32_t lcore) {
> > > > +     if (lcore >= RTE_MAX_LCORE || !lcore_states[lcore].is_service_core)
> > > > +             return -EINVAL;
> > > > +
> > > > +     return __atomic_load_n(&lcore_states[lcore].thread_active,
> > > > +                            __ATOMIC_RELAXED); }
> Agree with David. ACQUIRE is the safer order to ensure memory operations are not
> hoisted in cases like:
> 
> if (rte_service_lcore_active(lcore) == 0) {
> 	<do something>; /* Do not allow the memory operations to hoist above 'if'
> statement */
> }

OK, will change to ACQUIRE in v3.

<snip>

> > > > +/**
> > > > + * Reports if a service lcore is currently running.
> > > > + * @retval 0 Service thread is not active, and has been returned to EAL.
> > > > + * @retval 1 Service thread is in the service core polling loop.
> > > > + * @retval -EINVAL Invalid *lcore_id* provided.
> > > > + */
> > > > +__rte_experimental
> > > > +int32_t rte_service_lcore_active(uint32_t lcore_id);
> > > Would 'rte_service_lcore_may_be_active' better? It would be inline with
> > 'rte_service_may_be_active'?

I think the implementation behind the API is different, so I think _may_be_ is not appropriate for service_lcore_active, keeping same function name for v3.

rte_service_lcore_active() checks at a particular point in the calling thread if another thread is active *at that time*. It is either active or not. This is defined, it is deterministic in that the result is either yes or no, and there is no ambiguity at any given check. You're right the value can change *just* after the check - but at the time of the check the answer was deterministic.

rte_service_may_be_active() checks if a service *could* be run by a service core. It is not deterministic. A service lcore only sets a service as "active on lcore" (or not active) when it polls it - this opens a window of nondeterministic result. When a runstate is set to off, there is a window of "unknown" before we know certainly that the service is not run on a service core anymore. That is why I believe the _may_be_ is appropriate for this API, it shows this non determinism.

> > > I think we need additional documentation for 'rte_service_lcore_stop' to
> > indicate that the caller should not assume that the service thread on the lcore
> > has stopped running and should call the above API to check.
> >
> > +1
> > Additional documentation can't hurt.

Will add a section to the _stop() and _lcore_active() in doxygen for v3.