> -----Original Message-----
> From: Honnappa Nagarahalli <honnappa.nagaraha...@arm.com>
> Sent: Friday, May 1, 2020 3:56 PM
> To: Van Haaren, Harry <harry.van.haa...@intel.com>; Phil Yang
> <phil.y...@arm.com>; dev@dpdk.org
> Cc: tho...@monjalon.net; david.march...@redhat.com; Ananyev, Konstantin
> <konstantin.anan...@intel.com>; jer...@marvell.com;
> hemant.agra...@nxp.com; Gavin Hu <gavin...@arm.com>; nd
> <n...@arm.com>; sta...@dpdk.org; Eads, Gage <gage.e...@intel.com>;
> Richardson, Bruce <bruce.richard...@intel.com>; nd <n...@arm.com>;
> Honnappa Nagarahalli <honnappa.nagaraha...@arm.com>; nd <n...@arm.com>
> Subject: RE: [PATCH v2 1/6] service: fix race condition for MT unsafe service
>
> <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
> >
> > I've micro-benchmarked this code change inside the service cores autotest,
> > and it introduces around 35 cycles of overhead per service call. This is
> > not
> > ideal, but given it's a bugfix, and by far the simplest method to fix this
> > race-
> > condition. Having discussed and investigated multiple other solutions, I
> > believe this is the right solution.
> > Thanks Honnappa and Phil for identifying and driving a solution.
> You are welcome. Thank you for your timely responses.
Perhaps not so timely after all ... I'll review C11 patches Tuesday morning,
it's a long weekend in Ireland!
> > I suggest to post the benchmarking unit-test addition patch, and integrate
> > that
> > *before* the series under review here gets merged? This makes
> > benchmarking of the "before bugfix" performance in future easier should it
> > be
> > required.
> I do not see any issues, would be happy to review.
Thanks for volunteering, you're on CC when sent, for convenience:
http://patches.dpdk.org/patch/69651/
> I think we still have time to catch up with RC2 (May 8th).
Agree, merge into RC2 would be great.
> You had also mentioned about calling out that, the control plane APIs are not
> MT safe. Should I add that to this patch?
Yes, that'd be great.
<snip discussion details>
Cheers, -Harry
