> 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.nagaraha...@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
