[dpdk-dev] [PATCH 1/6] service cores: header and implementation

[Van Haaren, Harry]

> From: Jerin Jacob [mailto:jerin.jacob at 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.haaren at 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.