> From: Jerin Jacob [mailto:jerin.ja...@caviumnetworks.com]
<snip>
> > This is the third iteration of the service core concept,
> > now with an implementation.
> >
> > Signed-off-by: Harry van Haaren <harry.van.haa...@intel.com>
>
> Nice work. Detailed review comments below
Thanks for the (prompt!) feedback. Mostly agree, comments inline, <snip> lots
of noisy code out :)
I have a follow up question on service-core usage, and how we can work e.g
Eventdev PMDs into Service cores. I'll kick off a new thread on the mailing
list to discuss it.
Patchset v2 on the way soon.
> > [keepalive] (@ref rte_keepalive.h),
> > + [Service Cores] (@ref rte_service.h),
>
> 1) IMO, To keep the consistency we can rename to "[service cores]"
Done.
> 2) I thought, we decided to expose rte_service_register() and
> rte_service_unregister() as well, Considering the case where even application
> as register for service functions if required. If it is true then I
> think, registration functions can moved of private header file so that
> it will visible in doxygen.
To avoid bleeding implementation out of the API, this was not done. Services
register API is not currently publicly visible - this keeps the API abstraction
very powerful. If we decide to make the register-struct public, we lost
(almost) all of the API encapsulation, as the struct itself has to be public
Applications can #include <rte_service_private.h> if they insist - which would
provide the functionality as desired, but then the application is aware that it
is using DPDK private data structures.
I suggest we leave the service register API as private for this release. We can
always move it to public if required - once we are more comfortable with the
API and it is more widely implemented. This will help keep API/ABI stability -
we don't have the "luxury" of EXPERIMENTAL tag in EAL :D
> 3) Should we change core function name as lcore like
> rte_service_lcore_add(), rte_service_lcore_del() etc as we are operating
> on lcore here.
Yep, done. Added "l" to all core related functions for consistency, so lcore is
now used everywhere.
> > struct rte_config {
> > uint32_t master_lcore; /**< Id of the master lcore */
> > uint32_t lcore_count; /**< Number of available logical cores. */
> > + uint32_t score_count; /**< Number of available service cores. */
>
> Should we call it as service core or service lcore?
Done
> > +/** Return the number of services registered.
> > + *
> > + * The number of services registered can be passed to
> > *rte_service_get_by_id*,
> > + * enabling the application to retireve the specificaion of each service.
>
> s/retireve the specificaion/retrieve the specification
>
> > + *
> > + * @return The number of services registered.
> > + */
> > +uint32_t rte_service_get_count(void);
> > +
> > +/** Return the specificaion of each service.
>
> s/specificaion/specification
Fixed
> > +/* Check if a service has a specific capability.
> Missing the doxygen marker(ie. change to /** Check)
Fixed
> > +/* Start a service core.
> Missing the doxygen marker(ie. change to /** Start)
Fixed
> > +/** Retreve the number of service cores currently avaialble.
> typo: ^^^^^^^^ ^^^^^^^^^^
> Retrieve the number of service cores currently available.
Oh my do I have talent for mis-spelling :D Fixed
> > + * @param array An array of at least N items.
>
> @param [out] array An array of at least n items
>
> > + * @param The size of *array*.
>
> @param n The size of *array*.
Done!
> > + /** NUMA socket ID that this service is affinitized to */
> > + int8_t socket_id;
>
> All other places socket_id is of type "int".
Done
> > +/** Private function to allow EAL to initialied default mappings.
>
> typo: ^^^^^^^^^^^
Fixed
> > +#define RTE_SERVICE_FLAG_REGISTERED_SHIFT 0
>
> Internal macro, Can be shorten to reduce the length(SERVICE_F_REGISTERED?)
>
> > +
> > +#define RTE_SERVICE_RUNSTATE_STOPPED 0
> > +#define RTE_SERVICE_RUNSTATE_RUNNING 1
>
> Internal macro, Can be shorten to reduce the length(SERVICE_STATE_RUNNING?)
These are used for services and for lcore state, so just used RUNSTATE_RUNNING
and RUNSTATE_STOPPED.
> > +struct rte_service_spec_impl {
> > + /* public part of the struct */
> > + struct rte_service_spec spec;
>
> Nice approach.
<snip>
> Since it been used in fastpath. better to align to cache line
Done :)
> > +struct core_state {
<snip>
> aligned to cache line?
Done
> > +static uint32_t rte_service_count;
> > +static struct rte_service_spec_impl rte_services[RTE_SERVICE_NUM_MAX];
> > +static struct core_state cores_state[RTE_MAX_LCORE];
>
> Since these variable are used in fastpath, better to allocate form
> huge page area. It will avoid lot of global variables in code as well.
> Like other module, you can add a private function for service init and it can
> be
> called from eal_init()
Yep good point, done.
> > +static int
>
> static inline int
> > +service_valid(uint32_t id) {
Done
> > +int32_t
>
> bool could be enough here
>
> > +rte_service_probe_capability(const struct rte_service_spec *service,
> > + uint32_t capability)
Currently the entire API is <stdint.h> only, leaving as is.
> > +int32_t
> > +rte_service_register(const struct rte_service_spec *spec)
> > +{
> > + uint32_t i;
> > + int32_t free_slot = -1;
> > +
> > + if (spec->callback == NULL || strlen(spec->name) == 0)
> > + return -EINVAL;
> > +
> > + for (i = 0; i < RTE_SERVICE_NUM_MAX; i++) {
> > + if (!service_valid(i)) {
> > + free_slot = i;
> > + break;
> > + }
> > + }
> > +
> > + if (free_slot < 0)
>
> if ((free_slot < 0) || (i == RTE_SERVICE_NUM_MAX))
Ah - a bug! Nice catch, fixed.
> > + s->internal_flags |= (1 << RTE_SERVICE_FLAG_REGISTERED_SHIFT);
> > +
> > + rte_smp_wmb();
> > + rte_service_count++;
>
> IMO, You can move above rte_smp_wmb() here.
Perhaps I'm not understanding correctly, but don't we need the writes to the
service spec to be completed before allowing other cores to see the extra
service count? In short, I think the wmb() is in the right place?
> > + memset(&rte_services[service_id], 0,
> > + sizeof(struct rte_service_spec_impl));
> > +
> > + rte_smp_wmb();
> > + rte_service_count--;
>
> IMO, You can move above rte_smp_wmb() here.
I think this part needs refactoring actually;
count--;
wmb();
memset();
Stop cores from seeing service, wmb() to ensure writes complete, then clear
internal config?
> > +int32_t
> > +rte_service_start(struct rte_service_spec *service)
> > +{
> > + struct rte_service_spec_impl *s =
> > + (struct rte_service_spec_impl *)service;
> > + s->runstate = RTE_SERVICE_RUNSTATE_RUNNING;
>
> Is this function can called from worker thread? if so add rte_smp_wmb()
Done
> > + return 0;
> > +}
> > +
> > +int32_t
> > +rte_service_stop(struct rte_service_spec *service)
> > +{
> > + struct rte_service_spec_impl *s =
> > + (struct rte_service_spec_impl *)service;
> > + s->runstate = RTE_SERVICE_RUNSTATE_STOPPED;
>
> Is this function can called from worker thread? if so add rte_smp_wmb()
Done
> > +static int32_t
> > +rte_service_runner_func(void *arg)
> > +{
> > + RTE_SET_USED(arg);
> > + uint32_t i;
> > + const int lcore = rte_lcore_id();
> > + struct core_state *cs = &cores_state[lcore];
> > +
> > + while (cores_state[lcore].runstate == RTE_SERVICE_RUNSTATE_RUNNING) {
> > + for (i = 0; i < rte_service_count; i++) {
> > + struct rte_service_spec_impl *s = &rte_services[i];
> > + uint64_t service_mask = cs->service_mask;
>
> No need to read in loop, Move it above while loop and add const.
> const uint64_t service_mask = cs->service_mask;
Yep done, I wonder would a compiler be smart enough.. :)
> > + uint32_t *lock = (uint32_t *)&s->execute_lock;
> > + if (rte_atomic32_cmpset(lock, 0, 1)) {
>
> rte_atomic32 is costly. How about checking RTE_SERVICE_CAP_MT_SAFE
> first.
Yep this was on my scope for optimizing down the line.
> > + void *userdata = s->spec.callback_userdata;
> > + uint64_t start = rte_rdtsc();
> > + s->spec.callback(userdata);
> > + uint64_t end = rte_rdtsc();
> > +
> > + uint64_t spent = end - start;
> > + s->cycles_spent += spent;
> > + s->calls++;
> > + cs->calls_per_service[i]++;
>
> How about enabling the statistics based on some runtime configuration?
Good idea - added an API to enable/disable statistics collection.
> > + rte_atomic32_clear(&s->execute_lock);
> > + }
> > + }
> > + rte_mb();
>
> Do we need full barrier here. Is rte_smp_rmb() inside the loop is
> enough?
Actually I'm not quite sure why there's a barrier at all.. removed.
> > + uint32_t i;
> > + uint32_t idx = 0;
> > + for (i = 0; i < RTE_MAX_LCORE; i++) {
>
> Are we good if "count" being the upper limit instead of RTE_MAX_LCORE?
Nope, the cores could be anywhere from 0 to RTE_MAX_LCORE - we gotta scan them
all.
> > + struct core_state *cs = &cores_state[i];
> > + if (cs->is_service_core) {
> > + array[idx] = i;
> > + idx++;
> > + }
> > + }
> > +
<snip>
> > + ret = rte_service_enable_on_core(s, j);
> > + if (ret)
> > + rte_panic("Enabling service core %d on
> > service %s failed\n",
> > + j, s->name);
>
> avoid panic in library
Done
> > + ret = rte_service_start(s);
> > + if (ret)
> > + rte_panic("failed to start service %s\n", s->name);
>
> avoid panic in library
Done
> > +static int32_t
> > +service_update(struct rte_service_spec *service, uint32_t lcore,
> > + uint32_t *set, uint32_t *enabled)
> > +{
<snip>
>
> If the parent functions can be called from worker thread then add
> rte_smp_wmb() here.
Yes they could, done.
> > + lcore_config[lcore].core_role = ROLE_SERVICE;
> > +
> > + /* TODO: take from EAL by setting ROLE_SERVICE? */
>
> I think, we need to fix TODO in v2
Good point :) done
> > + lcore_config[lcore].core_role = ROLE_RTE;
> > + cores_state[lcore].is_service_core = 0;
> > + /* TODO: return to EAL by setting ROLE_RTE? */
>
> I think, we need to fix TODO in v2
Done
> > + /* set core to run state first, and then launch otherwise it will
> > + * return immidiatly as runstate keeps it in the service poll loop
>
> s/immidiatly/immediately
Fixed
> > + int ret = rte_eal_remote_launch(rte_service_runner_func, 0, lcore);
> > + /* returns -EBUSY if the core is already launched, 0 on success */
> > + return ret;
>
> return rte_eal_remote_launch(rte_service_runner_func, 0, lcore);
I got bitten by this twice - documenting the return values, and making it
obvious where they come from is worth the variable IMO. Any compiler will
optimize away anyways :)
> > + /* avoid divide by zeros */
>
> s/zeros/zero
Fixed!
Thanks for the lengthy review - the code has improved a lot - appreciated.
