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

-----Original Message-----

This will install rte_service_private.h file $RTE_TARGET. Based on
earlier email, You don't want to expose rte_service_private.h to
application. Right?
If so, I think, we remove rte_service_private.h from here and add CFLAGS
to point this header in local makefile of the _component_ which interested
in getting eal component specific functions.

s/retuns/returns/

+int32_t rte_service_lcore_reset_all(void);
+
+/** Enable or disable statistics collection.
+ *
+ * This function enables per core, per-service cycle count collection.
+ * @param enabled Zero to turn off statistics collection, non-zero to enable.
+ */
+void rte_service_set_stats_enable(int enabled);

Since it it is a "set" function, better argument is "int enable"

+
+/** Retrieve the list of currently enabled service cores.
+ *
+ * This function fills in an application supplied array, with each element
+ * indicating the lcore_id of a service core.
+ *
+ * Adding and removing service cores can be performed using
+ * *rte_service_lcore_add* and *rte_service_lcore_del*.
+ * @param [out] array An array of at least N items.

What is N here. It should be RTE_MAX_LCORE. Right?

+ * @param [out] The size of *array*.
+ * @retval >=0 Number of service cores that have been populated in the array
+ * @retval -ENOMEM The provided array is not large enough to fill in the
+ *          service core list. No items have been populated, call this function
+ *          with a size of at least *rte_service_core_count* items.
+ */
+int32_t rte_service_lcore_list(uint32_t array[], uint32_t n);
+
+	cores_state = rte_calloc("rte_service_core_states", RTE_MAX_LCORE,
+			sizeof(struct core_state), RTE_CACHE_LINE_SIZE);
+	if (!cores_state) {
+		printf("error allocating core states array\n");
+		return -ENOMEM;
+	}
+
+	int i;
+	int count = 0;
+	struct rte_config *cfg = rte_eal_get_configuration();
+	for (i = 0; i < RTE_MAX_LCORE; i++) {
+		if (lcore_config[i].core_role == ROLE_SERVICE) {
+			if ((unsigned)i == cfg->master_lcore)
+				continue;
+			rte_service_lcore_add(i);
+			count++;
+		}
+	}
+
+	rte_service_library_initialized = 1;
+	return 0;
+}
+
+void rte_service_set_stats_enable(int enabled)

Since it it is a "set" function, better argument is "int enable"

+{
+	uint32_t i;
+	for (i = 0; i < RTE_MAX_LCORE; i++)
+		cores_state[i].collect_statistics = enabled;
+}
+
+/* returns 1 if service is registered and has not been unregistered
+ * Returns 0 if service never registered, or has been unregistered
+ */
+static inline int
+service_valid(uint32_t id) {
+	return !!(rte_services[id].internal_flags &
+		 (1 << SERVICE_F_REGISTERED));
+}
+
+uint32_t
+rte_service_get_count(void)
+{
+	return rte_service_count;
+}
+
+struct rte_service_spec *
+rte_service_get_by_id(uint32_t id)
+{
+	struct rte_service_spec *service = NULL;
+	if (id < rte_service_count)
+		service = (struct rte_service_spec *)&rte_services[id];
+
+	return service;
+}
+
+struct rte_service_spec *rte_service_get_by_name(const char *name)
+{
+	struct rte_service_spec *service = NULL;
+	int i;
+	for (i = 0; i < RTE_SERVICE_NUM_MAX; i++) {
+		if (service_valid(i) &&
+				strcmp(name, rte_services[i].spec.name) == 0)
+			service = (struct rte_service_spec *)&rte_services[i];
+			break;

"break" should be under "if" condition. ie { and } for the "if" condition is
missing

+	}
+
+	return service;
+}
+
+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) || (i == RTE_SERVICE_NUM_MAX))
+		return -ENOSPC;
+
+	struct rte_service_spec_impl *s = &rte_services[free_slot];
+	s->spec = *spec;
+	s->internal_flags |= (1 << SERVICE_F_REGISTERED);
+
+	rte_smp_wmb();
+	rte_service_count++;

My earlier comments on rte_smp_wmb() was in assumption that
rte_service_register() called be from worker cores.

Can rte_service_register() called from worker threads. No. Right?
if yes then we need an atomic variable to increment rte_service_count.

+
+	return 0;
+}
+
+int32_t
+rte_service_start(struct rte_service_spec *service)
+{
+	struct rte_service_spec_impl *s =
+		(struct rte_service_spec_impl *)service;
+	s->runstate = RUNSTATE_RUNNING;
+	rte_smp_wmb();
+	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 = RUNSTATE_STOPPED;
+	rte_smp_wmb();
+	return 0;
+}
+
+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];

Couple of comments in fastpath code. IMO, better to move "service_mask"
and access to global variable(so that it allocated from local stack) outside the loop.
global variable may share with other frequent write variables.


	const uint64_t service_mask = cs->service_mask;
	const uint32_t __rte_service_count = rte_service_count;

+
+	while (cores_state[lcore].runstate == RUNSTATE_RUNNING) {
+		for (i = 0; i < rte_service_count; i++) {
+			struct rte_service_spec_impl *s = &rte_services[i];
+			if (s->runstate != RUNSTATE_RUNNING ||
+					!(service_mask & (1 << i)))
+				continue;
+
+			/* check if this is the only core mapped, else use
+			 * atomic to serialize cores mapped to this service
+			 */
+			uint32_t *lock = (uint32_t *)&s->execute_lock;
+			if ((s->spec.capabilities & RTE_SERVICE_CAP_MT_SAFE) ||
+					(s->num_mapped_cores == 1 ||
+					rte_atomic32_cmpset(lock, 0, 1))) {
+				void *userdata = s->spec.callback_userdata;
+
+				if (cs->collect_statistics) {
+					uint64_t start = rte_rdtsc();
+					s->spec.callback(userdata);
+					uint64_t end = rte_rdtsc();
+					s->cycles_spent += end - start;
+					cs->calls_per_service[i]++;
+					s->calls++;
+				} else {
+					cs->calls_per_service[i]++;

Should we need this  in non stat configuration ?

+					s->spec.callback(userdata);
+					s->calls++;

Should we need this  in non stat configuration ?

+				}
+
+				rte_atomic32_clear(&s->execute_lock);

Call this only when "rte_atomic32_cmpset" in action as it is costly for
normal case.

IMO, we need to have rte_smp_rmb() here to update the status of
cores_state[lcore].runstate or s->runstate(as it can be updated from other
lcores)

+			}
+		}
+	}
+
+	lcore_config[lcore].state = WAIT;
+
+	return 0;
+}
+