Re: [PATCH 1/6] service cores: header and implementation

From: Jerin Jacob [mailto:jerin.jacob@caviumnetworks.com]

<snip>

quoted

This is the third iteration of the service core concept,
now with an implementation.

Signed-off-by: Harry van Haaren <redacted>

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.

quoted

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

quoted

 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

quoted

+/** 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

quoted

+ *
+ * @return The number of services registered.
+ */
+uint32_t rte_service_get_count(void);
+
+/** Return the specificaion of each service.

s/specificaion/specification

Fixed

quoted

+/* Check if a service has a specific capability.

Missing the doxygen marker(ie. change to /** Check)

Fixed

quoted

+/* Start a service core.

Missing the doxygen marker(ie. change to /** Start)

Fixed

quoted

+/** 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

quoted

+ * @param array An array of at least N items.

@param [out]  array An array of at least n items

quoted

+ * @param The size of *array*.

@param n The size of *array*.

Done!

quoted

+       /** NUMA socket ID that this service is affinitized to */
+       int8_t socket_id;

All other places socket_id is of type "int".

Done

quoted

+/** Private function to allow EAL to initialied default mappings.

typo:                                   ^^^^^^^^^^^

Fixed

quoted

+#define RTE_SERVICE_FLAG_REGISTERED_SHIFT 0

Internal macro, Can be shorten to reduce the length(SERVICE_F_REGISTERED?)

quoted

+
+#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.

quoted

+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 :)

quoted

+struct core_state {

<snip>

aligned to cache line?

Done

quoted

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

quoted

+static int

static inline int

quoted

+service_valid(uint32_t id) {

Done

quoted

+int32_t

bool could be enough here

quoted

+rte_service_probe_capability(const struct rte_service_spec *service,
+			     uint32_t capability)

Currently the entire API is <stdint.h> only, leaving as is.

quoted

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

quoted

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

quoted

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

quoted

+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

quoted

+	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

quoted

+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.. :)

quoted

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

 

quoted

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

quoted

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

quoted

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

quoted

+		struct core_state *cs = &cores_state[i];
+		if (cs->is_service_core) {
+			array[idx] = i;
+			idx++;
+		}
+	}
+

<snip>

quoted

+				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

quoted

+		ret = rte_service_start(s);
+		if (ret)
+			rte_panic("failed to start service %s\n", s->name);

avoid panic in library

Done

quoted

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

quoted

+	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

quoted

+	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

quoted

+	/* 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

quoted

+	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 :)

quoted

+	/* avoid divide by zeros */

s/zeros/zero

Fixed!


Thanks for the lengthy review - the code has improved a lot - appreciated.