Re: [dpdk-dev] Questions on service core feature implementation

["Van Haaren, Harry" <harry.van.haaren@intel.com>]

> -----Original Message-----
> From: Honnappa Nagarahalli <Honnappa.Nagarahalli@arm.com>
> Sent: Saturday, March 7, 2020 1:15 AM
> To: Van Haaren, Harry <harry.van.haaren@intel.com>
> Cc: dev@dpdk.org; Phil Yang <Phil.Yang@arm.com>; Gavin Hu
> <Gavin.Hu@arm.com>; nd <nd@arm.com>; nd <nd@arm.com>
> Subject: Questions on service core feature implementation
> 
> Hi Harry,

Hey Honnappa,

> 	I have few observations on service core feature implementation.
> 
> I believe it is allowed to map/unmap a lcore to service while the service is
> running (i.e. not just during initialization stage). Please clarify
> otherwise.

Service-cores wasn't designed for this use case, but this "popped up"
as the implementation was being finalized. Service cores' solves the problem
of HW vs SW PMDs lcore requirements being different - and service cores is the
abstraction to hide that lcore requirement difference.

So I don't think we made a very conscious decision on if this is allowed or not.


> Assuming yes -
> 
> In the 'service_run' function, the code to detect use of atomics seems to
> result in allowing the mt-unsafe service to run on multiple cores. Looking
> at the following code:
> 
> 1   const int use_atomics = (service_mt_safe(s) == 0) &&
> 2                (rte_atomic32_read(&s->num_mapped_cores) > 1);
> 3   if (use_atomics) {
> 4                if (!rte_atomic32_cmpset((uint32_t *)&s->execute_lock, 0,
> 1))
> 5                        return -EBUSY;
> 6
> 7                rte_service_runner_do_callback(s, cs, i);
> 8                rte_atomic32_clear(&s->execute_lock);
> 9   } else
> 10             rte_service_runner_do_callback(s, cs, i);
> 
> Let us say, on core1, after line 4, 'use_atomics' is set to 0. core1 is
> running the service function.
> On the control plane let us say 'rte_service_map_lcore_set' is called to map
> the service to an additional core, core2.
> Now, on core2, after line 4, 'use_atomics' is set to 1. But since core1 did
> not take execute_lock, core2 will take the lock and starts running the
> service function. This should not be allowed since the service is a mt-
> unsafe service.
>
> Please let me know if my assessment is correct.
> A possible solution is to take the execute_lock all the time.

Yes agreed that your scenario seems to show an inconsistency.
As per above, I'm not sure this use case was really the target of
the service-cores design - but we as a community should identify
if we want to support this use case.

We could either update docs explicitly dis-allowing the above
situation - this puts the burden on applications to understand
what services could be running, and if they're being scheduled already.

The other solution being to change the implementation to allow this
use-case, which will likely have a performance impact for those already
using service cores for its original function, by (as you suggested)
taking execute_lock all the time (and paying the atomic cost for that..)


> Thank you,
> Honnappa

Regards, -HvH