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

[David Marchand <david.marchand@redhat.com>]

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?


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