[dpdk-dev] Questions on service core feature implementation

[dpdk-dev] Questions on service core feature implementation

[Honnappa Nagarahalli]

Hi Harry,
	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.

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.

Thank you,
Honnappa

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

[Van Haaren, Harry]

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

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

[Honnappa Nagarahalli]

<snip>

> > Hi Harry,
> 
> Hey Honnappa,
Thanks for your answers.

> 
> > 	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.
Agree with you, it will be difficult for the application to determine how the service is scheduled.

> 
> 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..)
'num_mapped_cores' indicates the number of cores the service can run on. But, to be able to effectively avoid the use of atomic instructions, we need to know if a service is 'currently' running on another core. This information is not maintained by the library I think maintaining that will also introduce additional performance impacts. So, currently,
a) the atomics are avoided only when the user configures MT-Unsafe service to run on a single core.
b) if the user really wants to allow a MT-Unsafe service to run on multiple cores non-concurrently, atomics are used.

I suggest the following changes:
a) library will use atomics if the service is MT-Unsafe irrespective of the value of 'num_mapped_cores'
b) if the user wants to run a MT-Unsafe service on a single core and avoid using atomics, then he should set the service capability to RTE_SERVICE_CAP_MT_SAFE (This might result in some performance improvements since 'num_mapped_cores' will not be checked anymore).

Impact on the current users
a) if they have configured MT-Unsafe service to run on multiple cores, then they are already taking the hit of atomic instructions (and some additional cost of reading/checking the 'num_mapped_cores', they should see some perf improvements). No impact here.
b) if they have configured MT-Unsafe service to run on single core, then they might experience additional performance penalty. It might not be full penalty of using atomics as we will be removing some instructions. This needs to be measured. They can regain the performance loss by configuring their service RTE_SERVICE_CAP_MT_SAFE.

> 
> 
> > Thank you,
> > Honnappa
> 
> Regards, -HvH