Hi Harry,
Thanks for getting back on this.
<snip>
> > Subject: [PATCH v2 1/6] service: fix race condition for MT unsafe
> > service
> >
> > From: Honnappa Nagarahalli <honnappa.nagaraha...@arm.com>
> >
> > The MT unsafe service might get configured to run on another core
> > while the service is running currently. This might result in the MT
> > unsafe service running on multiple cores simultaneously. Use
> > 'execute_lock' always when the service is MT unsafe.
> >
> > Fixes: e9139a32f6e8 ("service: add function to run on app lcore")
> > Cc: sta...@dpdk.org
> >
> > Signed-off-by: Honnappa Nagarahalli <honnappa.nagaraha...@arm.com>
> > Reviewed-by: Phil Yang <phil.y...@arm.com>
> > ---
>
> Thanks for spinning a new revision - based on ML discussion previously, it
> seems like the "use service-run-count" to avoid this race would be a complex
> solution.
>
> Suggesting the following;
> 1) Take the approach as per this patch, to always take the atomic, fixing the
> race condition.
Ok
> 2) Add an API to service-cores, which allows "committing" of mappings.
> Committing the mapping would imply that the mappings will not be changed
> in future. With runtime-remapping being removed from the equation, the
> existing branch-over-atomic optimization is valid again.
Ok. Just to make sure I understand this:
a) on the data plane, if commit API is called (probably a new state variable)
and num_mapped_cores is set to 1, there is no need to take the lock.
b) possible implementation of the commit API would check if num_mapped_cores
for the service is set to 1 and set a variable to indicate that the lock is not
required.
What do you think about asking the application to set the service capability
to MT_SAFE if it knows that the service will run on a single core? This would
require us to change the documentation and does not require additional code.
>
> So this would offer applications two situations
> A) No application change: possible performance regression due to atomic
> always taken.
> B) Call "commit" API, and regain the performance as per previous DPDK
> versions.
>
> Thoughts/opinions on the above? I've flagged the rest of the patchset for
> review ASAP. Regards, -Harry
>
> > lib/librte_eal/common/rte_service.c | 11 +++++------
> > 1 file changed, 5 insertions(+), 6 deletions(-)
> >
> > diff --git a/lib/librte_eal/common/rte_service.c
> > b/lib/librte_eal/common/rte_service.c
> > index 70d17a5..b8c465e 100644
> > --- a/lib/librte_eal/common/rte_service.c
> > +++ b/lib/librte_eal/common/rte_service.c
> > @@ -50,6 +50,10 @@ struct rte_service_spec_impl {
> > uint8_t internal_flags;
> >
> > /* per service statistics */
> > + /* Indicates how many cores the service is mapped to run on.
> > + * It does not indicate the number of cores the service is running
> > + * on currently.
> > + */
> > rte_atomic32_t num_mapped_cores;
> > uint64_t calls;
> > uint64_t cycles_spent;
> > @@ -370,12 +374,7 @@ service_run(uint32_t i, struct core_state *cs,
> > uint64_t service_mask,
> >
> > cs->service_active_on_lcore[i] = 1;
> >
> > - /* check do we need cmpset, if MT safe or <= 1 core
> > - * mapped, atomic ops are not required.
> > - */
> > - const int use_atomics = (service_mt_safe(s) == 0) &&
> > - (rte_atomic32_read(&s-
> >num_mapped_cores) > 1);
> > - if (use_atomics) {
> > + if (service_mt_safe(s) == 0) {
> > if (!rte_atomic32_cmpset((uint32_t *)&s->execute_lock, 0, 1))
> > return -EBUSY;
> >
> > --
> > 2.7.4
