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

[Honnappa Nagarahalli <Honnappa.Nagarahalli@arm.com>]

> 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.
> >
> > 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)

> 
> 
> >
> > >       return 0;
> > >  }
> > >
> > > +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 */
}

> > > +
> > >  int32_t
> > >  rte_service_lcore_count(void)
> > >  {
> > > diff --git a/lib/librte_eal/include/rte_service.h
> > > b/lib/librte_eal/include/rte_service.h
> > > index e2d0a6dd3..f7134b5c0 100644
> > > --- a/lib/librte_eal/include/rte_service.h
> > > +++ b/lib/librte_eal/include/rte_service.h
> > > @@ -261,6 +261,15 @@ int32_t rte_service_lcore_start(uint32_t lcore_id);
> > >   */
> > >  int32_t rte_service_lcore_stop(uint32_t lcore_id);
> > >
> > > +/**
> > > + * 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 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.
> 
> 
> --
> David Marchand