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?
>
> > 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);
> > +}
> > +
> > 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
