[dpdk-dev] [PATCH 0/2] service: enable app lcore to run service iter

[dpdk-dev] [PATCH 0/2] service: enable app lcore to run service iter

[Harry van Haaren]

This patchset enables an application lcore (non-service core) to
run an iteration of a service. This is useful in a variety of cases,
from advanced applications to unit-testing. A second function is added
to allow the application to disable the check that a service-core is
mapped to a service, in the return value from runstate_get(). This
function passes responsibility to the application to run the service
as required, allowing the first function to actually be of use. See
the second commit message for details.

This patchset is in response to rework on the Eventdev SW PMD tests[1],
which made it clear that there is a need for applications to be able
to run exactly one iteration of a service exactly when required.

Regards, -Harry

[1] http://dpdk.org/dev/patchwork/patch/30666/

Harry van Haaren (2):
  service: add function for app lcore to run service
  service: add runtime service core check disable

 lib/librte_eal/bsdapp/eal/rte_eal_version.map   |  2 +
 lib/librte_eal/common/include/rte_service.h     | 44 +++++++++++++-
 lib/librte_eal/common/rte_service.c             | 81 ++++++++++++++++++-------
 lib/librte_eal/linuxapp/eal/rte_eal_version.map |  2 +
 4 files changed, 105 insertions(+), 24 deletions(-)

-- 
2.7.4

[dpdk-dev] [PATCH 1/2] service: add function for app lcore to run service

[Harry van Haaren]

This commit adds a new function which allows an application lcore
(aka; *not* a dedicated service-lcore) to run a service. This
function is required to allow applications to gradually port
functionality to using services, while still being able to run
ordinary application functions.

This requirement became clear when a patch to the existing
eventdev/pipeline sample app was modified to use a service-core
for scheduling - and that same core should be capable of running
the "worker()" function from the application too.

This patch refactors the existing running code into a smaller
"service_run" function, which can be called by the dedicated
service-core loop, and the newly added function.

Signed-off-by: Harry van Haaren <harry.van.haaren@intel.com>

[1] http://dpdk.org/ml/archives/dev/2017-October/079876.html
---
 lib/librte_eal/bsdapp/eal/rte_eal_version.map   |  1 +
 lib/librte_eal/common/include/rte_service.h     | 21 +++++++++
 lib/librte_eal/common/rte_service.c             | 59 ++++++++++++++++---------
 lib/librte_eal/linuxapp/eal/rte_eal_version.map |  1 +
 4 files changed, 61 insertions(+), 21 deletions(-)

diff --git a/lib/librte_eal/bsdapp/eal/rte_eal_version.map b/lib/librte_eal/bsdapp/eal/rte_eal_version.map
index 080896f..ee8dc98 100644
--- a/lib/librte_eal/bsdapp/eal/rte_eal_version.map
+++ b/lib/librte_eal/bsdapp/eal/rte_eal_version.map
@@ -229,6 +229,7 @@ EXPERIMENTAL {
 	rte_service_map_lcore_set;
 	rte_service_probe_capability;
 	rte_service_reset;
+	rte_service_run_iter_on_app_lcore;
 	rte_service_runstate_get;
 	rte_service_runstate_set;
 	rte_service_set_stats_enable;
diff --git a/lib/librte_eal/common/include/rte_service.h b/lib/librte_eal/common/include/rte_service.h
index 21da739..63d3170 100644
--- a/lib/librte_eal/common/include/rte_service.h
+++ b/lib/librte_eal/common/include/rte_service.h
@@ -211,6 +211,27 @@ int32_t rte_service_runstate_get(uint32_t id);
  * @warning
  * @b EXPERIMENTAL: this API may change without prior notice
  *
+ * This function runs a service callback from a non-service lcore context.
+ * The *id* of the service to be run is passed in, and the service-callback
+ * is executed on the calling lcore immediately if possible. If the service is
+ * not multi-thread capable and another thread is currently executing it, this
+ * function returns without running the callback.
+ *
+ * Note that any thread calling this function MUST be a DPDK EAL thread, as
+ * the *rte_lcore_id* function is used to access internal data structures.
+ *
+ * @retval 0 Service was run on the calling thread successfully
+ * @retval -EBUSY Another lcore is executing the service, and it is not a
+ *         multi-thread safe service, so the service was not run on this lcore
+ * @retval -ENOEXEC Service is not in a runnable state
+ * @retval -EINVAL Invalid service id
+ */
+int32_t rte_service_run_iter_on_app_lcore(uint32_t id);
+
+/**
+ * @warning
+ * @b EXPERIMENTAL: this API may change without prior notice
+ *
  * Start a service core.
  *
  * Starting a core makes the core begin polling. Any services assigned to it
diff --git a/lib/librte_eal/common/rte_service.c b/lib/librte_eal/common/rte_service.c
index e598e16..4e27f75 100644
--- a/lib/librte_eal/common/rte_service.c
+++ b/lib/librte_eal/common/rte_service.c
@@ -331,6 +331,42 @@ rte_service_runner_do_callback(struct rte_service_spec_impl *s,
 		s->spec.callback(userdata);
 }
 
+
+static inline int32_t
+service_run(uint32_t i, struct core_state *cs, uint64_t service_mask)
+{
+	if (!service_valid(i))
+		return -EINVAL;
+	struct rte_service_spec_impl *s = &rte_services[i];
+	if (s->comp_runstate != RUNSTATE_RUNNING ||
+			s->app_runstate != RUNSTATE_RUNNING ||
+			!(service_mask & (UINT64_C(1) << i)))
+		return -ENOEXEC;
+
+	/* check do we need cmpset, if MT safe or <= 1 core
+	 * mapped, atomic ops are not required.
+	 */
+	const int use_atomics = (service_mt_safe(s) == 0) &&
+				(s->num_mapped_cores > 1);
+	if (use_atomics) {
+		if (!rte_atomic32_cmpset((uint32_t *)&s->execute_lock, 0, 1))
+			return -EBUSY;
+
+		rte_service_runner_do_callback(s, cs, i);
+		rte_atomic32_clear(&s->execute_lock);
+	} else
+		rte_service_runner_do_callback(s, cs, i);
+
+	return 0;
+}
+
+int32_t rte_service_run_iter_on_app_lcore(uint32_t id)
+{
+	/* run service on calling core, using all-ones as the service mask */
+	struct core_state *cs = &lcore_states[rte_lcore_id()];
+	return service_run(id, cs, UINT64_MAX);
+}
+
 static int32_t
 rte_service_runner_func(void *arg)
 {
@@ -343,27 +379,8 @@ rte_service_runner_func(void *arg)
 		const uint64_t service_mask = cs->service_mask;
 
 		for (i = 0; i < RTE_SERVICE_NUM_MAX; i++) {
-			if (!service_valid(i))
-				continue;
-			struct rte_service_spec_impl *s = &rte_services[i];
-			if (s->comp_runstate != RUNSTATE_RUNNING ||
-					s->app_runstate != RUNSTATE_RUNNING ||
-					!(service_mask & (UINT64_C(1) << i)))
-				continue;
-
-			/* check do we need cmpset, if MT safe or <= 1 core
-			 * mapped, atomic ops are not required.
-			 */
-			const int use_atomics = (service_mt_safe(s) == 0) &&
-						(s->num_mapped_cores > 1);
-			if (use_atomics) {
-				uint32_t *lock = (uint32_t *)&s->execute_lock;
-				if (rte_atomic32_cmpset(lock, 0, 1)) {
-					rte_service_runner_do_callback(s, cs, i);
-					rte_atomic32_clear(&s->execute_lock);
-				}
-			} else
-				rte_service_runner_do_callback(s, cs, i);
+			/* return value ignored as no change to code flow */
+			service_run(i, cs, service_mask);
 		}
 
 		rte_smp_rmb();
diff --git a/lib/librte_eal/linuxapp/eal/rte_eal_version.map b/lib/librte_eal/linuxapp/eal/rte_eal_version.map
index c173ccf..c4d2c27 100644
--- a/lib/librte_eal/linuxapp/eal/rte_eal_version.map
+++ b/lib/librte_eal/linuxapp/eal/rte_eal_version.map
@@ -233,6 +233,7 @@ EXPERIMENTAL {
 	rte_service_map_lcore_set;
 	rte_service_probe_capability;
 	rte_service_reset;
+	rte_service_run_iter_on_app_lcore;
 	rte_service_runstate_get;
 	rte_service_runstate_set;
 	rte_service_set_stats_enable;
-- 
2.7.4

Re: [dpdk-dev] [PATCH 1/2] service: add function for app lcore to run service

[Pavan Nikhilesh Bhagavatula]

On Mon, Oct 23, 2017 at 06:16:51PM +0100, Harry van Haaren wrote:
> This commit adds a new function which allows an application lcore
> (aka; *not* a dedicated service-lcore) to run a service. This
> function is required to allow applications to gradually port
> functionality to using services, while still being able to run
> ordinary application functions.
>
> This requirement became clear when a patch to the existing
> eventdev/pipeline sample app was modified to use a service-core
> for scheduling - and that same core should be capable of running
> the "worker()" function from the application too.
>
> This patch refactors the existing running code into a smaller
> "service_run" function, which can be called by the dedicated
> service-core loop, and the newly added function.
>
> Signed-off-by: Harry van Haaren <harry.van.haaren@intel.com>
>
> [1] http://dpdk.org/ml/archives/dev/2017-October/079876.html
> ---
>  lib/librte_eal/bsdapp/eal/rte_eal_version.map   |  1 +
>  lib/librte_eal/common/include/rte_service.h     | 21 +++++++++
>  lib/librte_eal/common/rte_service.c             | 59 ++++++++++++++++---------
>  lib/librte_eal/linuxapp/eal/rte_eal_version.map |  1 +
>  4 files changed, 61 insertions(+), 21 deletions(-)
>
> diff --git a/lib/librte_eal/bsdapp/eal/rte_eal_version.map b/lib/librte_eal/bsdapp/eal/rte_eal_version.map
> index 080896f..ee8dc98 100644
> --- a/lib/librte_eal/bsdapp/eal/rte_eal_version.map
> +++ b/lib/librte_eal/bsdapp/eal/rte_eal_version.map
> @@ -229,6 +229,7 @@ EXPERIMENTAL {
>  	rte_service_map_lcore_set;
>  	rte_service_probe_capability;
>  	rte_service_reset;
> +	rte_service_run_iter_on_app_lcore;
>  	rte_service_runstate_get;
>  	rte_service_runstate_set;
>  	rte_service_set_stats_enable;
> diff --git a/lib/librte_eal/common/include/rte_service.h b/lib/librte_eal/common/include/rte_service.h
> index 21da739..63d3170 100644
> --- a/lib/librte_eal/common/include/rte_service.h
> +++ b/lib/librte_eal/common/include/rte_service.h
> @@ -211,6 +211,27 @@ int32_t rte_service_runstate_get(uint32_t id);
>   * @warning
>   * @b EXPERIMENTAL: this API may change without prior notice
>   *
> + * This function runs a service callback from a non-service lcore context.
> + * The *id* of the service to be run is passed in, and the service-callback
> + * is executed on the calling lcore immediately if possible. If the service is
> + * not multi-thread capable and another thread is currently executing it, this
> + * function returns without running the callback.
> + *
> + * Note that any thread calling this function MUST be a DPDK EAL thread, as
> + * the *rte_lcore_id* function is used to access internal data structures.
> + *
> + * @retval 0 Service was run on the calling thread successfully
> + * @retval -EBUSY Another lcore is executing the service, and it is not a
> + *         multi-thread safe service, so the service was not run on this lcore
> + * @retval -ENOEXEC Service is not in a runnable state
> + * @retval -EINVAL Invalid service id
> + */
> +int32_t rte_service_run_iter_on_app_lcore(uint32_t id);
> +
> +/**
> + * @warning
> + * @b EXPERIMENTAL: this API may change without prior notice
> + *
>   * Start a service core.
>   *
>   * Starting a core makes the core begin polling. Any services assigned to it
> diff --git a/lib/librte_eal/common/rte_service.c b/lib/librte_eal/common/rte_service.c
> index e598e16..4e27f75 100644
> --- a/lib/librte_eal/common/rte_service.c
> +++ b/lib/librte_eal/common/rte_service.c
> @@ -331,6 +331,42 @@ rte_service_runner_do_callback(struct rte_service_spec_impl *s,
>  		s->spec.callback(userdata);
>  }
>
> +
> +static inline int32_t
> +service_run(uint32_t i, struct core_state *cs, uint64_t service_mask)
> +{
> +	if (!service_valid(i))
> +		return -EINVAL;
> +	struct rte_service_spec_impl *s = &rte_services[i];
> +	if (s->comp_runstate != RUNSTATE_RUNNING ||
> +			s->app_runstate != RUNSTATE_RUNNING ||
> +			!(service_mask & (UINT64_C(1) << i)))
> +		return -ENOEXEC;
> +
> +	/* check do we need cmpset, if MT safe or <= 1 core
> +	 * mapped, atomic ops are not required.
> +	 */
> +	const int use_atomics = (service_mt_safe(s) == 0) &&
> +				(s->num_mapped_cores > 1);
> +	if (use_atomics) {
> +		if (!rte_atomic32_cmpset((uint32_t *)&s->execute_lock, 0, 1))
> +			return -EBUSY;
> +
> +		rte_service_runner_do_callback(s, cs, i);
> +		rte_atomic32_clear(&s->execute_lock);
> +	} else
> +		rte_service_runner_do_callback(s, cs, i);
> +
> +	return 0;
> +}
> +
> +int32_t rte_service_run_iter_on_app_lcore(uint32_t id)
> +{
> +	/* run service on calling core, using all-ones as the service mask */
> +	struct core_state *cs = &lcore_states[rte_lcore_id()];
> +	return service_run(id, cs, UINT64_MAX);
> +}
> +
>  static int32_t
>  rte_service_runner_func(void *arg)
>  {
> @@ -343,27 +379,8 @@ rte_service_runner_func(void *arg)
>  		const uint64_t service_mask = cs->service_mask;
>
>  		for (i = 0; i < RTE_SERVICE_NUM_MAX; i++) {
> -			if (!service_valid(i))
> -				continue;
> -			struct rte_service_spec_impl *s = &rte_services[i];
> -			if (s->comp_runstate != RUNSTATE_RUNNING ||
> -					s->app_runstate != RUNSTATE_RUNNING ||
> -					!(service_mask & (UINT64_C(1) << i)))
> -				continue;
> -
> -			/* check do we need cmpset, if MT safe or <= 1 core
> -			 * mapped, atomic ops are not required.
> -			 */
> -			const int use_atomics = (service_mt_safe(s) == 0) &&
> -						(s->num_mapped_cores > 1);
> -			if (use_atomics) {
> -				uint32_t *lock = (uint32_t *)&s->execute_lock;
> -				if (rte_atomic32_cmpset(lock, 0, 1)) {
> -					rte_service_runner_do_callback(s, cs, i);
> -					rte_atomic32_clear(&s->execute_lock);
> -				}
> -			} else
> -				rte_service_runner_do_callback(s, cs, i);
> +			/* return value ignored as no change to code flow */
> +			service_run(i, cs, service_mask);
>  		}
>
>  		rte_smp_rmb();
> diff --git a/lib/librte_eal/linuxapp/eal/rte_eal_version.map b/lib/librte_eal/linuxapp/eal/rte_eal_version.map
> index c173ccf..c4d2c27 100644
> --- a/lib/librte_eal/linuxapp/eal/rte_eal_version.map
> +++ b/lib/librte_eal/linuxapp/eal/rte_eal_version.map
> @@ -233,6 +233,7 @@ EXPERIMENTAL {
>  	rte_service_map_lcore_set;
>  	rte_service_probe_capability;
>  	rte_service_reset;
> +	rte_service_run_iter_on_app_lcore;
>  	rte_service_runstate_get;
>  	rte_service_runstate_set;
>  	rte_service_set_stats_enable;
> --
> 2.7.4
>
Acked-by: Pavan Nikhilesh <pbhagavatula@caviumnetworks.com>

[dpdk-dev] [PATCH 2/2] service: add runtime service core check disable

[Harry van Haaren]

This commit adds a new function to disable the runtime mapped
service-cores check. This allows an application to take responsibility
of running unmapped services.

This feature is useful in cases like unit tests, where the application
code (or unit test in this case) requires accurate control over when
the service function is called to ensure correct behaviour, and when
an application has an advanced use-case and wishes to manage services
manually.

Signed-off-by: Harry van Haaren <harry.van.haaren@intel.com>
---
 lib/librte_eal/bsdapp/eal/rte_eal_version.map   |  1 +
 lib/librte_eal/common/include/rte_service.h     | 23 ++++++++++++++++++++++-
 lib/librte_eal/common/rte_service.c             | 22 ++++++++++++++++++++--
 lib/librte_eal/linuxapp/eal/rte_eal_version.map |  1 +
 4 files changed, 44 insertions(+), 3 deletions(-)

diff --git a/lib/librte_eal/bsdapp/eal/rte_eal_version.map b/lib/librte_eal/bsdapp/eal/rte_eal_version.map
index ee8dc98..80817a5 100644
--- a/lib/librte_eal/bsdapp/eal/rte_eal_version.map
+++ b/lib/librte_eal/bsdapp/eal/rte_eal_version.map
@@ -232,6 +232,7 @@ EXPERIMENTAL {
 	rte_service_run_iter_on_app_lcore;
 	rte_service_runstate_get;
 	rte_service_runstate_set;
+	rte_service_set_runstate_mapped_check;
 	rte_service_set_stats_enable;
 	rte_service_start_with_defaults;
 
diff --git a/lib/librte_eal/common/include/rte_service.h b/lib/librte_eal/common/include/rte_service.h
index 63d3170..75faafe 100644
--- a/lib/librte_eal/common/include/rte_service.h
+++ b/lib/librte_eal/common/include/rte_service.h
@@ -199,7 +199,12 @@ int32_t rte_service_runstate_set(uint32_t id, uint32_t runstate);
  * @b EXPERIMENTAL: this API may change without prior notice
  *
  * Get the runstate for the service with *id*. See *rte_service_runstate_set*
- * for details of runstates.
+ * for details of runstates. A service can call this function to ensure that
+ * the application has indicated that it will receive CPU cycles. Either a
+ * service-core is mapped (default case), or the application has explicitly
+ * disabled the check that a service-cores is mapped to the service and takes
+ * responsibility to run the service manually using the available function
+ * *rte_service_run_iter_on_app_lcore* to do so.
  *
  * @retval 1 Service is running
  * @retval 0 Service is stopped
@@ -211,6 +216,22 @@ int32_t rte_service_runstate_get(uint32_t id);
  * @warning
  * @b EXPERIMENTAL: this API may change without prior notice
  *
+ * Enable or disable the check for a service-core being mapped to the service.
+ * An application can disable the check when takes the responsibility to run a
+ * service itself using *rte_service_run_iter_on_app_lcore*.
+ *
+ * @param id The id of the service to set the check on
+ * @param enabled When zero, the check is disabled. Non-zero enables the check.
+ *
+ * @retval 0 Success
+ * @retval -EINVAL Invalid service ID
+ */
+int32_t rte_service_set_runstate_mapped_check(uint32_t id, int32_t enabled);
+
+/**
+ * @warning
+ * @b EXPERIMENTAL: this API may change without prior notice
+ *
  * This function runs a service callback from a non-service lcore context.
  * The *id* of the service to be run is passed in, and the service-callback
  * is executed on the calling lcore immediately if possible. If the service is
diff --git a/lib/librte_eal/common/rte_service.c b/lib/librte_eal/common/rte_service.c
index 4e27f75..f17bf4b 100644
--- a/lib/librte_eal/common/rte_service.c
+++ b/lib/librte_eal/common/rte_service.c
@@ -54,6 +54,7 @@
 
 #define SERVICE_F_REGISTERED    (1 << 0)
 #define SERVICE_F_STATS_ENABLED (1 << 1)
+#define SERVICE_F_START_CHECK   (1 << 2)
 
 /* runstates for services and lcores, denoting if they are active or not */
 #define RUNSTATE_STOPPED 0
@@ -180,6 +181,19 @@ int32_t rte_service_set_stats_enable(uint32_t id, int32_t enabled)
 	return 0;
 }
 
+int32_t rte_service_set_runstate_mapped_check(uint32_t id, int32_t enabled)
+{
+	struct rte_service_spec_impl *s;
+	SERVICE_VALID_GET_OR_ERR_RET(id, s, 0);
+
+	if (enabled)
+		s->internal_flags |= SERVICE_F_START_CHECK;
+	else
+		s->internal_flags &= ~(SERVICE_F_START_CHECK);
+
+	return 0;
+}
+
 uint32_t
 rte_service_get_count(void)
 {
@@ -241,7 +255,7 @@ rte_service_component_register(const struct rte_service_spec *spec,
 
 	struct rte_service_spec_impl *s = &rte_services[free_slot];
 	s->spec = *spec;
-	s->internal_flags |= SERVICE_F_REGISTERED;
+	s->internal_flags |= SERVICE_F_REGISTERED | SERVICE_F_START_CHECK;
 
 	rte_smp_wmb();
 	rte_service_count++;
@@ -309,9 +323,13 @@ rte_service_runstate_get(uint32_t id)
 	struct rte_service_spec_impl *s;
 	SERVICE_VALID_GET_OR_ERR_RET(id, s, -EINVAL);
 	rte_smp_rmb();
+
+	int check_disabled = !(s->internal_flags & SERVICE_F_START_CHECK);
+	int lcore_mapped = (s->num_mapped_cores > 0);
+
 	return (s->app_runstate == RUNSTATE_RUNNING) &&
 		(s->comp_runstate == RUNSTATE_RUNNING) &&
-		(s->num_mapped_cores > 0);
+		(check_disabled | lcore_mapped);
 }
 
 static inline void
diff --git a/lib/librte_eal/linuxapp/eal/rte_eal_version.map b/lib/librte_eal/linuxapp/eal/rte_eal_version.map
index c4d2c27..afe66b3 100644
--- a/lib/librte_eal/linuxapp/eal/rte_eal_version.map
+++ b/lib/librte_eal/linuxapp/eal/rte_eal_version.map
@@ -236,6 +236,7 @@ EXPERIMENTAL {
 	rte_service_run_iter_on_app_lcore;
 	rte_service_runstate_get;
 	rte_service_runstate_set;
+	rte_service_set_runstate_mapped_check;
 	rte_service_set_stats_enable;
 	rte_service_start_with_defaults;
 
-- 
2.7.4

Re: [dpdk-dev] [PATCH 2/2] service: add runtime service core check disable

[Pavan Nikhilesh Bhagavatula]

On Mon, Oct 23, 2017 at 06:16:52PM +0100, Harry van Haaren wrote:
> This commit adds a new function to disable the runtime mapped
> service-cores check. This allows an application to take responsibility
> of running unmapped services.
>
> This feature is useful in cases like unit tests, where the application
> code (or unit test in this case) requires accurate control over when
> the service function is called to ensure correct behaviour, and when
> an application has an advanced use-case and wishes to manage services
> manually.
>

Hey Harry,

The patch looks good from a logical point of view, I have integrated this with
4/7 and 5/7 of the patch set [1] for removing eventdev schedule API and everything
works well.

My only concern is that the function name is a bit tricky to grasp. Unless the
application(dev) knows the internal working of rte_service_runstate_get() it
would be a bit tricky to understand the role of
rte_service_set_runstate_mapped_check().

I think we should think of a more appropriate API name for this function.

> Signed-off-by: Harry van Haaren <harry.van.haaren@intel.com>
> ---
>  lib/librte_eal/bsdapp/eal/rte_eal_version.map   |  1 +
>  lib/librte_eal/common/include/rte_service.h     | 23 ++++++++++++++++++++++-
>  lib/librte_eal/common/rte_service.c             | 22 ++++++++++++++++++++--
>  lib/librte_eal/linuxapp/eal/rte_eal_version.map |  1 +
>  4 files changed, 44 insertions(+), 3 deletions(-)
<snip>
> + * @param enabled When zero, the check is disabled. Non-zero enables the check.
> + *
> + * @retval 0 Success
> + * @retval -EINVAL Invalid service ID
> + */
> +int32_t rte_service_set_runstate_mapped_check(uint32_t id, int32_t enabled);
> +
s/enabled/enable
> +/**
> + * @warning
> + * @b EXPERIMENTAL: this API may change without prior notice
> + *
>   * This function runs a service callback from a non-service lcore context.
>   * The *id* of the service to be run is passed in, and the service-callback
>   * is executed on the calling lcore immediately if possible. If the service is
>
<snip>
> --
> 2.7.4
>

Regards,
Pavan

Re: [dpdk-dev] [PATCH 2/2] service: add runtime service core check disable

[Van Haaren, Harry]

> From: Pavan Nikhilesh Bhagavatula [mailto:pbhagavatula@caviumnetworks.com]
> Sent: Tuesday, October 24, 2017 6:25 PM
> To: Van Haaren, Harry <harry.van.haaren@intel.com>
> Cc: dev@dpdk.org
> Subject: Re: [PATCH 2/2] service: add runtime service core check disable
> 
> On Mon, Oct 23, 2017 at 06:16:52PM +0100, Harry van Haaren wrote:
> > This commit adds a new function to disable the runtime mapped
> > service-cores check. This allows an application to take responsibility
> > of running unmapped services.
> >
> > This feature is useful in cases like unit tests, where the application
> > code (or unit test in this case) requires accurate control over when
> > the service function is called to ensure correct behaviour, and when
> > an application has an advanced use-case and wishes to manage services
> > manually.
> >
> 
> Hey Harry,
> 
> The patch looks good from a logical point of view, I have integrated this
> with
> 4/7 and 5/7 of the patch set [1] for removing eventdev schedule API and
> everything
> works well.
> 
> My only concern is that the function name is a bit tricky to grasp. Unless
> the
> application(dev) knows the internal working of rte_service_runstate_get() it
> would be a bit tricky to understand the role of
> rte_service_set_runstate_mapped_check().
> 
> I think we should think of a more appropriate API name for this function.


Suggestions welcome!

Otherwise I hope the documentation and unit-test usages explain the functionality ok.

Cheers, -Harry


> > Signed-off-by: Harry van Haaren <harry.van.haaren@intel.com>
> > ---
> >  lib/librte_eal/bsdapp/eal/rte_eal_version.map   |  1 +
> >  lib/librte_eal/common/include/rte_service.h     | 23
> ++++++++++++++++++++++-
> >  lib/librte_eal/common/rte_service.c             | 22
> ++++++++++++++++++++--
> >  lib/librte_eal/linuxapp/eal/rte_eal_version.map |  1 +
> >  4 files changed, 44 insertions(+), 3 deletions(-)
> <snip>
> > + * @param enabled When zero, the check is disabled. Non-zero enables the
> check.
> > + *
> > + * @retval 0 Success
> > + * @retval -EINVAL Invalid service ID
> > + */
> > +int32_t rte_service_set_runstate_mapped_check(uint32_t id, int32_t
> enabled);
> > +
> s/enabled/enable
> > +/**
> > + * @warning
> > + * @b EXPERIMENTAL: this API may change without prior notice
> > + *
> >   * This function runs a service callback from a non-service lcore
> context.
> >   * The *id* of the service to be run is passed in, and the service-
> callback
> >   * is executed on the calling lcore immediately if possible. If the
> service is
> >
> <snip>
> > --
> > 2.7.4
> >
> 
> Regards,
> Pavan

Re: [dpdk-dev] [PATCH 2/2] service: add runtime service core check disable

[Pavan Nikhilesh Bhagavatula]

On Wed, Oct 25, 2017 at 09:09:53AM +0000, Van Haaren, Harry wrote:
> > From: Pavan Nikhilesh Bhagavatula [mailto:pbhagavatula@caviumnetworks.com]
> > Sent: Tuesday, October 24, 2017 6:25 PM
> > To: Van Haaren, Harry <harry.van.haaren@intel.com>
> > Cc: dev@dpdk.org
> > Subject: Re: [PATCH 2/2] service: add runtime service core check disable
> >
> > On Mon, Oct 23, 2017 at 06:16:52PM +0100, Harry van Haaren wrote:
> > > This commit adds a new function to disable the runtime mapped
> > > service-cores check. This allows an application to take responsibility
> > > of running unmapped services.
> > >
> > > This feature is useful in cases like unit tests, where the application
> > > code (or unit test in this case) requires accurate control over when
> > > the service function is called to ensure correct behaviour, and when
> > > an application has an advanced use-case and wishes to manage services
> > > manually.
> > >
> >
> > Hey Harry,
> >
> > The patch looks good from a logical point of view, I have integrated this
> > with
> > 4/7 and 5/7 of the patch set [1] for removing eventdev schedule API and
> > everything
> > works well.
> >
> > My only concern is that the function name is a bit tricky to grasp. Unless
> > the
> > application(dev) knows the internal working of rte_service_runstate_get() it
> > would be a bit tricky to understand the role of
> > rte_service_set_runstate_mapped_check().
> >
> > I think we should think of a more appropriate API name for this function.
>
>
> Suggestions welcome!
>
> Otherwise I hope the documentation and unit-test usages explain the functionality ok.
>
> Cheers, -Harry
>

I think the documentation would be sufficient as I don't have a better
suggestion for the function name.


-Pavan

>
> > > Signed-off-by: Harry van Haaren <harry.van.haaren@intel.com>
> > > ---
> > >  lib/librte_eal/bsdapp/eal/rte_eal_version.map   |  1 +
> > >  lib/librte_eal/common/include/rte_service.h     | 23
> > ++++++++++++++++++++++-
> > >  lib/librte_eal/common/rte_service.c             | 22
> > ++++++++++++++++++++--
> > >  lib/librte_eal/linuxapp/eal/rte_eal_version.map |  1 +
> > >  4 files changed, 44 insertions(+), 3 deletions(-)
> > <snip>
> > > + * @param enabled When zero, the check is disabled. Non-zero enables the
> > check.
> > > + *
> > > + * @retval 0 Success
> > > + * @retval -EINVAL Invalid service ID
> > > + */
> > > +int32_t rte_service_set_runstate_mapped_check(uint32_t id, int32_t
> > enabled);
> > > +
> > s/enabled/enable
> > > +/**
> > > + * @warning
> > > + * @b EXPERIMENTAL: this API may change without prior notice
> > > + *
> > >   * This function runs a service callback from a non-service lcore
> > context.
> > >   * The *id* of the service to be run is passed in, and the service-
> > callback
> > >   * is executed on the calling lcore immediately if possible. If the
> > service is
> > >
> > <snip>
> > > --
> > > 2.7.4
> > >
> >
> > Regards,
> > Pavan

Acked-by: Pavan Nikhilesh <pbhagavatula@caviumnetworks.com>

[dpdk-dev] [PATCH v2 0/2] service: enable app lcore to run service iter

[Harry van Haaren]

This patchset enables an application lcore (non-service core) to
run an iteration of a service. This is useful in a variety of cases,
from advanced applications to unit-testing. A second function is added
to allow the application to disable the check that a service-core is
mapped to a service, in the return value from runstate_get(). This
function passes responsibility to the application to run the service
as required, allowing the first function to actually be of use. See
the second commit message for details.

This patchset is in response to rework on the Eventdev SW PMD tests[1],
which made it clear that there is a need for applications to be able
to run exactly one iteration of a service exactly when required.

Regards, -Harry

[1] http://dpdk.org/dev/patchwork/patch/30666/

---

v2:
- Rebase on master to fix version.map file conflicts
- Reword function parameter for clarity (Pavan)
- Add Acks from ML

Harry van Haaren (2):
  service: add function for app lcore to run service
  service: add runtime service core check disable

 lib/librte_eal/common/include/rte_service.h | 44 +++++++++++++++-
 lib/librte_eal/common/rte_service.c         | 81 +++++++++++++++++++++--------
 lib/librte_eal/rte_eal_version.map          |  2 +
 3 files changed, 103 insertions(+), 24 deletions(-)

-- 
2.7.4

[dpdk-dev] [PATCH v2 1/2] service: add function for app lcore to run service

[Harry van Haaren]

This commit adds a new function which allows an application lcore
(aka; *not* a dedicated service-lcore) to run a service. This
function is required to allow applications to gradually port
functionality to using services, while still being able to run
ordinary application functions.

This requirement became clear when a patch to the existing
eventdev/pipeline sample app was modified to use a service-core
for scheduling - and that same core should be capable of running
the "worker()" function from the application too.

This patch refactors the existing running code into a smaller
"service_run" function, which can be called by the dedicated
service-core loop, and the newly added function.

Signed-off-by: Harry van Haaren <harry.van.haaren@intel.com>
Acked-by: Pavan Nikhilesh <pbhagavatula@caviumnetworks.com>

[1] http://dpdk.org/ml/archives/dev/2017-October/079876.html

---

v2:
- Rebase on master to resolve version.map conflict
- Add Ack

---
 lib/librte_eal/common/include/rte_service.h | 21 ++++++++++
 lib/librte_eal/common/rte_service.c         | 59 +++++++++++++++++++----------
 lib/librte_eal/rte_eal_version.map          |  1 +
 3 files changed, 60 insertions(+), 21 deletions(-)

diff --git a/lib/librte_eal/common/include/rte_service.h b/lib/librte_eal/common/include/rte_service.h
index 21da739..63d3170 100644
--- a/lib/librte_eal/common/include/rte_service.h
+++ b/lib/librte_eal/common/include/rte_service.h
@@ -211,6 +211,27 @@ int32_t rte_service_runstate_get(uint32_t id);
  * @warning
  * @b EXPERIMENTAL: this API may change without prior notice
  *
+ * This function runs a service callback from a non-service lcore context.
+ * The *id* of the service to be run is passed in, and the service-callback
+ * is executed on the calling lcore immediately if possible. If the service is
+ * not multi-thread capable and another thread is currently executing it, this
+ * function returns without running the callback.
+ *
+ * Note that any thread calling this function MUST be a DPDK EAL thread, as
+ * the *rte_lcore_id* function is used to access internal data structures.
+ *
+ * @retval 0 Service was run on the calling thread successfully
+ * @retval -EBUSY Another lcore is executing the service, and it is not a
+ *         multi-thread safe service, so the service was not run on this lcore
+ * @retval -ENOEXEC Service is not in a runnable state
+ * @retval -EINVAL Invalid service id
+ */
+int32_t rte_service_run_iter_on_app_lcore(uint32_t id);
+
+/**
+ * @warning
+ * @b EXPERIMENTAL: this API may change without prior notice
+ *
  * Start a service core.
  *
  * Starting a core makes the core begin polling. Any services assigned to it
diff --git a/lib/librte_eal/common/rte_service.c b/lib/librte_eal/common/rte_service.c
index e598e16..4e27f75 100644
--- a/lib/librte_eal/common/rte_service.c
+++ b/lib/librte_eal/common/rte_service.c
@@ -331,6 +331,42 @@ rte_service_runner_do_callback(struct rte_service_spec_impl *s,
 		s->spec.callback(userdata);
 }
 
+
+static inline int32_t
+service_run(uint32_t i, struct core_state *cs, uint64_t service_mask)
+{
+	if (!service_valid(i))
+		return -EINVAL;
+	struct rte_service_spec_impl *s = &rte_services[i];
+	if (s->comp_runstate != RUNSTATE_RUNNING ||
+			s->app_runstate != RUNSTATE_RUNNING ||
+			!(service_mask & (UINT64_C(1) << i)))
+		return -ENOEXEC;
+
+	/* check do we need cmpset, if MT safe or <= 1 core
+	 * mapped, atomic ops are not required.
+	 */
+	const int use_atomics = (service_mt_safe(s) == 0) &&
+				(s->num_mapped_cores > 1);
+	if (use_atomics) {
+		if (!rte_atomic32_cmpset((uint32_t *)&s->execute_lock, 0, 1))
+			return -EBUSY;
+
+		rte_service_runner_do_callback(s, cs, i);
+		rte_atomic32_clear(&s->execute_lock);
+	} else
+		rte_service_runner_do_callback(s, cs, i);
+
+	return 0;
+}
+
+int32_t rte_service_run_iter_on_app_lcore(uint32_t id)
+{
+	/* run service on calling core, using all-ones as the service mask */
+	struct core_state *cs = &lcore_states[rte_lcore_id()];
+	return service_run(id, cs, UINT64_MAX);
+}
+
 static int32_t
 rte_service_runner_func(void *arg)
 {
@@ -343,27 +379,8 @@ rte_service_runner_func(void *arg)
 		const uint64_t service_mask = cs->service_mask;
 
 		for (i = 0; i < RTE_SERVICE_NUM_MAX; i++) {
-			if (!service_valid(i))
-				continue;
-			struct rte_service_spec_impl *s = &rte_services[i];
-			if (s->comp_runstate != RUNSTATE_RUNNING ||
-					s->app_runstate != RUNSTATE_RUNNING ||
-					!(service_mask & (UINT64_C(1) << i)))
-				continue;
-
-			/* check do we need cmpset, if MT safe or <= 1 core
-			 * mapped, atomic ops are not required.
-			 */
-			const int use_atomics = (service_mt_safe(s) == 0) &&
-						(s->num_mapped_cores > 1);
-			if (use_atomics) {
-				uint32_t *lock = (uint32_t *)&s->execute_lock;
-				if (rte_atomic32_cmpset(lock, 0, 1)) {
-					rte_service_runner_do_callback(s, cs, i);
-					rte_atomic32_clear(&s->execute_lock);
-				}
-			} else
-				rte_service_runner_do_callback(s, cs, i);
+			/* return value ignored as no change to code flow */
+			service_run(i, cs, service_mask);
 		}
 
 		rte_smp_rmb();
diff --git a/lib/librte_eal/rte_eal_version.map b/lib/librte_eal/rte_eal_version.map
index c0df978..1ce50ab 100644
--- a/lib/librte_eal/rte_eal_version.map
+++ b/lib/librte_eal/rte_eal_version.map
@@ -242,6 +242,7 @@ EXPERIMENTAL {
 	rte_service_map_lcore_set;
 	rte_service_probe_capability;
 	rte_service_reset;
+	rte_service_run_iter_on_app_lcore;
 	rte_service_runstate_get;
 	rte_service_runstate_set;
 	rte_service_set_stats_enable;
-- 
2.7.4

[dpdk-dev] [PATCH v2 2/2] service: add runtime service core check disable

[Harry van Haaren]

This commit adds a new function to disable the runtime mapped
service-cores check. This allows an application to take responsibility
of running unmapped services.

This feature is useful in cases like unit tests, where the application
code (or unit test in this case) requires accurate control over when
the service function is called to ensure correct behaviour, and when
an application has an advanced use-case and wishes to manage services
manually.

Signed-off-by: Harry van Haaren <harry.van.haaren@intel.com>
Acked-by: Pavan Nikhilesh <pbhagavatula@caviumnetworks.com>

---

v2:
- Rebase on master to fix .map version file conflits
- Reword argument "enabled" to "enable" in function header (Pavan)
- Add Ack

---
 lib/librte_eal/common/include/rte_service.h | 23 ++++++++++++++++++++++-
 lib/librte_eal/common/rte_service.c         | 22 ++++++++++++++++++++--
 lib/librte_eal/rte_eal_version.map          |  1 +
 3 files changed, 43 insertions(+), 3 deletions(-)

diff --git a/lib/librte_eal/common/include/rte_service.h b/lib/librte_eal/common/include/rte_service.h
index 63d3170..d9de5ad 100644
--- a/lib/librte_eal/common/include/rte_service.h
+++ b/lib/librte_eal/common/include/rte_service.h
@@ -199,7 +199,12 @@ int32_t rte_service_runstate_set(uint32_t id, uint32_t runstate);
  * @b EXPERIMENTAL: this API may change without prior notice
  *
  * Get the runstate for the service with *id*. See *rte_service_runstate_set*
- * for details of runstates.
+ * for details of runstates. A service can call this function to ensure that
+ * the application has indicated that it will receive CPU cycles. Either a
+ * service-core is mapped (default case), or the application has explicitly
+ * disabled the check that a service-cores is mapped to the service and takes
+ * responsibility to run the service manually using the available function
+ * *rte_service_run_iter_on_app_lcore* to do so.
  *
  * @retval 1 Service is running
  * @retval 0 Service is stopped
@@ -211,6 +216,22 @@ int32_t rte_service_runstate_get(uint32_t id);
  * @warning
  * @b EXPERIMENTAL: this API may change without prior notice
  *
+ * Enable or disable the check for a service-core being mapped to the service.
+ * An application can disable the check when takes the responsibility to run a
+ * service itself using *rte_service_run_iter_on_app_lcore*.
+ *
+ * @param id The id of the service to set the check on
+ * @param enable When zero, the check is disabled. Non-zero enables the check.
+ *
+ * @retval 0 Success
+ * @retval -EINVAL Invalid service ID
+ */
+int32_t rte_service_set_runstate_mapped_check(uint32_t id, int32_t enable);
+
+/**
+ * @warning
+ * @b EXPERIMENTAL: this API may change without prior notice
+ *
  * This function runs a service callback from a non-service lcore context.
  * The *id* of the service to be run is passed in, and the service-callback
  * is executed on the calling lcore immediately if possible. If the service is
diff --git a/lib/librte_eal/common/rte_service.c b/lib/librte_eal/common/rte_service.c
index 4e27f75..f17bf4b 100644
--- a/lib/librte_eal/common/rte_service.c
+++ b/lib/librte_eal/common/rte_service.c
@@ -54,6 +54,7 @@
 
 #define SERVICE_F_REGISTERED    (1 << 0)
 #define SERVICE_F_STATS_ENABLED (1 << 1)
+#define SERVICE_F_START_CHECK   (1 << 2)
 
 /* runstates for services and lcores, denoting if they are active or not */
 #define RUNSTATE_STOPPED 0
@@ -180,6 +181,19 @@ int32_t rte_service_set_stats_enable(uint32_t id, int32_t enabled)
 	return 0;
 }
 
+int32_t rte_service_set_runstate_mapped_check(uint32_t id, int32_t enabled)
+{
+	struct rte_service_spec_impl *s;
+	SERVICE_VALID_GET_OR_ERR_RET(id, s, 0);
+
+	if (enabled)
+		s->internal_flags |= SERVICE_F_START_CHECK;
+	else
+		s->internal_flags &= ~(SERVICE_F_START_CHECK);
+
+	return 0;
+}
+
 uint32_t
 rte_service_get_count(void)
 {
@@ -241,7 +255,7 @@ rte_service_component_register(const struct rte_service_spec *spec,
 
 	struct rte_service_spec_impl *s = &rte_services[free_slot];
 	s->spec = *spec;
-	s->internal_flags |= SERVICE_F_REGISTERED;
+	s->internal_flags |= SERVICE_F_REGISTERED | SERVICE_F_START_CHECK;
 
 	rte_smp_wmb();
 	rte_service_count++;
@@ -309,9 +323,13 @@ rte_service_runstate_get(uint32_t id)
 	struct rte_service_spec_impl *s;
 	SERVICE_VALID_GET_OR_ERR_RET(id, s, -EINVAL);
 	rte_smp_rmb();
+
+	int check_disabled = !(s->internal_flags & SERVICE_F_START_CHECK);
+	int lcore_mapped = (s->num_mapped_cores > 0);
+
 	return (s->app_runstate == RUNSTATE_RUNNING) &&
 		(s->comp_runstate == RUNSTATE_RUNNING) &&
-		(s->num_mapped_cores > 0);
+		(check_disabled | lcore_mapped);
 }
 
 static inline void
diff --git a/lib/librte_eal/rte_eal_version.map b/lib/librte_eal/rte_eal_version.map
index 1ce50ab..ff14fc4 100644
--- a/lib/librte_eal/rte_eal_version.map
+++ b/lib/librte_eal/rte_eal_version.map
@@ -245,6 +245,7 @@ EXPERIMENTAL {
 	rte_service_run_iter_on_app_lcore;
 	rte_service_runstate_get;
 	rte_service_runstate_set;
+	rte_service_set_runstate_mapped_check;
 	rte_service_set_stats_enable;
 	rte_service_start_with_defaults;
 
-- 
2.7.4

Re: [dpdk-dev] [PATCH v2 0/2] service: enable app lcore to run service iter

[Thomas Monjalon]

> Harry van Haaren (2):
>   service: add function for app lcore to run service
>   service: add runtime service core check disable

Applied, thanks