[dpdk-stable] [PATCH v2 1/6] service: fix race condition for MT unsafe service

[Van Haaren, Harry]

> -----Original Message-----
> From: Honnappa Nagarahalli <Honnappa.Nagarahalli at arm.com>
> Sent: Friday, May 1, 2020 3:56 PM
> To: Van Haaren, Harry <harry.van.haaren at intel.com>; Phil Yang
> <Phil.Yang at arm.com>; dev at dpdk.org
> Cc: thomas at monjalon.net; david.marchand at redhat.com; Ananyev, Konstantin
> <konstantin.ananyev at intel.com>; jerinj at marvell.com;
> hemant.agrawal at nxp.com; Gavin Hu <Gavin.Hu at arm.com>; nd
> <nd at arm.com>; stable at dpdk.org; Eads, Gage <gage.eads at intel.com>;
> Richardson, Bruce <bruce.richardson at intel.com>; nd <nd at arm.com>;
> Honnappa Nagarahalli <Honnappa.Nagarahalli at arm.com>; nd <nd at 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.nagarahalli at 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: stable at dpdk.org
> > > > >
> > > > > Signed-off-by: Honnappa Nagarahalli <honnappa.nagarahalli at arm.com>
> > > > > Reviewed-by: Phil Yang <phil.yang at 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