<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
