* [dpdk-dev] [PATCH 0/8] service: rework for usability
@ 2017-08-15 12:32 Harry van Haaren
2017-08-15 12:32 ` [dpdk-dev] [PATCH 1/8] service: rework probe and get name to use ids Harry van Haaren
` (9 more replies)
0 siblings, 10 replies; 32+ messages in thread
From: Harry van Haaren @ 2017-08-15 12:32 UTC (permalink / raw)
To: dev; +Cc: Harry van Haaren
This patchset reworks the service apis to be more user
friendly. In particular, the various rte_service_* functions
now take an integer id parameter instead of a service pointer.
This both reduces the API surface (no service_get_from_id()),
and allows easier debugging (gdb function calls with integer args),
and various other benefits (better encapsulation, less pointers :)
Finally, some APIs are changed or renamed for consistency and
clarity of what they do. See commit messages for details.
Note that the service library is merged as EXPERIMENTAL in
the 17.08 release, allowing API improvements for 17.11 release.
I hope to merge this patchset early in the 17.11 timeframe,
so please review ASAP to allow time for other DPDK components
to utilize services in this release :)
Feedback and input welcome, -Harry
---
There is one checkpatch warning: "macro with flow control", however
this same type of macro is used extensively in Ethdev and others,
I presume it is a false-positive.
Harry van Haaren (8):
service: rework probe and get name to use ids
service: rework lcore to service map functions
service: rework register to return service id
service: rework service start stop to runstate
service: rework service stats functions
service: rework unregister api to use integers
service: rework get by name function to use id
service: clarify documentation for register
drivers/event/sw/sw_evdev.c | 7 +-
drivers/event/sw/sw_evdev.h | 1 +
lib/librte_eal/bsdapp/eal/rte_eal_version.map | 11 +-
lib/librte_eal/common/include/rte_service.h | 144 +++++++-----------
.../common/include/rte_service_component.h | 13 +-
lib/librte_eal/common/rte_service.c | 167 +++++++++------------
lib/librte_eal/linuxapp/eal/rte_eal_version.map | 11 +-
test/test/test_service_cores.c | 123 +++++++--------
8 files changed, 215 insertions(+), 262 deletions(-)
--
2.7.4
^ permalink raw reply [flat|nested] 32+ messages in thread
* [dpdk-dev] [PATCH 1/8] service: rework probe and get name to use ids
2017-08-15 12:32 [dpdk-dev] [PATCH 0/8] service: rework for usability Harry van Haaren
@ 2017-08-15 12:32 ` Harry van Haaren
2017-08-15 12:32 ` [dpdk-dev] [PATCH 2/8] service: rework lcore to service map functions Harry van Haaren
` (8 subsequent siblings)
9 siblings, 0 replies; 32+ messages in thread
From: Harry van Haaren @ 2017-08-15 12:32 UTC (permalink / raw)
To: dev; +Cc: Harry van Haaren
This commit adds a macro to easily validate a service ID, and then
lookup the service pointer, or return a user-specified error code.
This marco will be heavily used in the following patches as it will
be ID based instead of pointer-based.
The probe_capability function is reworked to use an integer ID instead
of a pointer. Rework the service_get_name() function is updated to use
IDs. Unit tests are updated to keep things compiling after each commit.
Signed-off-by: Harry van Haaren <harry.van.haaren@intel.com>
---
lib/librte_eal/common/include/rte_service.h | 5 ++---
lib/librte_eal/common/rte_service.c | 20 +++++++++++++++-----
test/test/test_service_cores.c | 7 +++----
3 files changed, 20 insertions(+), 12 deletions(-)
diff --git a/lib/librte_eal/common/include/rte_service.h b/lib/librte_eal/common/include/rte_service.h
index 7c6f738..bed1a61 100644
--- a/lib/librte_eal/common/include/rte_service.h
+++ b/lib/librte_eal/common/include/rte_service.h
@@ -133,7 +133,7 @@ struct rte_service_spec *rte_service_get_by_name(const char *name);
* @return A pointer to the name of the service. The returned pointer remains
* in ownership of the service, and the application must not free it.
*/
-const char *rte_service_get_name(const struct rte_service_spec *service);
+const char *rte_service_get_name(uint32_t id);
/**
* @warning
@@ -146,8 +146,7 @@ const char *rte_service_get_name(const struct rte_service_spec *service);
* @retval 1 Capability supported by this service instance
* @retval 0 Capability not supported by this service instance
*/
-int32_t rte_service_probe_capability(const struct rte_service_spec *service,
- uint32_t capability);
+int32_t rte_service_probe_capability(uint32_t id, uint32_t capability);
/**
* @warning
diff --git a/lib/librte_eal/common/rte_service.c b/lib/librte_eal/common/rte_service.c
index 7efb76d..c969406 100644
--- a/lib/librte_eal/common/rte_service.c
+++ b/lib/librte_eal/common/rte_service.c
@@ -144,6 +144,13 @@ service_valid(uint32_t id)
return !!(rte_services[id].internal_flags & SERVICE_F_REGISTERED);
}
+/* validate ID and retrieve service pointer, or return error value */
+#define SERVICE_VALID_GET_OR_ERR_RET(id, service, retval) do { \
+ if (id >= RTE_SERVICE_NUM_MAX || !service_valid(id)) \
+ return retval; \
+ service = &rte_services[id]; \
+} while (0)
+
/* returns 1 if statistics should be colleced for service
* Returns 0 if statistics should not be collected for service
*/
@@ -207,16 +214,19 @@ struct rte_service_spec *rte_service_get_by_name(const char *name)
}
const char *
-rte_service_get_name(const struct rte_service_spec *service)
+rte_service_get_name(uint32_t id)
{
- return service->name;
+ struct rte_service_spec_impl *s;
+ SERVICE_VALID_GET_OR_ERR_RET(id, s, 0);
+ return s->spec.name;
}
int32_t
-rte_service_probe_capability(const struct rte_service_spec *service,
- uint32_t capability)
+rte_service_probe_capability(uint32_t id, uint32_t capability)
{
- return service->capabilities & capability;
+ struct rte_service_spec_impl *s;
+ SERVICE_VALID_GET_OR_ERR_RET(id, s, -EINVAL);
+ return s->spec.capabilities & capability;
}
int32_t
diff --git a/test/test/test_service_cores.c b/test/test/test_service_cores.c
index 88fac8f..940bc62 100644
--- a/test/test/test_service_cores.c
+++ b/test/test/test_service_cores.c
@@ -225,8 +225,8 @@ service_probe_capability(void)
"Register of MT SAFE service failed");
/* verify flag is enabled */
- struct rte_service_spec *s = rte_service_get_by_id(0);
- int32_t mt = rte_service_probe_capability(s, RTE_SERVICE_CAP_MT_SAFE);
+ const uint32_t sid = 0;
+ int32_t mt = rte_service_probe_capability(sid, RTE_SERVICE_CAP_MT_SAFE);
TEST_ASSERT_EQUAL(1, mt, "MT SAFE capability flag not set.");
@@ -239,8 +239,7 @@ service_probe_capability(void)
"Register of non-MT safe service failed");
/* verify flag is enabled */
- s = rte_service_get_by_id(0);
- mt = rte_service_probe_capability(s, RTE_SERVICE_CAP_MT_SAFE);
+ mt = rte_service_probe_capability(sid, RTE_SERVICE_CAP_MT_SAFE);
TEST_ASSERT_EQUAL(0, mt, "MT SAFE cap flag set on non MT SAFE service");
return unregister_all();
--
2.7.4
^ permalink raw reply [flat|nested] 32+ messages in thread
* [dpdk-dev] [PATCH 2/8] service: rework lcore to service map functions
2017-08-15 12:32 [dpdk-dev] [PATCH 0/8] service: rework for usability Harry van Haaren
2017-08-15 12:32 ` [dpdk-dev] [PATCH 1/8] service: rework probe and get name to use ids Harry van Haaren
@ 2017-08-15 12:32 ` Harry van Haaren
2017-08-15 12:32 ` [dpdk-dev] [PATCH 3/8] service: rework register to return service id Harry van Haaren
` (7 subsequent siblings)
9 siblings, 0 replies; 32+ messages in thread
From: Harry van Haaren @ 2017-08-15 12:32 UTC (permalink / raw)
To: dev; +Cc: Harry van Haaren
This commit updates the APIs exposed to map service cores and
services. The previous APIs required a pointer to a service,
and used two seperate functions for enable and disable. The
new API uses an integer ID for the service and has a parameter
for map or unmap. Unit tests are updated and passing, and the
map file is updated to the new function names.
Signed-off-by: Harry van Haaren <harry.van.haaren@intel.com>
---
lib/librte_eal/bsdapp/eal/rte_eal_version.map | 5 ++-
lib/librte_eal/common/include/rte_service.h | 43 +++++++++----------------
lib/librte_eal/common/rte_service.c | 31 ++++++++----------
lib/librte_eal/linuxapp/eal/rte_eal_version.map | 5 ++-
test/test/test_service_cores.c | 27 +++++++++-------
5 files changed, 48 insertions(+), 63 deletions(-)
diff --git a/lib/librte_eal/bsdapp/eal/rte_eal_version.map b/lib/librte_eal/bsdapp/eal/rte_eal_version.map
index aac6fd7..f626a6f 100644
--- a/lib/librte_eal/bsdapp/eal/rte_eal_version.map
+++ b/lib/librte_eal/bsdapp/eal/rte_eal_version.map
@@ -212,13 +212,10 @@ EXPERIMENTAL {
rte_eal_devargs_remove;
rte_eal_hotplug_add;
rte_eal_hotplug_remove;
- rte_service_disable_on_lcore;
rte_service_dump;
- rte_service_enable_on_lcore;
rte_service_get_by_id;
rte_service_get_by_name;
rte_service_get_count;
- rte_service_get_enabled_on_lcore;
rte_service_is_running;
rte_service_lcore_add;
rte_service_lcore_count;
@@ -227,6 +224,8 @@ EXPERIMENTAL {
rte_service_lcore_reset_all;
rte_service_lcore_start;
rte_service_lcore_stop;
+ rte_service_map_lcore_get;
+ rte_service_map_lcore_set;
rte_service_probe_capability;
rte_service_register;
rte_service_reset;
diff --git a/lib/librte_eal/common/include/rte_service.h b/lib/librte_eal/common/include/rte_service.h
index bed1a61..de69695 100644
--- a/lib/librte_eal/common/include/rte_service.h
+++ b/lib/librte_eal/common/include/rte_service.h
@@ -152,10 +152,10 @@ int32_t rte_service_probe_capability(uint32_t id, uint32_t capability);
* @warning
* @b EXPERIMENTAL: this API may change without prior notice
*
- * Enable a core to run a service.
+ * Map or unmap a lcore to a service.
*
- * Each core can be added or removed from running specific services. This
- * functions adds *lcore* to the set of cores that will run *service*.
+ * Each core can be added or removed from running a specific service. This
+ * function enables or disables *lcore* to run *service_id*.
*
* If multiple cores are enabled on a service, an atomic is used to ensure that
* only one cores runs the service at a time. The exception to this is when
@@ -163,43 +163,30 @@ int32_t rte_service_probe_capability(uint32_t id, uint32_t capability);
* called RTE_SERVICE_CAP_MT_SAFE. With the multi-thread safe capability set,
* the service function can be run on multiple threads at the same time.
*
- * @retval 0 lcore added successfully
- * @retval -EINVAL An invalid service or lcore was provided.
- */
-int32_t rte_service_enable_on_lcore(struct rte_service_spec *service,
- uint32_t lcore);
-
-/**
- * @warning
- * @b EXPERIMENTAL: this API may change without prior notice
- *
- * Disable a core to run a service.
+ * @param service_id the service to apply the lcore to
+ * @param lcore The lcore that will be mapped to service
+ * @param enable Zero to unmap or disable the core, non-zero to enable
*
- * Each core can be added or removed from running specific services. This
- * functions removes *lcore* to the set of cores that will run *service*.
- *
- * @retval 0 Lcore removed successfully
+ * @retval 0 lcore map updated successfully
* @retval -EINVAL An invalid service or lcore was provided.
*/
-int32_t rte_service_disable_on_lcore(struct rte_service_spec *service,
- uint32_t lcore);
+int32_t rte_service_map_lcore_set(uint32_t service_id, uint32_t lcore,
+ uint32_t enable);
/**
* @warning
* @b EXPERIMENTAL: this API may change without prior notice
*
- * Return if an lcore is enabled for the service.
+ * Retrieve the mapping of an lcore to a service.
*
- * This function allows the application to query if *lcore* is currently set to
- * run *service*.
+ * @param service_id the service to apply the lcore to
+ * @param lcore The lcore that will be mapped to service
*
- * @retval 1 Lcore enabled on this lcore
- * @retval 0 Lcore disabled on this lcore
+ * @retval 1 lcore is mapped to service
+ * @retval 0 lcore is not mapped to service
* @retval -EINVAL An invalid service or lcore was provided.
*/
-int32_t rte_service_get_enabled_on_lcore(struct rte_service_spec *service,
- uint32_t lcore);
-
+int32_t rte_service_map_lcore_get(uint32_t service_id, uint32_t lcore);
/**
* @warning
diff --git a/lib/librte_eal/common/rte_service.c b/lib/librte_eal/common/rte_service.c
index c969406..c5a8d0d 100644
--- a/lib/librte_eal/common/rte_service.c
+++ b/lib/librte_eal/common/rte_service.c
@@ -436,7 +436,7 @@ rte_service_start_with_defaults(void)
* should multiplex to a single core, or 1:1 if there are the
* same amount of services as service-cores
*/
- ret = rte_service_enable_on_lcore(s, ids[lcore_iter]);
+ ret = rte_service_map_lcore_set(i, ids[lcore_iter], 1);
if (ret)
return -ENODEV;
@@ -492,28 +492,25 @@ service_update(struct rte_service_spec *service, uint32_t lcore,
return 0;
}
-int32_t rte_service_get_enabled_on_lcore(struct rte_service_spec *service,
- uint32_t lcore)
-{
- uint32_t enabled;
- int ret = service_update(service, lcore, 0, &enabled);
- if (ret == 0)
- return enabled;
- return -EINVAL;
-}
-
int32_t
-rte_service_enable_on_lcore(struct rte_service_spec *service, uint32_t lcore)
+rte_service_map_lcore_set(uint32_t id, uint32_t lcore, uint32_t enabled)
{
- uint32_t on = 1;
- return service_update(service, lcore, &on, 0);
+ struct rte_service_spec_impl *s;
+ SERVICE_VALID_GET_OR_ERR_RET(id, s, -EINVAL);
+ uint32_t on = enabled > 0;
+ return service_update(&s->spec, lcore, &on, 0);
}
int32_t
-rte_service_disable_on_lcore(struct rte_service_spec *service, uint32_t lcore)
+rte_service_map_lcore_get(uint32_t id, uint32_t lcore)
{
- uint32_t off = 0;
- return service_update(service, lcore, &off, 0);
+ struct rte_service_spec_impl *s;
+ SERVICE_VALID_GET_OR_ERR_RET(id, s, -EINVAL);
+ uint32_t enabled;
+ int ret = service_update(&s->spec, lcore, 0, &enabled);
+ if (ret == 0)
+ return enabled;
+ return ret;
}
int32_t rte_service_lcore_reset_all(void)
diff --git a/lib/librte_eal/linuxapp/eal/rte_eal_version.map b/lib/librte_eal/linuxapp/eal/rte_eal_version.map
index 3a8f154..452ae1c 100644
--- a/lib/librte_eal/linuxapp/eal/rte_eal_version.map
+++ b/lib/librte_eal/linuxapp/eal/rte_eal_version.map
@@ -217,13 +217,10 @@ EXPERIMENTAL {
rte_eal_devargs_remove;
rte_eal_hotplug_add;
rte_eal_hotplug_remove;
- rte_service_disable_on_lcore;
rte_service_dump;
- rte_service_enable_on_lcore;
rte_service_get_by_id;
rte_service_get_by_name;
rte_service_get_count;
- rte_service_get_enabled_on_lcore;
rte_service_is_running;
rte_service_lcore_add;
rte_service_lcore_count;
@@ -232,6 +229,8 @@ EXPERIMENTAL {
rte_service_lcore_reset_all;
rte_service_lcore_start;
rte_service_lcore_stop;
+ rte_service_map_lcore_get;
+ rte_service_map_lcore_set;
rte_service_probe_capability;
rte_service_register;
rte_service_reset;
diff --git a/test/test/test_service_cores.c b/test/test/test_service_cores.c
index 940bc62..fd63efd 100644
--- a/test/test/test_service_cores.c
+++ b/test/test/test_service_cores.c
@@ -273,12 +273,13 @@ service_dump(void)
static int
service_start_stop(void)
{
+ const uint32_t sid = 0;
struct rte_service_spec *service = rte_service_get_by_id(0);
/* is_running() returns if service is running and slcore is mapped */
TEST_ASSERT_EQUAL(0, rte_service_lcore_add(slcore_id),
"Service core add did not return zero");
- int ret = rte_service_enable_on_lcore(service, slcore_id);
+ int ret = rte_service_map_lcore_set(sid, slcore_id, 1);
TEST_ASSERT_EQUAL(0, ret,
"Enabling service core, expected 0 got %d", ret);
@@ -313,12 +314,12 @@ service_remote_launch_func(void *arg)
static int
service_lcore_en_dis_able(void)
{
- struct rte_service_spec *s = rte_service_get_by_id(0);
+ const uint32_t sid = 0;
/* expected failure cases */
- TEST_ASSERT_EQUAL(-EINVAL, rte_service_enable_on_lcore(s, 100000),
+ TEST_ASSERT_EQUAL(-EINVAL, rte_service_map_lcore_set(sid, 100000, 1),
"Enable on invalid core did not fail");
- TEST_ASSERT_EQUAL(-EINVAL, rte_service_disable_on_lcore(s, 100000),
+ TEST_ASSERT_EQUAL(-EINVAL, rte_service_map_lcore_set(sid, 100000, 0),
"Disable on invalid core did not fail");
/* add service core to allow enabling */
@@ -326,15 +327,15 @@ service_lcore_en_dis_able(void)
"Add service core failed when not in use before");
/* valid enable */
- TEST_ASSERT_EQUAL(0, rte_service_enable_on_lcore(s, slcore_id),
+ TEST_ASSERT_EQUAL(0, rte_service_map_lcore_set(sid, slcore_id, 1),
"Enabling valid service and core failed");
- TEST_ASSERT_EQUAL(1, rte_service_get_enabled_on_lcore(s, slcore_id),
+ TEST_ASSERT_EQUAL(1, rte_service_map_lcore_get(sid, slcore_id),
"Enabled core returned not-enabled");
/* valid disable */
- TEST_ASSERT_EQUAL(0, rte_service_disable_on_lcore(s, slcore_id),
+ TEST_ASSERT_EQUAL(0, rte_service_map_lcore_set(sid, slcore_id, 0),
"Disabling valid service and lcore failed");
- TEST_ASSERT_EQUAL(0, rte_service_get_enabled_on_lcore(s, slcore_id),
+ TEST_ASSERT_EQUAL(0, rte_service_map_lcore_get(sid, slcore_id),
"Disabled core returned enabled");
/* call remote_launch to verify that app can launch ex-service lcore */
@@ -474,11 +475,12 @@ service_threaded_test(int mt_safe)
"Register of MT SAFE service failed");
struct rte_service_spec *s = rte_service_get_by_id(0);
+ const uint32_t sid = 0;
TEST_ASSERT_EQUAL(0, rte_service_start(s),
"Starting valid service failed");
- TEST_ASSERT_EQUAL(0, rte_service_enable_on_lcore(s, slcore_1),
+ TEST_ASSERT_EQUAL(0, rte_service_map_lcore_set(sid, slcore_1, 1),
"Failed to enable lcore 1 on mt safe service");
- TEST_ASSERT_EQUAL(0, rte_service_enable_on_lcore(s, slcore_2),
+ TEST_ASSERT_EQUAL(0, rte_service_map_lcore_set(sid, slcore_2, 1),
"Failed to enable lcore 2 on mt safe service");
rte_service_lcore_start(slcore_1);
rte_service_lcore_start(slcore_2);
@@ -529,10 +531,11 @@ static int
service_lcore_start_stop(void)
{
/* start service core and service, create mapping so tick() runs */
+ const uint32_t sid = 0;
struct rte_service_spec *s = rte_service_get_by_id(0);
TEST_ASSERT_EQUAL(0, rte_service_start(s),
"Starting valid service failed");
- TEST_ASSERT_EQUAL(-EINVAL, rte_service_enable_on_lcore(s, slcore_id),
+ TEST_ASSERT_EQUAL(-EINVAL, rte_service_map_lcore_set(sid, slcore_id, 1),
"Enabling valid service on non-service core must fail");
/* core start */
@@ -540,7 +543,7 @@ service_lcore_start_stop(void)
"Service core start without add should return EINVAL");
TEST_ASSERT_EQUAL(0, rte_service_lcore_add(slcore_id),
"Service core add did not return zero");
- TEST_ASSERT_EQUAL(0, rte_service_enable_on_lcore(s, slcore_id),
+ TEST_ASSERT_EQUAL(0, rte_service_map_lcore_set(sid, slcore_id, 1),
"Enabling valid service on valid core failed");
TEST_ASSERT_EQUAL(0, rte_service_lcore_start(slcore_id),
"Service core start after add failed");
--
2.7.4
^ permalink raw reply [flat|nested] 32+ messages in thread
* [dpdk-dev] [PATCH 3/8] service: rework register to return service id
2017-08-15 12:32 [dpdk-dev] [PATCH 0/8] service: rework for usability Harry van Haaren
2017-08-15 12:32 ` [dpdk-dev] [PATCH 1/8] service: rework probe and get name to use ids Harry van Haaren
2017-08-15 12:32 ` [dpdk-dev] [PATCH 2/8] service: rework lcore to service map functions Harry van Haaren
@ 2017-08-15 12:32 ` Harry van Haaren
2017-08-15 12:32 ` [dpdk-dev] [PATCH 4/8] service: rework service start stop to runstate Harry van Haaren
` (6 subsequent siblings)
9 siblings, 0 replies; 32+ messages in thread
From: Harry van Haaren @ 2017-08-15 12:32 UTC (permalink / raw)
To: dev; +Cc: Harry van Haaren
This commit reworks the service register function to accept
an extra parameter. The parameter is a uint32_t *, which when
provided will be set to the integer service_id that the newly
registered service is represented by.
This is useful for services that wish to validate settings at
a later point in time - they need to know thier own service id.
This commit updates the eventdev sw pmd, as well as unit tests
to use the new register API.
Signed-off-by: Harry van Haaren <harry.van.haaren@intel.com>
---
drivers/event/sw/sw_evdev.c | 4 +--
drivers/event/sw/sw_evdev.h | 1 +
.../common/include/rte_service_component.h | 6 +++-
lib/librte_eal/common/rte_service.c | 5 +++-
test/test/test_service_cores.c | 32 +++++++++++-----------
5 files changed, 28 insertions(+), 20 deletions(-)
diff --git a/drivers/event/sw/sw_evdev.c b/drivers/event/sw/sw_evdev.c
index 9c534b7..819d3ef 100644
--- a/drivers/event/sw/sw_evdev.c
+++ b/drivers/event/sw/sw_evdev.c
@@ -620,7 +620,7 @@ sw_start(struct rte_eventdev *dev)
struct rte_service_spec *s = rte_service_get_by_name(sw->service_name);
if (!rte_service_is_running(s))
SW_LOG_ERR("Warning: No Service core enabled on service %s\n",
- s->name);
+ sw->service_name);
/* check all ports are set up */
for (i = 0; i < sw->port_count; i++)
@@ -855,7 +855,7 @@ sw_probe(struct rte_vdev_device *vdev)
service.callback = sw_sched_service_func;
service.callback_userdata = (void *)dev;
- int32_t ret = rte_service_register(&service);
+ int32_t ret = rte_service_register(&service, &sw->service_id);
if (ret) {
SW_LOG_ERR("service register() failed");
return -ENOEXEC;
diff --git a/drivers/event/sw/sw_evdev.h b/drivers/event/sw/sw_evdev.h
index 71de3c1..e0dec91 100644
--- a/drivers/event/sw/sw_evdev.h
+++ b/drivers/event/sw/sw_evdev.h
@@ -278,6 +278,7 @@ struct sw_evdev {
uint16_t xstats_count_per_qid[RTE_EVENT_MAX_QUEUES_PER_DEV];
uint16_t xstats_offset_for_qid[RTE_EVENT_MAX_QUEUES_PER_DEV];
+ uint32_t service_id;
char service_name[SW_PMD_NAME_MAX];
};
diff --git a/lib/librte_eal/common/include/rte_service_component.h b/lib/librte_eal/common/include/rte_service_component.h
index 7a946a1..5ba5cdf 100644
--- a/lib/librte_eal/common/include/rte_service_component.h
+++ b/lib/librte_eal/common/include/rte_service_component.h
@@ -89,11 +89,15 @@ struct rte_service_spec {
* *rte_service_set_coremask*.
*
* @param spec The specification of the service to register
+ * @param[out] service_id A pointer to a uint32_t, which will be filled in
+ * during registration of the service. It is set to the integers
+ * service number given to the service. This parameter may be NULL.
* @retval 0 Successfully registered the service.
* -EINVAL Attempted to register an invalid service (eg, no callback
* set)
*/
-int32_t rte_service_register(const struct rte_service_spec *spec);
+int32_t rte_service_register(const struct rte_service_spec *spec,
+ uint32_t *service_id);
/**
* @warning
diff --git a/lib/librte_eal/common/rte_service.c b/lib/librte_eal/common/rte_service.c
index c5a8d0d..0ee0c13 100644
--- a/lib/librte_eal/common/rte_service.c
+++ b/lib/librte_eal/common/rte_service.c
@@ -242,7 +242,7 @@ rte_service_is_running(const struct rte_service_spec *spec)
}
int32_t
-rte_service_register(const struct rte_service_spec *spec)
+rte_service_register(const struct rte_service_spec *spec, uint32_t *id_ptr)
{
uint32_t i;
int32_t free_slot = -1;
@@ -267,6 +267,9 @@ rte_service_register(const struct rte_service_spec *spec)
rte_smp_wmb();
rte_service_count++;
+ if (id_ptr)
+ *id_ptr = free_slot;
+
return 0;
}
diff --git a/test/test/test_service_cores.c b/test/test/test_service_cores.c
index fd63efd..72d774d 100644
--- a/test/test/test_service_cores.c
+++ b/test/test/test_service_cores.c
@@ -160,15 +160,15 @@ dummy_register(void)
struct rte_service_spec service;
memset(&service, 0, sizeof(struct rte_service_spec));
- TEST_ASSERT_EQUAL(-EINVAL, rte_service_register(&service),
+ TEST_ASSERT_EQUAL(-EINVAL, rte_service_register(&service, NULL),
"Invalid callback");
service.callback = dummy_cb;
- TEST_ASSERT_EQUAL(-EINVAL, rte_service_register(&service),
+ TEST_ASSERT_EQUAL(-EINVAL, rte_service_register(&service, NULL),
"Invalid name");
snprintf(service.name, sizeof(service.name), DUMMY_SERVICE_NAME);
- TEST_ASSERT_EQUAL(0, rte_service_register(&service),
+ TEST_ASSERT_EQUAL(0, rte_service_register(&service, NULL),
"Failed to register valid service");
return TEST_SUCCESS;
@@ -187,13 +187,13 @@ service_get_by_name(void)
/* register service */
struct rte_service_spec service;
memset(&service, 0, sizeof(struct rte_service_spec));
- TEST_ASSERT_EQUAL(-EINVAL, rte_service_register(&service),
+ TEST_ASSERT_EQUAL(-EINVAL, rte_service_register(&service, NULL),
"Invalid callback");
service.callback = dummy_cb;
- TEST_ASSERT_EQUAL(-EINVAL, rte_service_register(&service),
+ TEST_ASSERT_EQUAL(-EINVAL, rte_service_register(&service, NULL),
"Invalid name");
snprintf(service.name, sizeof(service.name), DUMMY_SERVICE_NAME);
- TEST_ASSERT_EQUAL(0, rte_service_register(&service),
+ TEST_ASSERT_EQUAL(0, rte_service_register(&service, NULL),
"Failed to register valid service");
/* ensure with dummy services registered returns same ptr as ID */
@@ -221,7 +221,7 @@ service_probe_capability(void)
service.callback = dummy_cb;
snprintf(service.name, sizeof(service.name), DUMMY_SERVICE_NAME);
service.capabilities |= RTE_SERVICE_CAP_MT_SAFE;
- TEST_ASSERT_EQUAL(0, rte_service_register(&service),
+ TEST_ASSERT_EQUAL(0, rte_service_register(&service, NULL),
"Register of MT SAFE service failed");
/* verify flag is enabled */
@@ -235,7 +235,7 @@ service_probe_capability(void)
memset(&service, 0, sizeof(struct rte_service_spec));
service.callback = dummy_cb;
snprintf(service.name, sizeof(service.name), DUMMY_SERVICE_NAME);
- TEST_ASSERT_EQUAL(0, rte_service_register(&service),
+ TEST_ASSERT_EQUAL(0, rte_service_register(&service, NULL),
"Register of non-MT safe service failed");
/* verify flag is enabled */
@@ -274,28 +274,28 @@ static int
service_start_stop(void)
{
const uint32_t sid = 0;
- struct rte_service_spec *service = rte_service_get_by_id(0);
+ struct rte_service_spec *s = rte_service_get_by_id(0);
- /* is_running() returns if service is running and slcore is mapped */
+ /* runstate_get() returns if service is running and slcore is mapped */
TEST_ASSERT_EQUAL(0, rte_service_lcore_add(slcore_id),
"Service core add did not return zero");
int ret = rte_service_map_lcore_set(sid, slcore_id, 1);
TEST_ASSERT_EQUAL(0, ret,
"Enabling service core, expected 0 got %d", ret);
- TEST_ASSERT_EQUAL(0, rte_service_is_running(service),
+ TEST_ASSERT_EQUAL(0, rte_service_is_running(s),
"Error: Service should be stopped");
- TEST_ASSERT_EQUAL(0, rte_service_stop(service),
+ TEST_ASSERT_EQUAL(0, rte_service_stop(s),
"Error: Service stopped returned non-zero");
- TEST_ASSERT_EQUAL(0, rte_service_is_running(service),
+ TEST_ASSERT_EQUAL(0, rte_service_is_running(s),
"Error: Service is running - should be stopped");
- TEST_ASSERT_EQUAL(0, rte_service_start(service),
+ TEST_ASSERT_EQUAL(0, rte_service_start(s),
"Error: Service start returned non-zero");
- TEST_ASSERT_EQUAL(1, rte_service_is_running(service),
+ TEST_ASSERT_EQUAL(1, rte_service_is_running(s),
"Error: Service is not running");
return unregister_all();
@@ -471,7 +471,7 @@ service_threaded_test(int mt_safe)
service.callback = dummy_mt_unsafe_cb;
}
- TEST_ASSERT_EQUAL(0, rte_service_register(&service),
+ TEST_ASSERT_EQUAL(0, rte_service_register(&service, NULL),
"Register of MT SAFE service failed");
struct rte_service_spec *s = rte_service_get_by_id(0);
--
2.7.4
^ permalink raw reply [flat|nested] 32+ messages in thread
* [dpdk-dev] [PATCH 4/8] service: rework service start stop to runstate
2017-08-15 12:32 [dpdk-dev] [PATCH 0/8] service: rework for usability Harry van Haaren
` (2 preceding siblings ...)
2017-08-15 12:32 ` [dpdk-dev] [PATCH 3/8] service: rework register to return service id Harry van Haaren
@ 2017-08-15 12:32 ` Harry van Haaren
2017-08-15 12:32 ` [dpdk-dev] [PATCH 5/8] service: rework service stats functions Harry van Haaren
` (5 subsequent siblings)
9 siblings, 0 replies; 32+ messages in thread
From: Harry van Haaren @ 2017-08-15 12:32 UTC (permalink / raw)
To: dev; +Cc: Harry van Haaren
This commit reworks the API to move from two seperate start
and stop functions, to a "runstate" API which allows setting
the runstate. The is_running API is replaced with an function
to query the runstate. The runstate functions take a id value
for service. Unit tests and the eventdev sw pmd are updated.
Signed-off-by: Harry van Haaren <harry.van.haaren@intel.com>
---
drivers/event/sw/sw_evdev.c | 3 +-
lib/librte_eal/bsdapp/eal/rte_eal_version.map | 5 ++--
lib/librte_eal/common/include/rte_service.h | 37 +++++++++---------------
lib/librte_eal/common/rte_service.c | 38 ++++++++++---------------
lib/librte_eal/linuxapp/eal/rte_eal_version.map | 5 ++--
test/test/test_service_cores.c | 19 ++++++-------
6 files changed, 42 insertions(+), 65 deletions(-)
diff --git a/drivers/event/sw/sw_evdev.c b/drivers/event/sw/sw_evdev.c
index 819d3ef..fd2fb72 100644
--- a/drivers/event/sw/sw_evdev.c
+++ b/drivers/event/sw/sw_evdev.c
@@ -617,8 +617,7 @@ sw_start(struct rte_eventdev *dev)
struct sw_evdev *sw = sw_pmd_priv(dev);
/* check a service core is mapped to this service */
- struct rte_service_spec *s = rte_service_get_by_name(sw->service_name);
- if (!rte_service_is_running(s))
+ if (!rte_service_runstate_get(sw->service_id))
SW_LOG_ERR("Warning: No Service core enabled on service %s\n",
sw->service_name);
diff --git a/lib/librte_eal/bsdapp/eal/rte_eal_version.map b/lib/librte_eal/bsdapp/eal/rte_eal_version.map
index f626a6f..f76d1ea 100644
--- a/lib/librte_eal/bsdapp/eal/rte_eal_version.map
+++ b/lib/librte_eal/bsdapp/eal/rte_eal_version.map
@@ -216,7 +216,6 @@ EXPERIMENTAL {
rte_service_get_by_id;
rte_service_get_by_name;
rte_service_get_count;
- rte_service_is_running;
rte_service_lcore_add;
rte_service_lcore_count;
rte_service_lcore_del;
@@ -229,10 +228,10 @@ EXPERIMENTAL {
rte_service_probe_capability;
rte_service_register;
rte_service_reset;
+ rte_service_runstate_get;
+ rte_service_runstate_set;
rte_service_set_stats_enable;
- rte_service_start;
rte_service_start_with_defaults;
- rte_service_stop;
rte_service_unregister;
} DPDK_17.08;
diff --git a/lib/librte_eal/common/include/rte_service.h b/lib/librte_eal/common/include/rte_service.h
index de69695..e133ca1 100644
--- a/lib/librte_eal/common/include/rte_service.h
+++ b/lib/librte_eal/common/include/rte_service.h
@@ -192,40 +192,31 @@ int32_t rte_service_map_lcore_get(uint32_t service_id, uint32_t lcore);
* @warning
* @b EXPERIMENTAL: this API may change without prior notice
*
- * Enable *service* to run.
+ * Set the runstate of the service.
*
- * This function switches on a service during runtime.
- * @retval 0 The service was successfully started
- */
-int32_t rte_service_start(struct rte_service_spec *service);
-
-/**
- * @warning
- * @b EXPERIMENTAL: this API may change without prior notice
+ * Each service is either running or stopped. Setting a non-zero runstate
+ * enables the service to run, while setting runstate zero disables it.
*
- * Disable *service*.
+ * @param id The id of the service
+ * @param runstate The run state to apply to the service
*
- * Switch off a service, so it is not run until it is *rte_service_start* is
- * called on it.
- * @retval 0 Service successfully switched off
+ * @retval 0 The service was successfully started
+ * @retval -EINVAL Invalid service id
*/
-int32_t rte_service_stop(struct rte_service_spec *service);
+int32_t rte_service_runstate_set(uint32_t id, uint32_t runstate);
/**
* @warning
* @b EXPERIMENTAL: this API may change without prior notice
*
- * Returns if *service* is currently running.
- *
- * This function returns true if the service has been started using
- * *rte_service_start*, AND a service core is mapped to the service. This
- * function can be used to ensure that the service will be run.
+ * Get the runstate for the service with *id*. See *rte_service_runstate_set*
+ * for details of runstates.
*
- * @retval 1 Service is currently running, and has a service lcore mapped
- * @retval 0 Service is currently stopped, or no service lcore is mapped
- * @retval -EINVAL Invalid service pointer provided
+ * @retval 1 Service is running
+ * @retval 0 Service is stopped
+ * @retval -EINVAL Invalid service id
*/
-int32_t rte_service_is_running(const struct rte_service_spec *service);
+int32_t rte_service_runstate_get(uint32_t id);
/**
* @warning
diff --git a/lib/librte_eal/common/rte_service.c b/lib/librte_eal/common/rte_service.c
index 0ee0c13..7e9bf4d 100644
--- a/lib/librte_eal/common/rte_service.c
+++ b/lib/librte_eal/common/rte_service.c
@@ -230,18 +230,6 @@ rte_service_probe_capability(uint32_t id, uint32_t capability)
}
int32_t
-rte_service_is_running(const struct rte_service_spec *spec)
-{
- const struct rte_service_spec_impl *impl =
- (const struct rte_service_spec_impl *)spec;
- if (!impl)
- return -EINVAL;
-
- return (impl->runstate == RUNSTATE_RUNNING) &&
- (impl->num_mapped_cores > 0);
-}
-
-int32_t
rte_service_register(const struct rte_service_spec *spec, uint32_t *id_ptr)
{
uint32_t i;
@@ -308,23 +296,27 @@ rte_service_unregister(struct rte_service_spec *spec)
}
int32_t
-rte_service_start(struct rte_service_spec *service)
+rte_service_runstate_set(uint32_t id, uint32_t runstate)
{
- struct rte_service_spec_impl *s =
- (struct rte_service_spec_impl *)service;
- s->runstate = RUNSTATE_RUNNING;
+ struct rte_service_spec_impl *s;
+ SERVICE_VALID_GET_OR_ERR_RET(id, s, -EINVAL);
+
+ if (runstate)
+ s->runstate = RUNSTATE_RUNNING;
+ else
+ s->runstate = RUNSTATE_STOPPED;
+
rte_smp_wmb();
return 0;
}
int32_t
-rte_service_stop(struct rte_service_spec *service)
+rte_service_runstate_get(uint32_t id)
{
- struct rte_service_spec_impl *s =
- (struct rte_service_spec_impl *)service;
- s->runstate = RUNSTATE_STOPPED;
- rte_smp_wmb();
- return 0;
+ struct rte_service_spec_impl *s;
+ SERVICE_VALID_GET_OR_ERR_RET(id, s, -EINVAL);
+
+ return (s->runstate == RUNSTATE_RUNNING) && (s->num_mapped_cores > 0);
}
static int32_t
@@ -447,7 +439,7 @@ rte_service_start_with_defaults(void)
if (lcore_iter >= lcore_count)
lcore_iter = 0;
- ret = rte_service_start(s);
+ ret = rte_service_runstate_set(i, 1);
if (ret)
return -ENOEXEC;
}
diff --git a/lib/librte_eal/linuxapp/eal/rte_eal_version.map b/lib/librte_eal/linuxapp/eal/rte_eal_version.map
index 452ae1c..557b5e7 100644
--- a/lib/librte_eal/linuxapp/eal/rte_eal_version.map
+++ b/lib/librte_eal/linuxapp/eal/rte_eal_version.map
@@ -221,7 +221,6 @@ EXPERIMENTAL {
rte_service_get_by_id;
rte_service_get_by_name;
rte_service_get_count;
- rte_service_is_running;
rte_service_lcore_add;
rte_service_lcore_count;
rte_service_lcore_del;
@@ -234,10 +233,10 @@ EXPERIMENTAL {
rte_service_probe_capability;
rte_service_register;
rte_service_reset;
+ rte_service_runstate_get;
+ rte_service_runstate_set;
rte_service_set_stats_enable;
- rte_service_start;
rte_service_start_with_defaults;
- rte_service_stop;
rte_service_unregister;
} DPDK_17.08;
diff --git a/test/test/test_service_cores.c b/test/test/test_service_cores.c
index 72d774d..ba252c5 100644
--- a/test/test/test_service_cores.c
+++ b/test/test/test_service_cores.c
@@ -274,7 +274,6 @@ static int
service_start_stop(void)
{
const uint32_t sid = 0;
- struct rte_service_spec *s = rte_service_get_by_id(0);
/* runstate_get() returns if service is running and slcore is mapped */
TEST_ASSERT_EQUAL(0, rte_service_lcore_add(slcore_id),
@@ -283,19 +282,19 @@ service_start_stop(void)
TEST_ASSERT_EQUAL(0, ret,
"Enabling service core, expected 0 got %d", ret);
- TEST_ASSERT_EQUAL(0, rte_service_is_running(s),
+ TEST_ASSERT_EQUAL(0, rte_service_runstate_get(sid),
"Error: Service should be stopped");
- TEST_ASSERT_EQUAL(0, rte_service_stop(s),
+ TEST_ASSERT_EQUAL(0, rte_service_runstate_set(sid, 0),
"Error: Service stopped returned non-zero");
- TEST_ASSERT_EQUAL(0, rte_service_is_running(s),
+ TEST_ASSERT_EQUAL(0, rte_service_runstate_get(sid),
"Error: Service is running - should be stopped");
- TEST_ASSERT_EQUAL(0, rte_service_start(s),
+ TEST_ASSERT_EQUAL(0, rte_service_runstate_set(sid, 1),
"Error: Service start returned non-zero");
- TEST_ASSERT_EQUAL(1, rte_service_is_running(s),
+ TEST_ASSERT_EQUAL(1, rte_service_runstate_get(sid),
"Error: Service is not running");
return unregister_all();
@@ -474,9 +473,8 @@ service_threaded_test(int mt_safe)
TEST_ASSERT_EQUAL(0, rte_service_register(&service, NULL),
"Register of MT SAFE service failed");
- struct rte_service_spec *s = rte_service_get_by_id(0);
const uint32_t sid = 0;
- TEST_ASSERT_EQUAL(0, rte_service_start(s),
+ TEST_ASSERT_EQUAL(0, rte_service_runstate_set(sid, 1),
"Starting valid service failed");
TEST_ASSERT_EQUAL(0, rte_service_map_lcore_set(sid, slcore_1, 1),
"Failed to enable lcore 1 on mt safe service");
@@ -493,7 +491,7 @@ service_threaded_test(int mt_safe)
TEST_ASSERT_EQUAL(1, test_params[1],
"MT Safe service not run by two cores concurrently");
- TEST_ASSERT_EQUAL(0, rte_service_stop(s),
+ TEST_ASSERT_EQUAL(0, rte_service_runstate_set(sid, 0),
"Failed to stop MT Safe service");
unregister_all();
@@ -532,8 +530,7 @@ service_lcore_start_stop(void)
{
/* start service core and service, create mapping so tick() runs */
const uint32_t sid = 0;
- struct rte_service_spec *s = rte_service_get_by_id(0);
- TEST_ASSERT_EQUAL(0, rte_service_start(s),
+ TEST_ASSERT_EQUAL(0, rte_service_runstate_set(sid, 1),
"Starting valid service failed");
TEST_ASSERT_EQUAL(-EINVAL, rte_service_map_lcore_set(sid, slcore_id, 1),
"Enabling valid service on non-service core must fail");
--
2.7.4
^ permalink raw reply [flat|nested] 32+ messages in thread
* [dpdk-dev] [PATCH 5/8] service: rework service stats functions
2017-08-15 12:32 [dpdk-dev] [PATCH 0/8] service: rework for usability Harry van Haaren
` (3 preceding siblings ...)
2017-08-15 12:32 ` [dpdk-dev] [PATCH 4/8] service: rework service start stop to runstate Harry van Haaren
@ 2017-08-15 12:32 ` Harry van Haaren
2017-08-15 12:32 ` [dpdk-dev] [PATCH 6/8] service: rework unregister api to use integers Harry van Haaren
` (4 subsequent siblings)
9 siblings, 0 replies; 32+ messages in thread
From: Harry van Haaren @ 2017-08-15 12:32 UTC (permalink / raw)
To: dev; +Cc: Harry van Haaren
This commit reworks the statistics functions to use integer ids
for services instead of pointers. Passing UINT32_MAX to the dump
function prints all info, similar to passing NULL previously.
Signed-off-by: Harry van Haaren <harry.van.haaren@intel.com>
---
lib/librte_eal/common/include/rte_service.h | 14 ++++++++------
lib/librte_eal/common/rte_service.c | 24 ++++++++++++------------
test/test/test_service_cores.c | 10 +++++-----
3 files changed, 25 insertions(+), 23 deletions(-)
diff --git a/lib/librte_eal/common/include/rte_service.h b/lib/librte_eal/common/include/rte_service.h
index e133ca1..afa6c0bf 100644
--- a/lib/librte_eal/common/include/rte_service.h
+++ b/lib/librte_eal/common/include/rte_service.h
@@ -318,13 +318,12 @@ int32_t rte_service_lcore_reset_all(void);
* Enable or disable statistics collection for *service*.
*
* This function enables per core, per-service cycle count collection.
- * @param service The service to enable statistics gathering on.
+ * @param id The service to enable statistics gathering on.
* @param enable Zero to disable statistics, non-zero to enable.
* @retval 0 Success
* @retval -EINVAL Invalid service pointer passed
*/
-int32_t rte_service_set_stats_enable(struct rte_service_spec *service,
- int32_t enable);
+int32_t rte_service_set_stats_enable(uint32_t id, int32_t enable);
/**
* @warning
@@ -351,10 +350,13 @@ int32_t rte_service_lcore_list(uint32_t array[], uint32_t n);
* @warning
* @b EXPERIMENTAL: this API may change without prior notice
*
- * Dumps any information available about the service. If service is NULL,
- * dumps info for all services.
+ * Dumps any information available about the service. When id is UINT32_MAX,
+ * this function dumps info for all services.
+ *
+ * @retval 0 Statistics have been successfully dumped
+ * @retval -EINVAL Invalid service id provided
*/
-int32_t rte_service_dump(FILE *f, struct rte_service_spec *service);
+int32_t rte_service_dump(FILE *f, uint32_t id);
#ifdef __cplusplus
}
diff --git a/lib/librte_eal/common/rte_service.c b/lib/librte_eal/common/rte_service.c
index 7e9bf4d..308ec6f 100644
--- a/lib/librte_eal/common/rte_service.c
+++ b/lib/librte_eal/common/rte_service.c
@@ -166,18 +166,15 @@ service_mt_safe(struct rte_service_spec_impl *s)
return s->spec.capabilities & RTE_SERVICE_CAP_MT_SAFE;
}
-int32_t rte_service_set_stats_enable(struct rte_service_spec *service,
- int32_t enabled)
+int32_t rte_service_set_stats_enable(uint32_t id, int32_t enabled)
{
- struct rte_service_spec_impl *impl =
- (struct rte_service_spec_impl *)service;
- if (!impl)
- return -EINVAL;
+ struct rte_service_spec_impl *s;
+ SERVICE_VALID_GET_OR_ERR_RET(id, s, 0);
if (enabled)
- impl->internal_flags |= SERVICE_F_STATS_ENABLED;
+ s->internal_flags |= SERVICE_F_STATS_ENABLED;
else
- impl->internal_flags &= ~(SERVICE_F_STATS_ENABLED);
+ s->internal_flags &= ~(SERVICE_F_STATS_ENABLED);
return 0;
}
@@ -669,9 +666,10 @@ service_dump_calls_per_lcore(FILE *f, uint32_t lcore, uint32_t reset)
fprintf(f, "\n");
}
-int32_t rte_service_dump(FILE *f, struct rte_service_spec *service)
+int32_t rte_service_dump(FILE *f, uint32_t id)
{
uint32_t i;
+ int print_one = (id != UINT32_MAX);
uint64_t total_cycles = 0;
for (i = 0; i < rte_service_count; i++) {
@@ -680,15 +678,17 @@ int32_t rte_service_dump(FILE *f, struct rte_service_spec *service)
total_cycles += rte_services[i].cycles_spent;
}
- if (service) {
- struct rte_service_spec_impl *s =
- (struct rte_service_spec_impl *)service;
+ /* print only the specified service */
+ if (print_one) {
+ struct rte_service_spec_impl *s;
+ SERVICE_VALID_GET_OR_ERR_RET(id, s, -EINVAL);
fprintf(f, "Service %s Summary\n", s->spec.name);
uint32_t reset = 0;
rte_service_dump_one(f, s, total_cycles, reset);
return 0;
}
+ /* print all services, as UINT32_MAX was passed as id */
fprintf(f, "Services Summary\n");
for (i = 0; i < rte_service_count; i++) {
uint32_t reset = 1;
diff --git a/test/test/test_service_cores.c b/test/test/test_service_cores.c
index ba252c5..4ece080 100644
--- a/test/test/test_service_cores.c
+++ b/test/test/test_service_cores.c
@@ -261,11 +261,11 @@ service_name(void)
static int
service_dump(void)
{
- struct rte_service_spec *service = rte_service_get_by_id(0);
- rte_service_set_stats_enable(service, 1);
- rte_service_dump(stdout, service);
- rte_service_set_stats_enable(service, 0);
- rte_service_dump(stdout, service);
+ const uint32_t sid = 0;
+ rte_service_set_stats_enable(sid, 1);
+ rte_service_dump(stdout, 0);
+ rte_service_set_stats_enable(sid, 0);
+ rte_service_dump(stdout, 0);
return unregister_all();
}
--
2.7.4
^ permalink raw reply [flat|nested] 32+ messages in thread
* [dpdk-dev] [PATCH 6/8] service: rework unregister api to use integers
2017-08-15 12:32 [dpdk-dev] [PATCH 0/8] service: rework for usability Harry van Haaren
` (4 preceding siblings ...)
2017-08-15 12:32 ` [dpdk-dev] [PATCH 5/8] service: rework service stats functions Harry van Haaren
@ 2017-08-15 12:32 ` Harry van Haaren
2017-08-15 12:32 ` [dpdk-dev] [PATCH 7/8] service: rework get by name function to use id Harry van Haaren
` (3 subsequent siblings)
9 siblings, 0 replies; 32+ messages in thread
From: Harry van Haaren @ 2017-08-15 12:32 UTC (permalink / raw)
To: dev; +Cc: Harry van Haaren
This commit reworks the unregister API to accept an integer.
Signed-off-by: Harry van Haaren <harry.van.haaren@intel.com>
---
.../common/include/rte_service_component.h | 2 +-
lib/librte_eal/common/rte_service.c | 25 ++++++----------------
test/test/test_service_cores.c | 10 +++------
3 files changed, 10 insertions(+), 27 deletions(-)
diff --git a/lib/librte_eal/common/include/rte_service_component.h b/lib/librte_eal/common/include/rte_service_component.h
index 5ba5cdf..70cca69 100644
--- a/lib/librte_eal/common/include/rte_service_component.h
+++ b/lib/librte_eal/common/include/rte_service_component.h
@@ -111,7 +111,7 @@ int32_t rte_service_register(const struct rte_service_spec *spec,
* @retval -EBUSY The service is currently running, stop the service before
* calling unregister. No action has been taken.
*/
-int32_t rte_service_unregister(struct rte_service_spec *service);
+int32_t rte_service_unregister(uint32_t id);
/**
* @warning
diff --git a/lib/librte_eal/common/rte_service.c b/lib/librte_eal/common/rte_service.c
index 308ec6f..7299514 100644
--- a/lib/librte_eal/common/rte_service.c
+++ b/lib/librte_eal/common/rte_service.c
@@ -259,35 +259,22 @@ rte_service_register(const struct rte_service_spec *spec, uint32_t *id_ptr)
}
int32_t
-rte_service_unregister(struct rte_service_spec *spec)
+rte_service_unregister(uint32_t id)
{
- struct rte_service_spec_impl *s = NULL;
- struct rte_service_spec_impl *spec_impl =
- (struct rte_service_spec_impl *)spec;
-
uint32_t i;
- uint32_t service_id;
- for (i = 0; i < RTE_SERVICE_NUM_MAX; i++) {
- if (&rte_services[i] == spec_impl) {
- s = spec_impl;
- service_id = i;
- break;
- }
- }
-
- if (!s)
- return -EINVAL;
+ struct rte_service_spec_impl *s;
+ SERVICE_VALID_GET_OR_ERR_RET(id, s, -EINVAL);
rte_service_count--;
rte_smp_wmb();
s->internal_flags &= ~(SERVICE_F_REGISTERED);
+ /* clear the run-bit in all cores */
for (i = 0; i < RTE_MAX_LCORE; i++)
- lcore_states[i].service_mask &= ~(UINT64_C(1) << service_id);
+ lcore_states[i].service_mask &= ~(UINT64_C(1) << id);
- memset(&rte_services[service_id], 0,
- sizeof(struct rte_service_spec_impl));
+ memset(&rte_services[id], 0, sizeof(struct rte_service_spec_impl));
return 0;
}
diff --git a/test/test/test_service_cores.c b/test/test/test_service_cores.c
index 4ece080..43ad027 100644
--- a/test/test/test_service_cores.c
+++ b/test/test/test_service_cores.c
@@ -131,17 +131,13 @@ static int
unregister_all(void)
{
uint32_t i;
- struct rte_service_spec *dead = (struct rte_service_spec *)0xdead;
- TEST_ASSERT_EQUAL(-EINVAL, rte_service_unregister(0),
- "Unregistered NULL pointer");
- TEST_ASSERT_EQUAL(-EINVAL, rte_service_unregister(dead),
- "Unregistered invalid pointer");
+ TEST_ASSERT_EQUAL(-EINVAL, rte_service_unregister(1000),
+ "Unregistered invalid service id");
uint32_t c = rte_service_get_count();
for (i = 0; i < c; i++) {
- struct rte_service_spec *s = rte_service_get_by_id(i);
- TEST_ASSERT_EQUAL(0, rte_service_unregister(s),
+ TEST_ASSERT_EQUAL(0, rte_service_unregister(i),
"Error unregistering a valid service");
}
--
2.7.4
^ permalink raw reply [flat|nested] 32+ messages in thread
* [dpdk-dev] [PATCH 7/8] service: rework get by name function to use id
2017-08-15 12:32 [dpdk-dev] [PATCH 0/8] service: rework for usability Harry van Haaren
` (5 preceding siblings ...)
2017-08-15 12:32 ` [dpdk-dev] [PATCH 6/8] service: rework unregister api to use integers Harry van Haaren
@ 2017-08-15 12:32 ` Harry van Haaren
2017-08-15 12:32 ` [dpdk-dev] [PATCH 8/8] service: clarify documentation for register Harry van Haaren
` (2 subsequent siblings)
9 siblings, 0 replies; 32+ messages in thread
From: Harry van Haaren @ 2017-08-15 12:32 UTC (permalink / raw)
To: dev; +Cc: Harry van Haaren
This commit reworks the service_get_by_name() function to
accept an integer, and removes the service_get_by_id() function.
All functions now accept an integer argument representing the
service, so it is no longer required to expose the service_spec
pointers to the application.
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 | 45 ++++++++++---------------
lib/librte_eal/common/rte_service.c | 24 ++++---------
lib/librte_eal/linuxapp/eal/rte_eal_version.map | 1 +
test/test/test_service_cores.c | 30 ++++++++++-------
5 files changed, 43 insertions(+), 58 deletions(-)
diff --git a/lib/librte_eal/bsdapp/eal/rte_eal_version.map b/lib/librte_eal/bsdapp/eal/rte_eal_version.map
index f76d1ea..97113e2 100644
--- a/lib/librte_eal/bsdapp/eal/rte_eal_version.map
+++ b/lib/librte_eal/bsdapp/eal/rte_eal_version.map
@@ -216,6 +216,7 @@ EXPERIMENTAL {
rte_service_get_by_id;
rte_service_get_by_name;
rte_service_get_count;
+ rte_service_get_name;
rte_service_lcore_add;
rte_service_lcore_count;
rte_service_lcore_del;
diff --git a/lib/librte_eal/common/include/rte_service.h b/lib/librte_eal/common/include/rte_service.h
index afa6c0bf..e06b075 100644
--- a/lib/librte_eal/common/include/rte_service.h
+++ b/lib/librte_eal/common/include/rte_service.h
@@ -61,9 +61,6 @@ extern "C" {
#include <rte_lcore.h>
-/* forward declaration only. Definition in rte_service_private.h */
-struct rte_service_spec;
-
#define RTE_SERVICE_NAME_MAX 32
/* Capabilities of a service.
@@ -89,40 +86,32 @@ struct rte_service_spec;
*/
uint32_t rte_service_get_count(void);
-
/**
* @warning
* @b EXPERIMENTAL: this API may change without prior notice
*
- * Return the specification of a service by integer id.
- *
- * This function provides the specification of a service. This can be used by
- * the application to understand what the service represents. The service
- * must not be modified by the application directly, only passed to the various
- * rte_service_* functions.
- *
- * @param id The integer id of the service to retrieve
- * @retval non-zero A valid pointer to the service_spec
- * @retval NULL Invalid *id* provided.
- */
-struct rte_service_spec *rte_service_get_by_id(uint32_t id);
-
-/**
- * @warning
- * @b EXPERIMENTAL: this API may change without prior notice
+ * Return the id of a service by name.
*
- * Return the specification of a service by name.
+ * This function provides the id of the service using the service name as
+ * lookup key. The service id is to be passed to other functions in the
+ * rte_service_* API.
*
- * This function provides the specification of a service using the service name
- * as lookup key. This can be used by the application to understand what the
- * service represents. The service must not be modified by the application
- * directly, only passed to the various rte_service_* functions.
+ * Example usage:
+ * @code
+ * uint32_t service_id;
+ * int32_t ret = rte_service_get_by_name("service_X", &service_id);
+ * if (ret) {
+ * // handle error
+ * }
+ * @endcode
*
* @param name The name of the service to retrieve
- * @retval non-zero A valid pointer to the service_spec
- * @retval NULL Invalid *name* provided.
+ * @param[out] service_id A pointer to a uint32_t, to be filled in with the id.
+ * @retval 0 Success. The service id is provided in *service_id*.
+ * @retval -EINVAL Null *service_id* pointer provided
+ * @retval -ENODEV No such service registered
*/
-struct rte_service_spec *rte_service_get_by_name(const char *name);
+int32_t rte_service_get_by_name(const char *name, uint32_t *service_id);
/**
* @warning
diff --git a/lib/librte_eal/common/rte_service.c b/lib/librte_eal/common/rte_service.c
index 7299514..7a825b7 100644
--- a/lib/librte_eal/common/rte_service.c
+++ b/lib/librte_eal/common/rte_service.c
@@ -185,29 +185,21 @@ rte_service_get_count(void)
return rte_service_count;
}
-struct rte_service_spec *
-rte_service_get_by_id(uint32_t id)
+int32_t rte_service_get_by_name(const char *name, uint32_t *service_id)
{
- struct rte_service_spec *service = NULL;
- if (id < rte_service_count)
- service = (struct rte_service_spec *)&rte_services[id];
-
- return service;
-}
+ if (!service_id)
+ return -EINVAL;
-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;
+ *service_id = i;
+ return 0;
}
}
- return service;
+ return -ENODEV;
}
const char *
@@ -406,10 +398,6 @@ rte_service_start_with_defaults(void)
rte_service_lcore_start(ids[i]);
for (i = 0; i < count; i++) {
- struct rte_service_spec *s = rte_service_get_by_id(i);
- if (!s)
- return -EINVAL;
-
/* do 1:1 core mapping here, with each service getting
* assigned a single core by default. Adding multiple services
* should multiplex to a single core, or 1:1 if there are the
diff --git a/lib/librte_eal/linuxapp/eal/rte_eal_version.map b/lib/librte_eal/linuxapp/eal/rte_eal_version.map
index 557b5e7..d179fa5 100644
--- a/lib/librte_eal/linuxapp/eal/rte_eal_version.map
+++ b/lib/librte_eal/linuxapp/eal/rte_eal_version.map
@@ -221,6 +221,7 @@ EXPERIMENTAL {
rte_service_get_by_id;
rte_service_get_by_name;
rte_service_get_count;
+ rte_service_get_name;
rte_service_lcore_add;
rte_service_lcore_count;
rte_service_lcore_del;
diff --git a/test/test/test_service_cores.c b/test/test/test_service_cores.c
index 43ad027..ff8dcb1 100644
--- a/test/test/test_service_cores.c
+++ b/test/test/test_service_cores.c
@@ -176,9 +176,13 @@ service_get_by_name(void)
{
unregister_all();
- /* ensure with no services registered returns NULL */
- TEST_ASSERT_EQUAL(0, rte_service_get_by_name(DUMMY_SERVICE_NAME),
- "Service get by name should return NULL");
+ uint32_t sid;
+ TEST_ASSERT_EQUAL(-ENODEV,
+ rte_service_get_by_name(DUMMY_SERVICE_NAME, &sid),
+ "get by name with invalid name should return -ENODEV");
+ TEST_ASSERT_EQUAL(-EINVAL,
+ rte_service_get_by_name(DUMMY_SERVICE_NAME, 0x0),
+ "get by name with NULL ptr should return -ENODEV");
/* register service */
struct rte_service_spec service;
@@ -192,16 +196,19 @@ service_get_by_name(void)
TEST_ASSERT_EQUAL(0, rte_service_register(&service, NULL),
"Failed to register valid service");
- /* ensure with dummy services registered returns same ptr as ID */
- struct rte_service_spec *s_by_id = rte_service_get_by_id(0);
- TEST_ASSERT_EQUAL(s_by_id, rte_service_get_by_name(DUMMY_SERVICE_NAME),
- "Service get_by_name should equal get_by_id()");
+ /* we unregistered all service, now registering 1, should be id 0 */
+ uint32_t service_id_as_expected = 0;
+ TEST_ASSERT_EQUAL(0, rte_service_get_by_name(DUMMY_SERVICE_NAME, &sid),
+ "Service get_by_name should return 0 on valid inputs");
+ TEST_ASSERT_EQUAL(service_id_as_expected, sid,
+ "Service get_by_name should equal expected id");
unregister_all();
/* ensure after unregister, get_by_name returns NULL */
- TEST_ASSERT_EQUAL(0, rte_service_get_by_name(DUMMY_SERVICE_NAME),
- "get by name should return NULL after unregister");
+ TEST_ASSERT_EQUAL(-ENODEV,
+ rte_service_get_by_name(DUMMY_SERVICE_NAME, &sid),
+ "get by name should return -ENODEV after unregister");
return TEST_SUCCESS;
}
@@ -245,9 +252,8 @@ service_probe_capability(void)
static int
service_name(void)
{
- struct rte_service_spec *service = rte_service_get_by_id(0);
-
- int equal = strcmp(service->name, DUMMY_SERVICE_NAME);
+ const char *name = rte_service_get_name(0);
+ int equal = strcmp(name, DUMMY_SERVICE_NAME);
TEST_ASSERT_EQUAL(0, equal, "Error: Service name not correct");
return unregister_all();
--
2.7.4
^ permalink raw reply [flat|nested] 32+ messages in thread
* [dpdk-dev] [PATCH 8/8] service: clarify documentation for register
2017-08-15 12:32 [dpdk-dev] [PATCH 0/8] service: rework for usability Harry van Haaren
` (6 preceding siblings ...)
2017-08-15 12:32 ` [dpdk-dev] [PATCH 7/8] service: rework get by name function to use id Harry van Haaren
@ 2017-08-15 12:32 ` Harry van Haaren
2017-08-16 11:16 ` [dpdk-dev] [PATCH 0/8] service: rework for usability Neil Horman
2017-08-21 12:58 ` [dpdk-dev] [PATCH v2 00/15] service: API improvements and updates Harry van Haaren
9 siblings, 0 replies; 32+ messages in thread
From: Harry van Haaren @ 2017-08-15 12:32 UTC (permalink / raw)
To: dev; +Cc: Harry van Haaren
This commit adds a section to the service register function
to make it clear that when registering a service, it must not
call any other service core APIs (eg: adding lcores or mappings).
The reason for this is that it breaks the abstraction that the
services represent a task or work-item to be performed, while
service-cores and assinging of tasks to cores is perfomed by the
application (or default settings from EAL are used).
Signed-off-by: Harry van Haaren <harry.van.haaren@intel.com>
---
lib/librte_eal/common/include/rte_service_component.h | 5 +++++
1 file changed, 5 insertions(+)
diff --git a/lib/librte_eal/common/include/rte_service_component.h b/lib/librte_eal/common/include/rte_service_component.h
index 70cca69..977a668 100644
--- a/lib/librte_eal/common/include/rte_service_component.h
+++ b/lib/librte_eal/common/include/rte_service_component.h
@@ -83,6 +83,11 @@ struct rte_service_spec {
* A service represents a component that the requires CPU time periodically to
* achieve its purpose.
*
+ * Note that when registering a service it is illegal to call any other
+ * public service core API, as this would break the abstraction that services
+ * present work to be performed, and EAL or the application schedule that work
+ * on service cores.
+ *
* For example the eventdev SW PMD requires CPU cycles to perform its
* scheduling. This can be achieved by registering it as a service, and the
* application can then assign CPU resources to it using
--
2.7.4
^ permalink raw reply [flat|nested] 32+ messages in thread
* Re: [dpdk-dev] [PATCH 0/8] service: rework for usability
2017-08-15 12:32 [dpdk-dev] [PATCH 0/8] service: rework for usability Harry van Haaren
` (7 preceding siblings ...)
2017-08-15 12:32 ` [dpdk-dev] [PATCH 8/8] service: clarify documentation for register Harry van Haaren
@ 2017-08-16 11:16 ` Neil Horman
2017-08-16 11:31 ` Van Haaren, Harry
2017-08-21 12:58 ` [dpdk-dev] [PATCH v2 00/15] service: API improvements and updates Harry van Haaren
9 siblings, 1 reply; 32+ messages in thread
From: Neil Horman @ 2017-08-16 11:16 UTC (permalink / raw)
To: Harry van Haaren; +Cc: dev
On Tue, Aug 15, 2017 at 01:32:32PM +0100, Harry van Haaren wrote:
> This patchset reworks the service apis to be more user
> friendly. In particular, the various rte_service_* functions
> now take an integer id parameter instead of a service pointer.
> This both reduces the API surface (no service_get_from_id()),
> and allows easier debugging (gdb function calls with integer args),
> and various other benefits (better encapsulation, less pointers :)
>
> Finally, some APIs are changed or renamed for consistency and
> clarity of what they do. See commit messages for details.
> Note that the service library is merged as EXPERIMENTAL in
> the 17.08 release, allowing API improvements for 17.11 release.
>
> I hope to merge this patchset early in the 17.11 timeframe,
> so please review ASAP to allow time for other DPDK components
> to utilize services in this release :)
>
> Feedback and input welcome, -Harry
>
You need to add a deprecation note in the rel notes area so that people are
aware of the upcomming ABI changes
Neil
> ---
>
> There is one checkpatch warning: "macro with flow control", however
> this same type of macro is used extensively in Ethdev and others,
> I presume it is a false-positive.
>
> Harry van Haaren (8):
> service: rework probe and get name to use ids
> service: rework lcore to service map functions
> service: rework register to return service id
> service: rework service start stop to runstate
> service: rework service stats functions
> service: rework unregister api to use integers
> service: rework get by name function to use id
> service: clarify documentation for register
>
> drivers/event/sw/sw_evdev.c | 7 +-
> drivers/event/sw/sw_evdev.h | 1 +
> lib/librte_eal/bsdapp/eal/rte_eal_version.map | 11 +-
> lib/librte_eal/common/include/rte_service.h | 144 +++++++-----------
> .../common/include/rte_service_component.h | 13 +-
> lib/librte_eal/common/rte_service.c | 167 +++++++++------------
> lib/librte_eal/linuxapp/eal/rte_eal_version.map | 11 +-
> test/test/test_service_cores.c | 123 +++++++--------
> 8 files changed, 215 insertions(+), 262 deletions(-)
>
> --
> 2.7.4
>
>
^ permalink raw reply [flat|nested] 32+ messages in thread
* Re: [dpdk-dev] [PATCH 0/8] service: rework for usability
2017-08-16 11:16 ` [dpdk-dev] [PATCH 0/8] service: rework for usability Neil Horman
@ 2017-08-16 11:31 ` Van Haaren, Harry
2017-08-16 12:07 ` Van Haaren, Harry
0 siblings, 1 reply; 32+ messages in thread
From: Van Haaren, Harry @ 2017-08-16 11:31 UTC (permalink / raw)
To: Neil Horman; +Cc: dev
> From: Neil Horman [mailto:nhorman@tuxdriver.com]
> Sent: Wednesday, August 16, 2017 12:16 PM
> To: Van Haaren, Harry <harry.van.haaren@intel.com>
> Cc: dev@dpdk.org
> Subject: Re: [dpdk-dev] [PATCH 0/8] service: rework for usability
>
> On Tue, Aug 15, 2017 at 01:32:32PM +0100, Harry van Haaren wrote:
> > This patchset reworks the service apis to be more user
> > friendly. In particular, the various rte_service_* functions
> > now take an integer id parameter instead of a service pointer.
> > This both reduces the API surface (no service_get_from_id()),
> > and allows easier debugging (gdb function calls with integer args),
> > and various other benefits (better encapsulation, less pointers :)
> >
> > Finally, some APIs are changed or renamed for consistency and
> > clarity of what they do. See commit messages for details.
> > Note that the service library is merged as EXPERIMENTAL in
> > the 17.08 release, allowing API improvements for 17.11 release.
> >
> > I hope to merge this patchset early in the 17.11 timeframe,
> > so please review ASAP to allow time for other DPDK components
> > to utilize services in this release :)
> >
> > Feedback and input welcome, -Harry
> >
> You need to add a deprecation note in the rel notes area so that people are
> aware of the upcomming ABI changes
Service cores was merged into 17.08 with the EXPERIMENTAL tag, which indicates that the API and ABI are not stable. The version map file has the service cores functions added in the Experimental staging area, instead of in the 17.08 stable ABI[1]. To make this very visible to the users, the documentation has large "Warning: Experimental, this API may change without prior notice" marks[2], and the MAINTAINERS file[3] has the Experimental tag.
As far as I am aware, those are all the requirements to be able to remove / change / update / fix APIs. It was discussed on #IRC that it would be better to merge service-cores as experimental to allow faster iteration, and to get improvements out the door, and I'm still of that opinion.
Given the above, I don't see any issue with merging service-core changes into the 17.11 release.
[1] http://dpdk.org/browse/dpdk/tree/lib/librte_eal/linuxapp/eal/rte_eal_version.map#n212
[2] http://dpdk.org/doc/api/rte__service_8h.html#aea7fce2a101bf2c00194dffb30bfc4ea
[3] http://dpdk.org/browse/dpdk/tree/MAINTAINERS#n138
^ permalink raw reply [flat|nested] 32+ messages in thread
* Re: [dpdk-dev] [PATCH 0/8] service: rework for usability
2017-08-16 11:31 ` Van Haaren, Harry
@ 2017-08-16 12:07 ` Van Haaren, Harry
2017-08-16 13:27 ` Neil Horman
0 siblings, 1 reply; 32+ messages in thread
From: Van Haaren, Harry @ 2017-08-16 12:07 UTC (permalink / raw)
To: Van Haaren, Harry, Neil Horman; +Cc: dev
> From: dev [mailto:dev-bounces@dpdk.org] On Behalf Of Van Haaren, Harry
> Sent: Wednesday, August 16, 2017 12:32 PM
> To: Neil Horman <nhorman@tuxdriver.com>
> Cc: dev@dpdk.org
> Subject: Re: [dpdk-dev] [PATCH 0/8] service: rework for usability
>
> > From: Neil Horman [mailto:nhorman@tuxdriver.com]
> > Sent: Wednesday, August 16, 2017 12:16 PM
> > To: Van Haaren, Harry <harry.van.haaren@intel.com>
> > Cc: dev@dpdk.org
> > Subject: Re: [dpdk-dev] [PATCH 0/8] service: rework for usability
> >
> > On Tue, Aug 15, 2017 at 01:32:32PM +0100, Harry van Haaren wrote:
> > > This patchset reworks the service apis to be more user
> > > friendly. In particular, the various rte_service_* functions
> > > now take an integer id parameter instead of a service pointer.
> > > This both reduces the API surface (no service_get_from_id()),
> > > and allows easier debugging (gdb function calls with integer args),
> > > and various other benefits (better encapsulation, less pointers :)
> > >
> > > Finally, some APIs are changed or renamed for consistency and
> > > clarity of what they do. See commit messages for details.
> > > Note that the service library is merged as EXPERIMENTAL in
> > > the 17.08 release, allowing API improvements for 17.11 release.
> > >
> > > I hope to merge this patchset early in the 17.11 timeframe,
> > > so please review ASAP to allow time for other DPDK components
> > > to utilize services in this release :)
> > >
> > > Feedback and input welcome, -Harry
> > >
> > You need to add a deprecation note in the rel notes area so that people are
> > aware of the upcomming ABI changes
>
> Service cores was merged into 17.08 with the EXPERIMENTAL tag, which indicates that the
> API and ABI are not stable. The version map file has the service cores functions added in
> the Experimental staging area, instead of in the 17.08 stable ABI[1]. To make this very
> visible to the users, the documentation has large "Warning: Experimental, this API may
> change without prior notice" marks[2], and the MAINTAINERS file[3] has the Experimental
> tag.
>
> As far as I am aware, those are all the requirements to be able to remove / change /
> update / fix APIs. It was discussed on #IRC that it would be better to merge service-cores
> as experimental to allow faster iteration, and to get improvements out the door, and I'm
> still of that opinion.
>
> Given the above, I don't see any issue with merging service-core changes into the 17.11
> release.
>
> [1] http://dpdk.org/browse/dpdk/tree/lib/librte_eal/linuxapp/eal/rte_eal_version.map#n212
> [2] http://dpdk.org/doc/api/rte__service_8h.html#aea7fce2a101bf2c00194dffb30bfc4ea
> [3] http://dpdk.org/browse/dpdk/tree/MAINTAINERS#n138
Self-reply;
On re-reading, perhaps I understood you wrong; did you mean that I need to add a section to the Release notes that the service core APIs have been updated in 17.11 itself?
That's a very valid point - and I'll fix that in v2, (some other typo fixes to fix too).
-Harry
^ permalink raw reply [flat|nested] 32+ messages in thread
* Re: [dpdk-dev] [PATCH 0/8] service: rework for usability
2017-08-16 12:07 ` Van Haaren, Harry
@ 2017-08-16 13:27 ` Neil Horman
0 siblings, 0 replies; 32+ messages in thread
From: Neil Horman @ 2017-08-16 13:27 UTC (permalink / raw)
To: Van Haaren, Harry; +Cc: dev
On Wed, Aug 16, 2017 at 12:07:11PM +0000, Van Haaren, Harry wrote:
> > From: dev [mailto:dev-bounces@dpdk.org] On Behalf Of Van Haaren, Harry
> > Sent: Wednesday, August 16, 2017 12:32 PM
> > To: Neil Horman <nhorman@tuxdriver.com>
> > Cc: dev@dpdk.org
> > Subject: Re: [dpdk-dev] [PATCH 0/8] service: rework for usability
> >
> > > From: Neil Horman [mailto:nhorman@tuxdriver.com]
> > > Sent: Wednesday, August 16, 2017 12:16 PM
> > > To: Van Haaren, Harry <harry.van.haaren@intel.com>
> > > Cc: dev@dpdk.org
> > > Subject: Re: [dpdk-dev] [PATCH 0/8] service: rework for usability
> > >
> > > On Tue, Aug 15, 2017 at 01:32:32PM +0100, Harry van Haaren wrote:
> > > > This patchset reworks the service apis to be more user
> > > > friendly. In particular, the various rte_service_* functions
> > > > now take an integer id parameter instead of a service pointer.
> > > > This both reduces the API surface (no service_get_from_id()),
> > > > and allows easier debugging (gdb function calls with integer args),
> > > > and various other benefits (better encapsulation, less pointers :)
> > > >
> > > > Finally, some APIs are changed or renamed for consistency and
> > > > clarity of what they do. See commit messages for details.
> > > > Note that the service library is merged as EXPERIMENTAL in
> > > > the 17.08 release, allowing API improvements for 17.11 release.
> > > >
> > > > I hope to merge this patchset early in the 17.11 timeframe,
> > > > so please review ASAP to allow time for other DPDK components
> > > > to utilize services in this release :)
> > > >
> > > > Feedback and input welcome, -Harry
> > > >
> > > You need to add a deprecation note in the rel notes area so that people are
> > > aware of the upcomming ABI changes
> >
> > Service cores was merged into 17.08 with the EXPERIMENTAL tag, which indicates that the
> > API and ABI are not stable. The version map file has the service cores functions added in
> > the Experimental staging area, instead of in the 17.08 stable ABI[1]. To make this very
> > visible to the users, the documentation has large "Warning: Experimental, this API may
> > change without prior notice" marks[2], and the MAINTAINERS file[3] has the Experimental
> > tag.
> >
> > As far as I am aware, those are all the requirements to be able to remove / change /
> > update / fix APIs. It was discussed on #IRC that it would be better to merge service-cores
> > as experimental to allow faster iteration, and to get improvements out the door, and I'm
> > still of that opinion.
> >
> > Given the above, I don't see any issue with merging service-core changes into the 17.11
> > release.
> >
> > [1] http://dpdk.org/browse/dpdk/tree/lib/librte_eal/linuxapp/eal/rte_eal_version.map#n212
> > [2] http://dpdk.org/doc/api/rte__service_8h.html#aea7fce2a101bf2c00194dffb30bfc4ea
> > [3] http://dpdk.org/browse/dpdk/tree/MAINTAINERS#n138
>
> Self-reply;
>
> On re-reading, perhaps I understood you wrong; did you mean that I need to add a section to the Release notes that the service core APIs have been updated in 17.11 itself?
>
> That's a very valid point - and I'll fix that in v2, (some other typo fixes to fix too).
>
I hadn't noted the experimental tag, so you likely don't need a deprecation
warning, but yes, a release note to indicate the change to the API in 17.11
would be good.
Best
NEil
> -Harry
>
^ permalink raw reply [flat|nested] 32+ messages in thread
* [dpdk-dev] [PATCH v2 00/15] service: API improvements and updates
2017-08-15 12:32 [dpdk-dev] [PATCH 0/8] service: rework for usability Harry van Haaren
` (8 preceding siblings ...)
2017-08-16 11:16 ` [dpdk-dev] [PATCH 0/8] service: rework for usability Neil Horman
@ 2017-08-21 12:58 ` Harry van Haaren
2017-08-21 12:58 ` [dpdk-dev] [PATCH v2 01/15] service: rework probe and get name to use ids Harry van Haaren
` (15 more replies)
9 siblings, 16 replies; 32+ messages in thread
From: Harry van Haaren @ 2017-08-21 12:58 UTC (permalink / raw)
To: dev; +Cc: Harry van Haaren
This patchset reworks the service apis to be more user
friendly. In particular, the various rte_service_* functions
now take an integer id parameter instead of a service pointer.
This both reduces the API surface (no service_get_from_id()),
and allows easier debugging (gdb function calls with integer args),
and various other benefits (better encapsulation, less pointers :)
Finally, some APIs are changed or renamed for consistency and
clarity of what they do. See commit messages for details.
Note that the service library is merged as EXPERIMENTAL in
the 17.08 release, allowing API improvements for 17.11 release.
I hope to merge this patchset early in the 17.11 timeframe,
so please review ASAP to allow time for other DPDK components
to utilize services in this release :)
Feedback and input welcome, -Harry
v2:
- Rename API functions from service_register to service_component_register
to clearly show which of the APIs apply to services and which apply to the
service components.
- There are 4 "fixes" patches, each one targetting a specific issue that was
observered or discovered since the 17.08 release. See commit messages for
details about the issue, and the resolution.
- Added "component runstate" concept, to allow components indicate if they are
ready to have their service callback invoked.
- Improve documentation, and update doxygen comments to make the usage of the
service component APIs clearer.
Harry van Haaren (15):
service: rework probe and get name to use ids
service: rework lcore to service map functions
service: rework register to return service id
service: rework service start stop to runstate
service: rework service stats functions
service: rework unregister api to use integers
service: rework get by name function to use id
service: fix and refactor atomic service accesses
service: fix loops to always scan all services
service: fix return values of functions to 0 or 1
service: fix lcore in wait state in lcore add
service: reset core call stats on dump
service: add component runstate
service: clarify documentation for register
docs: add notes on service cores API updates
doc/guides/rel_notes/release_17_11.rst | 20 ++
drivers/event/sw/sw_evdev.c | 7 +-
drivers/event/sw/sw_evdev.h | 1 +
lib/librte_eal/bsdapp/eal/rte_eal_version.map | 16 +-
lib/librte_eal/common/include/rte_service.h | 144 +++++-------
.../common/include/rte_service_component.h | 36 ++-
lib/librte_eal/common/rte_service.c | 256 +++++++++++----------
lib/librte_eal/linuxapp/eal/rte_eal_version.map | 16 +-
test/test/test_service_cores.c | 155 +++++++------
9 files changed, 349 insertions(+), 302 deletions(-)
--
2.7.4
^ permalink raw reply [flat|nested] 32+ messages in thread
* [dpdk-dev] [PATCH v2 01/15] service: rework probe and get name to use ids
2017-08-21 12:58 ` [dpdk-dev] [PATCH v2 00/15] service: API improvements and updates Harry van Haaren
@ 2017-08-21 12:58 ` Harry van Haaren
2017-08-30 19:25 ` Pavan Nikhilesh Bhagavatula
2017-09-04 14:32 ` Pavan Nikhilesh Bhagavatula
2017-08-21 12:58 ` [dpdk-dev] [PATCH v2 02/15] service: rework lcore to service map functions Harry van Haaren
` (14 subsequent siblings)
15 siblings, 2 replies; 32+ messages in thread
From: Harry van Haaren @ 2017-08-21 12:58 UTC (permalink / raw)
To: dev; +Cc: Harry van Haaren
This commit adds a macro to easily validate a service ID, and then
lookup the service pointer, or return a user-specified error code.
This marco will be heavily used in the following patches as it will
be ID based instead of pointer-based.
The probe_capability function is reworked to use an integer ID instead
of a pointer. Rework the service_get_name() function is updated to use
IDs. Unit tests are updated to keep things compiling after each commit.
Signed-off-by: Harry van Haaren <harry.van.haaren@intel.com>
---
lib/librte_eal/common/include/rte_service.h | 5 ++---
lib/librte_eal/common/rte_service.c | 20 +++++++++++++++-----
test/test/test_service_cores.c | 7 +++----
3 files changed, 20 insertions(+), 12 deletions(-)
diff --git a/lib/librte_eal/common/include/rte_service.h b/lib/librte_eal/common/include/rte_service.h
index 7c6f738..bed1a61 100644
--- a/lib/librte_eal/common/include/rte_service.h
+++ b/lib/librte_eal/common/include/rte_service.h
@@ -133,7 +133,7 @@ struct rte_service_spec *rte_service_get_by_name(const char *name);
* @return A pointer to the name of the service. The returned pointer remains
* in ownership of the service, and the application must not free it.
*/
-const char *rte_service_get_name(const struct rte_service_spec *service);
+const char *rte_service_get_name(uint32_t id);
/**
* @warning
@@ -146,8 +146,7 @@ const char *rte_service_get_name(const struct rte_service_spec *service);
* @retval 1 Capability supported by this service instance
* @retval 0 Capability not supported by this service instance
*/
-int32_t rte_service_probe_capability(const struct rte_service_spec *service,
- uint32_t capability);
+int32_t rte_service_probe_capability(uint32_t id, uint32_t capability);
/**
* @warning
diff --git a/lib/librte_eal/common/rte_service.c b/lib/librte_eal/common/rte_service.c
index 7efb76d..c969406 100644
--- a/lib/librte_eal/common/rte_service.c
+++ b/lib/librte_eal/common/rte_service.c
@@ -144,6 +144,13 @@ service_valid(uint32_t id)
return !!(rte_services[id].internal_flags & SERVICE_F_REGISTERED);
}
+/* validate ID and retrieve service pointer, or return error value */
+#define SERVICE_VALID_GET_OR_ERR_RET(id, service, retval) do { \
+ if (id >= RTE_SERVICE_NUM_MAX || !service_valid(id)) \
+ return retval; \
+ service = &rte_services[id]; \
+} while (0)
+
/* returns 1 if statistics should be colleced for service
* Returns 0 if statistics should not be collected for service
*/
@@ -207,16 +214,19 @@ struct rte_service_spec *rte_service_get_by_name(const char *name)
}
const char *
-rte_service_get_name(const struct rte_service_spec *service)
+rte_service_get_name(uint32_t id)
{
- return service->name;
+ struct rte_service_spec_impl *s;
+ SERVICE_VALID_GET_OR_ERR_RET(id, s, 0);
+ return s->spec.name;
}
int32_t
-rte_service_probe_capability(const struct rte_service_spec *service,
- uint32_t capability)
+rte_service_probe_capability(uint32_t id, uint32_t capability)
{
- return service->capabilities & capability;
+ struct rte_service_spec_impl *s;
+ SERVICE_VALID_GET_OR_ERR_RET(id, s, -EINVAL);
+ return s->spec.capabilities & capability;
}
int32_t
diff --git a/test/test/test_service_cores.c b/test/test/test_service_cores.c
index 88fac8f..940bc62 100644
--- a/test/test/test_service_cores.c
+++ b/test/test/test_service_cores.c
@@ -225,8 +225,8 @@ service_probe_capability(void)
"Register of MT SAFE service failed");
/* verify flag is enabled */
- struct rte_service_spec *s = rte_service_get_by_id(0);
- int32_t mt = rte_service_probe_capability(s, RTE_SERVICE_CAP_MT_SAFE);
+ const uint32_t sid = 0;
+ int32_t mt = rte_service_probe_capability(sid, RTE_SERVICE_CAP_MT_SAFE);
TEST_ASSERT_EQUAL(1, mt, "MT SAFE capability flag not set.");
@@ -239,8 +239,7 @@ service_probe_capability(void)
"Register of non-MT safe service failed");
/* verify flag is enabled */
- s = rte_service_get_by_id(0);
- mt = rte_service_probe_capability(s, RTE_SERVICE_CAP_MT_SAFE);
+ mt = rte_service_probe_capability(sid, RTE_SERVICE_CAP_MT_SAFE);
TEST_ASSERT_EQUAL(0, mt, "MT SAFE cap flag set on non MT SAFE service");
return unregister_all();
--
2.7.4
^ permalink raw reply [flat|nested] 32+ messages in thread
* [dpdk-dev] [PATCH v2 02/15] service: rework lcore to service map functions
2017-08-21 12:58 ` [dpdk-dev] [PATCH v2 00/15] service: API improvements and updates Harry van Haaren
2017-08-21 12:58 ` [dpdk-dev] [PATCH v2 01/15] service: rework probe and get name to use ids Harry van Haaren
@ 2017-08-21 12:58 ` Harry van Haaren
2017-08-21 12:58 ` [dpdk-dev] [PATCH v2 03/15] service: rework register to return service id Harry van Haaren
` (13 subsequent siblings)
15 siblings, 0 replies; 32+ messages in thread
From: Harry van Haaren @ 2017-08-21 12:58 UTC (permalink / raw)
To: dev; +Cc: Harry van Haaren
This commit updates the APIs exposed to map service cores and
services. The previous APIs required a pointer to a service,
and used two separate functions for enable and disable. The
new API uses an integer ID for the service and has a parameter
for map or unmap. Unit tests are updated and passing, and the
map file is updated to the new function names.
Signed-off-by: Harry van Haaren <harry.van.haaren@intel.com>
---
lib/librte_eal/bsdapp/eal/rte_eal_version.map | 5 ++-
lib/librte_eal/common/include/rte_service.h | 43 +++++++++----------------
lib/librte_eal/common/rte_service.c | 31 ++++++++----------
lib/librte_eal/linuxapp/eal/rte_eal_version.map | 5 ++-
test/test/test_service_cores.c | 27 +++++++++-------
5 files changed, 48 insertions(+), 63 deletions(-)
diff --git a/lib/librte_eal/bsdapp/eal/rte_eal_version.map b/lib/librte_eal/bsdapp/eal/rte_eal_version.map
index aac6fd7..f626a6f 100644
--- a/lib/librte_eal/bsdapp/eal/rte_eal_version.map
+++ b/lib/librte_eal/bsdapp/eal/rte_eal_version.map
@@ -212,13 +212,10 @@ EXPERIMENTAL {
rte_eal_devargs_remove;
rte_eal_hotplug_add;
rte_eal_hotplug_remove;
- rte_service_disable_on_lcore;
rte_service_dump;
- rte_service_enable_on_lcore;
rte_service_get_by_id;
rte_service_get_by_name;
rte_service_get_count;
- rte_service_get_enabled_on_lcore;
rte_service_is_running;
rte_service_lcore_add;
rte_service_lcore_count;
@@ -227,6 +224,8 @@ EXPERIMENTAL {
rte_service_lcore_reset_all;
rte_service_lcore_start;
rte_service_lcore_stop;
+ rte_service_map_lcore_get;
+ rte_service_map_lcore_set;
rte_service_probe_capability;
rte_service_register;
rte_service_reset;
diff --git a/lib/librte_eal/common/include/rte_service.h b/lib/librte_eal/common/include/rte_service.h
index bed1a61..de69695 100644
--- a/lib/librte_eal/common/include/rte_service.h
+++ b/lib/librte_eal/common/include/rte_service.h
@@ -152,10 +152,10 @@ int32_t rte_service_probe_capability(uint32_t id, uint32_t capability);
* @warning
* @b EXPERIMENTAL: this API may change without prior notice
*
- * Enable a core to run a service.
+ * Map or unmap a lcore to a service.
*
- * Each core can be added or removed from running specific services. This
- * functions adds *lcore* to the set of cores that will run *service*.
+ * Each core can be added or removed from running a specific service. This
+ * function enables or disables *lcore* to run *service_id*.
*
* If multiple cores are enabled on a service, an atomic is used to ensure that
* only one cores runs the service at a time. The exception to this is when
@@ -163,43 +163,30 @@ int32_t rte_service_probe_capability(uint32_t id, uint32_t capability);
* called RTE_SERVICE_CAP_MT_SAFE. With the multi-thread safe capability set,
* the service function can be run on multiple threads at the same time.
*
- * @retval 0 lcore added successfully
- * @retval -EINVAL An invalid service or lcore was provided.
- */
-int32_t rte_service_enable_on_lcore(struct rte_service_spec *service,
- uint32_t lcore);
-
-/**
- * @warning
- * @b EXPERIMENTAL: this API may change without prior notice
- *
- * Disable a core to run a service.
+ * @param service_id the service to apply the lcore to
+ * @param lcore The lcore that will be mapped to service
+ * @param enable Zero to unmap or disable the core, non-zero to enable
*
- * Each core can be added or removed from running specific services. This
- * functions removes *lcore* to the set of cores that will run *service*.
- *
- * @retval 0 Lcore removed successfully
+ * @retval 0 lcore map updated successfully
* @retval -EINVAL An invalid service or lcore was provided.
*/
-int32_t rte_service_disable_on_lcore(struct rte_service_spec *service,
- uint32_t lcore);
+int32_t rte_service_map_lcore_set(uint32_t service_id, uint32_t lcore,
+ uint32_t enable);
/**
* @warning
* @b EXPERIMENTAL: this API may change without prior notice
*
- * Return if an lcore is enabled for the service.
+ * Retrieve the mapping of an lcore to a service.
*
- * This function allows the application to query if *lcore* is currently set to
- * run *service*.
+ * @param service_id the service to apply the lcore to
+ * @param lcore The lcore that will be mapped to service
*
- * @retval 1 Lcore enabled on this lcore
- * @retval 0 Lcore disabled on this lcore
+ * @retval 1 lcore is mapped to service
+ * @retval 0 lcore is not mapped to service
* @retval -EINVAL An invalid service or lcore was provided.
*/
-int32_t rte_service_get_enabled_on_lcore(struct rte_service_spec *service,
- uint32_t lcore);
-
+int32_t rte_service_map_lcore_get(uint32_t service_id, uint32_t lcore);
/**
* @warning
diff --git a/lib/librte_eal/common/rte_service.c b/lib/librte_eal/common/rte_service.c
index c969406..c5a8d0d 100644
--- a/lib/librte_eal/common/rte_service.c
+++ b/lib/librte_eal/common/rte_service.c
@@ -436,7 +436,7 @@ rte_service_start_with_defaults(void)
* should multiplex to a single core, or 1:1 if there are the
* same amount of services as service-cores
*/
- ret = rte_service_enable_on_lcore(s, ids[lcore_iter]);
+ ret = rte_service_map_lcore_set(i, ids[lcore_iter], 1);
if (ret)
return -ENODEV;
@@ -492,28 +492,25 @@ service_update(struct rte_service_spec *service, uint32_t lcore,
return 0;
}
-int32_t rte_service_get_enabled_on_lcore(struct rte_service_spec *service,
- uint32_t lcore)
-{
- uint32_t enabled;
- int ret = service_update(service, lcore, 0, &enabled);
- if (ret == 0)
- return enabled;
- return -EINVAL;
-}
-
int32_t
-rte_service_enable_on_lcore(struct rte_service_spec *service, uint32_t lcore)
+rte_service_map_lcore_set(uint32_t id, uint32_t lcore, uint32_t enabled)
{
- uint32_t on = 1;
- return service_update(service, lcore, &on, 0);
+ struct rte_service_spec_impl *s;
+ SERVICE_VALID_GET_OR_ERR_RET(id, s, -EINVAL);
+ uint32_t on = enabled > 0;
+ return service_update(&s->spec, lcore, &on, 0);
}
int32_t
-rte_service_disable_on_lcore(struct rte_service_spec *service, uint32_t lcore)
+rte_service_map_lcore_get(uint32_t id, uint32_t lcore)
{
- uint32_t off = 0;
- return service_update(service, lcore, &off, 0);
+ struct rte_service_spec_impl *s;
+ SERVICE_VALID_GET_OR_ERR_RET(id, s, -EINVAL);
+ uint32_t enabled;
+ int ret = service_update(&s->spec, lcore, 0, &enabled);
+ if (ret == 0)
+ return enabled;
+ return ret;
}
int32_t rte_service_lcore_reset_all(void)
diff --git a/lib/librte_eal/linuxapp/eal/rte_eal_version.map b/lib/librte_eal/linuxapp/eal/rte_eal_version.map
index 3a8f154..452ae1c 100644
--- a/lib/librte_eal/linuxapp/eal/rte_eal_version.map
+++ b/lib/librte_eal/linuxapp/eal/rte_eal_version.map
@@ -217,13 +217,10 @@ EXPERIMENTAL {
rte_eal_devargs_remove;
rte_eal_hotplug_add;
rte_eal_hotplug_remove;
- rte_service_disable_on_lcore;
rte_service_dump;
- rte_service_enable_on_lcore;
rte_service_get_by_id;
rte_service_get_by_name;
rte_service_get_count;
- rte_service_get_enabled_on_lcore;
rte_service_is_running;
rte_service_lcore_add;
rte_service_lcore_count;
@@ -232,6 +229,8 @@ EXPERIMENTAL {
rte_service_lcore_reset_all;
rte_service_lcore_start;
rte_service_lcore_stop;
+ rte_service_map_lcore_get;
+ rte_service_map_lcore_set;
rte_service_probe_capability;
rte_service_register;
rte_service_reset;
diff --git a/test/test/test_service_cores.c b/test/test/test_service_cores.c
index 940bc62..fd63efd 100644
--- a/test/test/test_service_cores.c
+++ b/test/test/test_service_cores.c
@@ -273,12 +273,13 @@ service_dump(void)
static int
service_start_stop(void)
{
+ const uint32_t sid = 0;
struct rte_service_spec *service = rte_service_get_by_id(0);
/* is_running() returns if service is running and slcore is mapped */
TEST_ASSERT_EQUAL(0, rte_service_lcore_add(slcore_id),
"Service core add did not return zero");
- int ret = rte_service_enable_on_lcore(service, slcore_id);
+ int ret = rte_service_map_lcore_set(sid, slcore_id, 1);
TEST_ASSERT_EQUAL(0, ret,
"Enabling service core, expected 0 got %d", ret);
@@ -313,12 +314,12 @@ service_remote_launch_func(void *arg)
static int
service_lcore_en_dis_able(void)
{
- struct rte_service_spec *s = rte_service_get_by_id(0);
+ const uint32_t sid = 0;
/* expected failure cases */
- TEST_ASSERT_EQUAL(-EINVAL, rte_service_enable_on_lcore(s, 100000),
+ TEST_ASSERT_EQUAL(-EINVAL, rte_service_map_lcore_set(sid, 100000, 1),
"Enable on invalid core did not fail");
- TEST_ASSERT_EQUAL(-EINVAL, rte_service_disable_on_lcore(s, 100000),
+ TEST_ASSERT_EQUAL(-EINVAL, rte_service_map_lcore_set(sid, 100000, 0),
"Disable on invalid core did not fail");
/* add service core to allow enabling */
@@ -326,15 +327,15 @@ service_lcore_en_dis_able(void)
"Add service core failed when not in use before");
/* valid enable */
- TEST_ASSERT_EQUAL(0, rte_service_enable_on_lcore(s, slcore_id),
+ TEST_ASSERT_EQUAL(0, rte_service_map_lcore_set(sid, slcore_id, 1),
"Enabling valid service and core failed");
- TEST_ASSERT_EQUAL(1, rte_service_get_enabled_on_lcore(s, slcore_id),
+ TEST_ASSERT_EQUAL(1, rte_service_map_lcore_get(sid, slcore_id),
"Enabled core returned not-enabled");
/* valid disable */
- TEST_ASSERT_EQUAL(0, rte_service_disable_on_lcore(s, slcore_id),
+ TEST_ASSERT_EQUAL(0, rte_service_map_lcore_set(sid, slcore_id, 0),
"Disabling valid service and lcore failed");
- TEST_ASSERT_EQUAL(0, rte_service_get_enabled_on_lcore(s, slcore_id),
+ TEST_ASSERT_EQUAL(0, rte_service_map_lcore_get(sid, slcore_id),
"Disabled core returned enabled");
/* call remote_launch to verify that app can launch ex-service lcore */
@@ -474,11 +475,12 @@ service_threaded_test(int mt_safe)
"Register of MT SAFE service failed");
struct rte_service_spec *s = rte_service_get_by_id(0);
+ const uint32_t sid = 0;
TEST_ASSERT_EQUAL(0, rte_service_start(s),
"Starting valid service failed");
- TEST_ASSERT_EQUAL(0, rte_service_enable_on_lcore(s, slcore_1),
+ TEST_ASSERT_EQUAL(0, rte_service_map_lcore_set(sid, slcore_1, 1),
"Failed to enable lcore 1 on mt safe service");
- TEST_ASSERT_EQUAL(0, rte_service_enable_on_lcore(s, slcore_2),
+ TEST_ASSERT_EQUAL(0, rte_service_map_lcore_set(sid, slcore_2, 1),
"Failed to enable lcore 2 on mt safe service");
rte_service_lcore_start(slcore_1);
rte_service_lcore_start(slcore_2);
@@ -529,10 +531,11 @@ static int
service_lcore_start_stop(void)
{
/* start service core and service, create mapping so tick() runs */
+ const uint32_t sid = 0;
struct rte_service_spec *s = rte_service_get_by_id(0);
TEST_ASSERT_EQUAL(0, rte_service_start(s),
"Starting valid service failed");
- TEST_ASSERT_EQUAL(-EINVAL, rte_service_enable_on_lcore(s, slcore_id),
+ TEST_ASSERT_EQUAL(-EINVAL, rte_service_map_lcore_set(sid, slcore_id, 1),
"Enabling valid service on non-service core must fail");
/* core start */
@@ -540,7 +543,7 @@ service_lcore_start_stop(void)
"Service core start without add should return EINVAL");
TEST_ASSERT_EQUAL(0, rte_service_lcore_add(slcore_id),
"Service core add did not return zero");
- TEST_ASSERT_EQUAL(0, rte_service_enable_on_lcore(s, slcore_id),
+ TEST_ASSERT_EQUAL(0, rte_service_map_lcore_set(sid, slcore_id, 1),
"Enabling valid service on valid core failed");
TEST_ASSERT_EQUAL(0, rte_service_lcore_start(slcore_id),
"Service core start after add failed");
--
2.7.4
^ permalink raw reply [flat|nested] 32+ messages in thread
* [dpdk-dev] [PATCH v2 03/15] service: rework register to return service id
2017-08-21 12:58 ` [dpdk-dev] [PATCH v2 00/15] service: API improvements and updates Harry van Haaren
2017-08-21 12:58 ` [dpdk-dev] [PATCH v2 01/15] service: rework probe and get name to use ids Harry van Haaren
2017-08-21 12:58 ` [dpdk-dev] [PATCH v2 02/15] service: rework lcore to service map functions Harry van Haaren
@ 2017-08-21 12:58 ` Harry van Haaren
2017-08-21 12:58 ` [dpdk-dev] [PATCH v2 04/15] service: rework service start stop to runstate Harry van Haaren
` (12 subsequent siblings)
15 siblings, 0 replies; 32+ messages in thread
From: Harry van Haaren @ 2017-08-21 12:58 UTC (permalink / raw)
To: dev; +Cc: Harry van Haaren
This commit reworks the service register function to accept
an extra parameter. The parameter is a uint32_t *, which when
provided will be set to the integer service_id that the newly
registered service is represented by.
This is useful for services that wish to validate settings at
a later point in time - they need to know their own service id.
This commit updates the eventdev sw pmd, as well as unit tests
to use the new register API.
Signed-off-by: Harry van Haaren <harry.van.haaren@intel.com>
---
v2:
- Rename rte_service_register to rte_service_component_register
This makes it clearer which APIs apply to components vs services.
---
drivers/event/sw/sw_evdev.c | 4 +--
drivers/event/sw/sw_evdev.h | 1 +
lib/librte_eal/bsdapp/eal/rte_eal_version.map | 2 +-
.../common/include/rte_service_component.h | 6 +++-
lib/librte_eal/common/rte_service.c | 6 +++-
lib/librte_eal/linuxapp/eal/rte_eal_version.map | 2 +-
test/test/test_service_cores.c | 36 ++++++++++++----------
7 files changed, 35 insertions(+), 22 deletions(-)
diff --git a/drivers/event/sw/sw_evdev.c b/drivers/event/sw/sw_evdev.c
index 9c534b7..9cc63a0 100644
--- a/drivers/event/sw/sw_evdev.c
+++ b/drivers/event/sw/sw_evdev.c
@@ -620,7 +620,7 @@ sw_start(struct rte_eventdev *dev)
struct rte_service_spec *s = rte_service_get_by_name(sw->service_name);
if (!rte_service_is_running(s))
SW_LOG_ERR("Warning: No Service core enabled on service %s\n",
- s->name);
+ sw->service_name);
/* check all ports are set up */
for (i = 0; i < sw->port_count; i++)
@@ -855,7 +855,7 @@ sw_probe(struct rte_vdev_device *vdev)
service.callback = sw_sched_service_func;
service.callback_userdata = (void *)dev;
- int32_t ret = rte_service_register(&service);
+ int32_t ret = rte_service_component_register(&service, &sw->service_id);
if (ret) {
SW_LOG_ERR("service register() failed");
return -ENOEXEC;
diff --git a/drivers/event/sw/sw_evdev.h b/drivers/event/sw/sw_evdev.h
index 71de3c1..e0dec91 100644
--- a/drivers/event/sw/sw_evdev.h
+++ b/drivers/event/sw/sw_evdev.h
@@ -278,6 +278,7 @@ struct sw_evdev {
uint16_t xstats_count_per_qid[RTE_EVENT_MAX_QUEUES_PER_DEV];
uint16_t xstats_offset_for_qid[RTE_EVENT_MAX_QUEUES_PER_DEV];
+ uint32_t service_id;
char service_name[SW_PMD_NAME_MAX];
};
diff --git a/lib/librte_eal/bsdapp/eal/rte_eal_version.map b/lib/librte_eal/bsdapp/eal/rte_eal_version.map
index f626a6f..2b1089d 100644
--- a/lib/librte_eal/bsdapp/eal/rte_eal_version.map
+++ b/lib/librte_eal/bsdapp/eal/rte_eal_version.map
@@ -212,6 +212,7 @@ EXPERIMENTAL {
rte_eal_devargs_remove;
rte_eal_hotplug_add;
rte_eal_hotplug_remove;
+ rte_service_component_register;
rte_service_dump;
rte_service_get_by_id;
rte_service_get_by_name;
@@ -227,7 +228,6 @@ EXPERIMENTAL {
rte_service_map_lcore_get;
rte_service_map_lcore_set;
rte_service_probe_capability;
- rte_service_register;
rte_service_reset;
rte_service_set_stats_enable;
rte_service_start;
diff --git a/lib/librte_eal/common/include/rte_service_component.h b/lib/librte_eal/common/include/rte_service_component.h
index 7a946a1..6ef3ee4 100644
--- a/lib/librte_eal/common/include/rte_service_component.h
+++ b/lib/librte_eal/common/include/rte_service_component.h
@@ -89,11 +89,15 @@ struct rte_service_spec {
* *rte_service_set_coremask*.
*
* @param spec The specification of the service to register
+ * @param[out] service_id A pointer to a uint32_t, which will be filled in
+ * during registration of the service. It is set to the integers
+ * service number given to the service. This parameter may be NULL.
* @retval 0 Successfully registered the service.
* -EINVAL Attempted to register an invalid service (eg, no callback
* set)
*/
-int32_t rte_service_register(const struct rte_service_spec *spec);
+int32_t rte_service_component_register(const struct rte_service_spec *spec,
+ uint32_t *service_id);
/**
* @warning
diff --git a/lib/librte_eal/common/rte_service.c b/lib/librte_eal/common/rte_service.c
index c5a8d0d..152eafe 100644
--- a/lib/librte_eal/common/rte_service.c
+++ b/lib/librte_eal/common/rte_service.c
@@ -242,7 +242,8 @@ rte_service_is_running(const struct rte_service_spec *spec)
}
int32_t
-rte_service_register(const struct rte_service_spec *spec)
+rte_service_component_register(const struct rte_service_spec *spec,
+ uint32_t *id_ptr)
{
uint32_t i;
int32_t free_slot = -1;
@@ -267,6 +268,9 @@ rte_service_register(const struct rte_service_spec *spec)
rte_smp_wmb();
rte_service_count++;
+ if (id_ptr)
+ *id_ptr = free_slot;
+
return 0;
}
diff --git a/lib/librte_eal/linuxapp/eal/rte_eal_version.map b/lib/librte_eal/linuxapp/eal/rte_eal_version.map
index 452ae1c..336e40e 100644
--- a/lib/librte_eal/linuxapp/eal/rte_eal_version.map
+++ b/lib/librte_eal/linuxapp/eal/rte_eal_version.map
@@ -217,6 +217,7 @@ EXPERIMENTAL {
rte_eal_devargs_remove;
rte_eal_hotplug_add;
rte_eal_hotplug_remove;
+ rte_service_component_register;
rte_service_dump;
rte_service_get_by_id;
rte_service_get_by_name;
@@ -232,7 +233,6 @@ EXPERIMENTAL {
rte_service_map_lcore_get;
rte_service_map_lcore_set;
rte_service_probe_capability;
- rte_service_register;
rte_service_reset;
rte_service_set_stats_enable;
rte_service_start;
diff --git a/test/test/test_service_cores.c b/test/test/test_service_cores.c
index fd63efd..7767fa2 100644
--- a/test/test/test_service_cores.c
+++ b/test/test/test_service_cores.c
@@ -160,15 +160,17 @@ dummy_register(void)
struct rte_service_spec service;
memset(&service, 0, sizeof(struct rte_service_spec));
- TEST_ASSERT_EQUAL(-EINVAL, rte_service_register(&service),
+ TEST_ASSERT_EQUAL(-EINVAL,
+ rte_service_component_register(&service, NULL),
"Invalid callback");
service.callback = dummy_cb;
- TEST_ASSERT_EQUAL(-EINVAL, rte_service_register(&service),
+ TEST_ASSERT_EQUAL(-EINVAL,
+ rte_service_component_register(&service, NULL),
"Invalid name");
snprintf(service.name, sizeof(service.name), DUMMY_SERVICE_NAME);
- TEST_ASSERT_EQUAL(0, rte_service_register(&service),
+ TEST_ASSERT_EQUAL(0, rte_service_component_register(&service, NULL),
"Failed to register valid service");
return TEST_SUCCESS;
@@ -187,13 +189,15 @@ service_get_by_name(void)
/* register service */
struct rte_service_spec service;
memset(&service, 0, sizeof(struct rte_service_spec));
- TEST_ASSERT_EQUAL(-EINVAL, rte_service_register(&service),
+ TEST_ASSERT_EQUAL(-EINVAL,
+ rte_service_component_register(&service, NULL),
"Invalid callback");
service.callback = dummy_cb;
- TEST_ASSERT_EQUAL(-EINVAL, rte_service_register(&service),
+ TEST_ASSERT_EQUAL(-EINVAL,
+ rte_service_component_register(&service, NULL),
"Invalid name");
snprintf(service.name, sizeof(service.name), DUMMY_SERVICE_NAME);
- TEST_ASSERT_EQUAL(0, rte_service_register(&service),
+ TEST_ASSERT_EQUAL(0, rte_service_component_register(&service, NULL),
"Failed to register valid service");
/* ensure with dummy services registered returns same ptr as ID */
@@ -221,7 +225,7 @@ service_probe_capability(void)
service.callback = dummy_cb;
snprintf(service.name, sizeof(service.name), DUMMY_SERVICE_NAME);
service.capabilities |= RTE_SERVICE_CAP_MT_SAFE;
- TEST_ASSERT_EQUAL(0, rte_service_register(&service),
+ TEST_ASSERT_EQUAL(0, rte_service_component_register(&service, NULL),
"Register of MT SAFE service failed");
/* verify flag is enabled */
@@ -235,7 +239,7 @@ service_probe_capability(void)
memset(&service, 0, sizeof(struct rte_service_spec));
service.callback = dummy_cb;
snprintf(service.name, sizeof(service.name), DUMMY_SERVICE_NAME);
- TEST_ASSERT_EQUAL(0, rte_service_register(&service),
+ TEST_ASSERT_EQUAL(0, rte_service_component_register(&service, NULL),
"Register of non-MT safe service failed");
/* verify flag is enabled */
@@ -274,28 +278,28 @@ static int
service_start_stop(void)
{
const uint32_t sid = 0;
- struct rte_service_spec *service = rte_service_get_by_id(0);
+ struct rte_service_spec *s = rte_service_get_by_id(0);
- /* is_running() returns if service is running and slcore is mapped */
+ /* runstate_get() returns if service is running and slcore is mapped */
TEST_ASSERT_EQUAL(0, rte_service_lcore_add(slcore_id),
"Service core add did not return zero");
int ret = rte_service_map_lcore_set(sid, slcore_id, 1);
TEST_ASSERT_EQUAL(0, ret,
"Enabling service core, expected 0 got %d", ret);
- TEST_ASSERT_EQUAL(0, rte_service_is_running(service),
+ TEST_ASSERT_EQUAL(0, rte_service_is_running(s),
"Error: Service should be stopped");
- TEST_ASSERT_EQUAL(0, rte_service_stop(service),
+ TEST_ASSERT_EQUAL(0, rte_service_stop(s),
"Error: Service stopped returned non-zero");
- TEST_ASSERT_EQUAL(0, rte_service_is_running(service),
+ TEST_ASSERT_EQUAL(0, rte_service_is_running(s),
"Error: Service is running - should be stopped");
- TEST_ASSERT_EQUAL(0, rte_service_start(service),
+ TEST_ASSERT_EQUAL(0, rte_service_start(s),
"Error: Service start returned non-zero");
- TEST_ASSERT_EQUAL(1, rte_service_is_running(service),
+ TEST_ASSERT_EQUAL(1, rte_service_is_running(s),
"Error: Service is not running");
return unregister_all();
@@ -471,7 +475,7 @@ service_threaded_test(int mt_safe)
service.callback = dummy_mt_unsafe_cb;
}
- TEST_ASSERT_EQUAL(0, rte_service_register(&service),
+ TEST_ASSERT_EQUAL(0, rte_service_component_register(&service, NULL),
"Register of MT SAFE service failed");
struct rte_service_spec *s = rte_service_get_by_id(0);
--
2.7.4
^ permalink raw reply [flat|nested] 32+ messages in thread
* [dpdk-dev] [PATCH v2 04/15] service: rework service start stop to runstate
2017-08-21 12:58 ` [dpdk-dev] [PATCH v2 00/15] service: API improvements and updates Harry van Haaren
` (2 preceding siblings ...)
2017-08-21 12:58 ` [dpdk-dev] [PATCH v2 03/15] service: rework register to return service id Harry van Haaren
@ 2017-08-21 12:58 ` Harry van Haaren
2017-08-21 12:58 ` [dpdk-dev] [PATCH v2 05/15] service: rework service stats functions Harry van Haaren
` (11 subsequent siblings)
15 siblings, 0 replies; 32+ messages in thread
From: Harry van Haaren @ 2017-08-21 12:58 UTC (permalink / raw)
To: dev; +Cc: Harry van Haaren
This commit reworks the API to move from two separate start
and stop functions, to a "runstate" API which allows setting
the runstate. The is_running API is replaced with an function
to query the runstate. The runstate functions take a id value
for service. Unit tests and the eventdev sw pmd are updated.
Signed-off-by: Harry van Haaren <harry.van.haaren@intel.com>
---
drivers/event/sw/sw_evdev.c | 3 +-
lib/librte_eal/bsdapp/eal/rte_eal_version.map | 5 ++--
lib/librte_eal/common/include/rte_service.h | 37 +++++++++---------------
lib/librte_eal/common/rte_service.c | 38 ++++++++++---------------
lib/librte_eal/linuxapp/eal/rte_eal_version.map | 5 ++--
test/test/test_service_cores.c | 19 ++++++-------
6 files changed, 42 insertions(+), 65 deletions(-)
diff --git a/drivers/event/sw/sw_evdev.c b/drivers/event/sw/sw_evdev.c
index 9cc63a0..da6ac30 100644
--- a/drivers/event/sw/sw_evdev.c
+++ b/drivers/event/sw/sw_evdev.c
@@ -617,8 +617,7 @@ sw_start(struct rte_eventdev *dev)
struct sw_evdev *sw = sw_pmd_priv(dev);
/* check a service core is mapped to this service */
- struct rte_service_spec *s = rte_service_get_by_name(sw->service_name);
- if (!rte_service_is_running(s))
+ if (!rte_service_runstate_get(sw->service_id))
SW_LOG_ERR("Warning: No Service core enabled on service %s\n",
sw->service_name);
diff --git a/lib/librte_eal/bsdapp/eal/rte_eal_version.map b/lib/librte_eal/bsdapp/eal/rte_eal_version.map
index 2b1089d..b336f54 100644
--- a/lib/librte_eal/bsdapp/eal/rte_eal_version.map
+++ b/lib/librte_eal/bsdapp/eal/rte_eal_version.map
@@ -217,7 +217,6 @@ EXPERIMENTAL {
rte_service_get_by_id;
rte_service_get_by_name;
rte_service_get_count;
- rte_service_is_running;
rte_service_lcore_add;
rte_service_lcore_count;
rte_service_lcore_del;
@@ -229,10 +228,10 @@ EXPERIMENTAL {
rte_service_map_lcore_set;
rte_service_probe_capability;
rte_service_reset;
+ rte_service_runstate_get;
+ rte_service_runstate_set;
rte_service_set_stats_enable;
- rte_service_start;
rte_service_start_with_defaults;
- rte_service_stop;
rte_service_unregister;
} DPDK_17.08;
diff --git a/lib/librte_eal/common/include/rte_service.h b/lib/librte_eal/common/include/rte_service.h
index de69695..e133ca1 100644
--- a/lib/librte_eal/common/include/rte_service.h
+++ b/lib/librte_eal/common/include/rte_service.h
@@ -192,40 +192,31 @@ int32_t rte_service_map_lcore_get(uint32_t service_id, uint32_t lcore);
* @warning
* @b EXPERIMENTAL: this API may change without prior notice
*
- * Enable *service* to run.
+ * Set the runstate of the service.
*
- * This function switches on a service during runtime.
- * @retval 0 The service was successfully started
- */
-int32_t rte_service_start(struct rte_service_spec *service);
-
-/**
- * @warning
- * @b EXPERIMENTAL: this API may change without prior notice
+ * Each service is either running or stopped. Setting a non-zero runstate
+ * enables the service to run, while setting runstate zero disables it.
*
- * Disable *service*.
+ * @param id The id of the service
+ * @param runstate The run state to apply to the service
*
- * Switch off a service, so it is not run until it is *rte_service_start* is
- * called on it.
- * @retval 0 Service successfully switched off
+ * @retval 0 The service was successfully started
+ * @retval -EINVAL Invalid service id
*/
-int32_t rte_service_stop(struct rte_service_spec *service);
+int32_t rte_service_runstate_set(uint32_t id, uint32_t runstate);
/**
* @warning
* @b EXPERIMENTAL: this API may change without prior notice
*
- * Returns if *service* is currently running.
- *
- * This function returns true if the service has been started using
- * *rte_service_start*, AND a service core is mapped to the service. This
- * function can be used to ensure that the service will be run.
+ * Get the runstate for the service with *id*. See *rte_service_runstate_set*
+ * for details of runstates.
*
- * @retval 1 Service is currently running, and has a service lcore mapped
- * @retval 0 Service is currently stopped, or no service lcore is mapped
- * @retval -EINVAL Invalid service pointer provided
+ * @retval 1 Service is running
+ * @retval 0 Service is stopped
+ * @retval -EINVAL Invalid service id
*/
-int32_t rte_service_is_running(const struct rte_service_spec *service);
+int32_t rte_service_runstate_get(uint32_t id);
/**
* @warning
diff --git a/lib/librte_eal/common/rte_service.c b/lib/librte_eal/common/rte_service.c
index 152eafe..2ff1ab0 100644
--- a/lib/librte_eal/common/rte_service.c
+++ b/lib/librte_eal/common/rte_service.c
@@ -230,18 +230,6 @@ rte_service_probe_capability(uint32_t id, uint32_t capability)
}
int32_t
-rte_service_is_running(const struct rte_service_spec *spec)
-{
- const struct rte_service_spec_impl *impl =
- (const struct rte_service_spec_impl *)spec;
- if (!impl)
- return -EINVAL;
-
- return (impl->runstate == RUNSTATE_RUNNING) &&
- (impl->num_mapped_cores > 0);
-}
-
-int32_t
rte_service_component_register(const struct rte_service_spec *spec,
uint32_t *id_ptr)
{
@@ -309,23 +297,27 @@ rte_service_unregister(struct rte_service_spec *spec)
}
int32_t
-rte_service_start(struct rte_service_spec *service)
+rte_service_runstate_set(uint32_t id, uint32_t runstate)
{
- struct rte_service_spec_impl *s =
- (struct rte_service_spec_impl *)service;
- s->runstate = RUNSTATE_RUNNING;
+ struct rte_service_spec_impl *s;
+ SERVICE_VALID_GET_OR_ERR_RET(id, s, -EINVAL);
+
+ if (runstate)
+ s->runstate = RUNSTATE_RUNNING;
+ else
+ s->runstate = RUNSTATE_STOPPED;
+
rte_smp_wmb();
return 0;
}
int32_t
-rte_service_stop(struct rte_service_spec *service)
+rte_service_runstate_get(uint32_t id)
{
- struct rte_service_spec_impl *s =
- (struct rte_service_spec_impl *)service;
- s->runstate = RUNSTATE_STOPPED;
- rte_smp_wmb();
- return 0;
+ struct rte_service_spec_impl *s;
+ SERVICE_VALID_GET_OR_ERR_RET(id, s, -EINVAL);
+
+ return (s->runstate == RUNSTATE_RUNNING) && (s->num_mapped_cores > 0);
}
static int32_t
@@ -448,7 +440,7 @@ rte_service_start_with_defaults(void)
if (lcore_iter >= lcore_count)
lcore_iter = 0;
- ret = rte_service_start(s);
+ ret = rte_service_runstate_set(i, 1);
if (ret)
return -ENOEXEC;
}
diff --git a/lib/librte_eal/linuxapp/eal/rte_eal_version.map b/lib/librte_eal/linuxapp/eal/rte_eal_version.map
index 336e40e..b2b2235 100644
--- a/lib/librte_eal/linuxapp/eal/rte_eal_version.map
+++ b/lib/librte_eal/linuxapp/eal/rte_eal_version.map
@@ -222,7 +222,6 @@ EXPERIMENTAL {
rte_service_get_by_id;
rte_service_get_by_name;
rte_service_get_count;
- rte_service_is_running;
rte_service_lcore_add;
rte_service_lcore_count;
rte_service_lcore_del;
@@ -234,10 +233,10 @@ EXPERIMENTAL {
rte_service_map_lcore_set;
rte_service_probe_capability;
rte_service_reset;
+ rte_service_runstate_get;
+ rte_service_runstate_set;
rte_service_set_stats_enable;
- rte_service_start;
rte_service_start_with_defaults;
- rte_service_stop;
rte_service_unregister;
} DPDK_17.08;
diff --git a/test/test/test_service_cores.c b/test/test/test_service_cores.c
index 7767fa2..196277b 100644
--- a/test/test/test_service_cores.c
+++ b/test/test/test_service_cores.c
@@ -278,7 +278,6 @@ static int
service_start_stop(void)
{
const uint32_t sid = 0;
- struct rte_service_spec *s = rte_service_get_by_id(0);
/* runstate_get() returns if service is running and slcore is mapped */
TEST_ASSERT_EQUAL(0, rte_service_lcore_add(slcore_id),
@@ -287,19 +286,19 @@ service_start_stop(void)
TEST_ASSERT_EQUAL(0, ret,
"Enabling service core, expected 0 got %d", ret);
- TEST_ASSERT_EQUAL(0, rte_service_is_running(s),
+ TEST_ASSERT_EQUAL(0, rte_service_runstate_get(sid),
"Error: Service should be stopped");
- TEST_ASSERT_EQUAL(0, rte_service_stop(s),
+ TEST_ASSERT_EQUAL(0, rte_service_runstate_set(sid, 0),
"Error: Service stopped returned non-zero");
- TEST_ASSERT_EQUAL(0, rte_service_is_running(s),
+ TEST_ASSERT_EQUAL(0, rte_service_runstate_get(sid),
"Error: Service is running - should be stopped");
- TEST_ASSERT_EQUAL(0, rte_service_start(s),
+ TEST_ASSERT_EQUAL(0, rte_service_runstate_set(sid, 1),
"Error: Service start returned non-zero");
- TEST_ASSERT_EQUAL(1, rte_service_is_running(s),
+ TEST_ASSERT_EQUAL(1, rte_service_runstate_get(sid),
"Error: Service is not running");
return unregister_all();
@@ -478,9 +477,8 @@ service_threaded_test(int mt_safe)
TEST_ASSERT_EQUAL(0, rte_service_component_register(&service, NULL),
"Register of MT SAFE service failed");
- struct rte_service_spec *s = rte_service_get_by_id(0);
const uint32_t sid = 0;
- TEST_ASSERT_EQUAL(0, rte_service_start(s),
+ TEST_ASSERT_EQUAL(0, rte_service_runstate_set(sid, 1),
"Starting valid service failed");
TEST_ASSERT_EQUAL(0, rte_service_map_lcore_set(sid, slcore_1, 1),
"Failed to enable lcore 1 on mt safe service");
@@ -497,7 +495,7 @@ service_threaded_test(int mt_safe)
TEST_ASSERT_EQUAL(1, test_params[1],
"MT Safe service not run by two cores concurrently");
- TEST_ASSERT_EQUAL(0, rte_service_stop(s),
+ TEST_ASSERT_EQUAL(0, rte_service_runstate_set(sid, 0),
"Failed to stop MT Safe service");
unregister_all();
@@ -536,8 +534,7 @@ service_lcore_start_stop(void)
{
/* start service core and service, create mapping so tick() runs */
const uint32_t sid = 0;
- struct rte_service_spec *s = rte_service_get_by_id(0);
- TEST_ASSERT_EQUAL(0, rte_service_start(s),
+ TEST_ASSERT_EQUAL(0, rte_service_runstate_set(sid, 1),
"Starting valid service failed");
TEST_ASSERT_EQUAL(-EINVAL, rte_service_map_lcore_set(sid, slcore_id, 1),
"Enabling valid service on non-service core must fail");
--
2.7.4
^ permalink raw reply [flat|nested] 32+ messages in thread
* [dpdk-dev] [PATCH v2 05/15] service: rework service stats functions
2017-08-21 12:58 ` [dpdk-dev] [PATCH v2 00/15] service: API improvements and updates Harry van Haaren
` (3 preceding siblings ...)
2017-08-21 12:58 ` [dpdk-dev] [PATCH v2 04/15] service: rework service start stop to runstate Harry van Haaren
@ 2017-08-21 12:58 ` Harry van Haaren
2017-08-21 12:58 ` [dpdk-dev] [PATCH v2 06/15] service: rework unregister api to use integers Harry van Haaren
` (10 subsequent siblings)
15 siblings, 0 replies; 32+ messages in thread
From: Harry van Haaren @ 2017-08-21 12:58 UTC (permalink / raw)
To: dev; +Cc: Harry van Haaren
This commit reworks the statistics functions to use integer ids
for services instead of pointers. Passing UINT32_MAX to the dump
function prints all info, similar to passing NULL previously.
Signed-off-by: Harry van Haaren <harry.van.haaren@intel.com>
---
lib/librte_eal/common/include/rte_service.h | 14 ++++++++------
lib/librte_eal/common/rte_service.c | 24 ++++++++++++------------
test/test/test_service_cores.c | 10 +++++-----
3 files changed, 25 insertions(+), 23 deletions(-)
diff --git a/lib/librte_eal/common/include/rte_service.h b/lib/librte_eal/common/include/rte_service.h
index e133ca1..afa6c0bf 100644
--- a/lib/librte_eal/common/include/rte_service.h
+++ b/lib/librte_eal/common/include/rte_service.h
@@ -318,13 +318,12 @@ int32_t rte_service_lcore_reset_all(void);
* Enable or disable statistics collection for *service*.
*
* This function enables per core, per-service cycle count collection.
- * @param service The service to enable statistics gathering on.
+ * @param id The service to enable statistics gathering on.
* @param enable Zero to disable statistics, non-zero to enable.
* @retval 0 Success
* @retval -EINVAL Invalid service pointer passed
*/
-int32_t rte_service_set_stats_enable(struct rte_service_spec *service,
- int32_t enable);
+int32_t rte_service_set_stats_enable(uint32_t id, int32_t enable);
/**
* @warning
@@ -351,10 +350,13 @@ int32_t rte_service_lcore_list(uint32_t array[], uint32_t n);
* @warning
* @b EXPERIMENTAL: this API may change without prior notice
*
- * Dumps any information available about the service. If service is NULL,
- * dumps info for all services.
+ * Dumps any information available about the service. When id is UINT32_MAX,
+ * this function dumps info for all services.
+ *
+ * @retval 0 Statistics have been successfully dumped
+ * @retval -EINVAL Invalid service id provided
*/
-int32_t rte_service_dump(FILE *f, struct rte_service_spec *service);
+int32_t rte_service_dump(FILE *f, uint32_t id);
#ifdef __cplusplus
}
diff --git a/lib/librte_eal/common/rte_service.c b/lib/librte_eal/common/rte_service.c
index 2ff1ab0..ac1c90c 100644
--- a/lib/librte_eal/common/rte_service.c
+++ b/lib/librte_eal/common/rte_service.c
@@ -166,18 +166,15 @@ service_mt_safe(struct rte_service_spec_impl *s)
return s->spec.capabilities & RTE_SERVICE_CAP_MT_SAFE;
}
-int32_t rte_service_set_stats_enable(struct rte_service_spec *service,
- int32_t enabled)
+int32_t rte_service_set_stats_enable(uint32_t id, int32_t enabled)
{
- struct rte_service_spec_impl *impl =
- (struct rte_service_spec_impl *)service;
- if (!impl)
- return -EINVAL;
+ struct rte_service_spec_impl *s;
+ SERVICE_VALID_GET_OR_ERR_RET(id, s, 0);
if (enabled)
- impl->internal_flags |= SERVICE_F_STATS_ENABLED;
+ s->internal_flags |= SERVICE_F_STATS_ENABLED;
else
- impl->internal_flags &= ~(SERVICE_F_STATS_ENABLED);
+ s->internal_flags &= ~(SERVICE_F_STATS_ENABLED);
return 0;
}
@@ -670,9 +667,10 @@ service_dump_calls_per_lcore(FILE *f, uint32_t lcore, uint32_t reset)
fprintf(f, "\n");
}
-int32_t rte_service_dump(FILE *f, struct rte_service_spec *service)
+int32_t rte_service_dump(FILE *f, uint32_t id)
{
uint32_t i;
+ int print_one = (id != UINT32_MAX);
uint64_t total_cycles = 0;
for (i = 0; i < rte_service_count; i++) {
@@ -681,15 +679,17 @@ int32_t rte_service_dump(FILE *f, struct rte_service_spec *service)
total_cycles += rte_services[i].cycles_spent;
}
- if (service) {
- struct rte_service_spec_impl *s =
- (struct rte_service_spec_impl *)service;
+ /* print only the specified service */
+ if (print_one) {
+ struct rte_service_spec_impl *s;
+ SERVICE_VALID_GET_OR_ERR_RET(id, s, -EINVAL);
fprintf(f, "Service %s Summary\n", s->spec.name);
uint32_t reset = 0;
rte_service_dump_one(f, s, total_cycles, reset);
return 0;
}
+ /* print all services, as UINT32_MAX was passed as id */
fprintf(f, "Services Summary\n");
for (i = 0; i < rte_service_count; i++) {
uint32_t reset = 1;
diff --git a/test/test/test_service_cores.c b/test/test/test_service_cores.c
index 196277b..d52fe49 100644
--- a/test/test/test_service_cores.c
+++ b/test/test/test_service_cores.c
@@ -265,11 +265,11 @@ service_name(void)
static int
service_dump(void)
{
- struct rte_service_spec *service = rte_service_get_by_id(0);
- rte_service_set_stats_enable(service, 1);
- rte_service_dump(stdout, service);
- rte_service_set_stats_enable(service, 0);
- rte_service_dump(stdout, service);
+ const uint32_t sid = 0;
+ rte_service_set_stats_enable(sid, 1);
+ rte_service_dump(stdout, 0);
+ rte_service_set_stats_enable(sid, 0);
+ rte_service_dump(stdout, 0);
return unregister_all();
}
--
2.7.4
^ permalink raw reply [flat|nested] 32+ messages in thread
* [dpdk-dev] [PATCH v2 06/15] service: rework unregister api to use integers
2017-08-21 12:58 ` [dpdk-dev] [PATCH v2 00/15] service: API improvements and updates Harry van Haaren
` (4 preceding siblings ...)
2017-08-21 12:58 ` [dpdk-dev] [PATCH v2 05/15] service: rework service stats functions Harry van Haaren
@ 2017-08-21 12:58 ` Harry van Haaren
2017-08-21 12:58 ` [dpdk-dev] [PATCH v2 07/15] service: rework get by name function to use id Harry van Haaren
` (9 subsequent siblings)
15 siblings, 0 replies; 32+ messages in thread
From: Harry van Haaren @ 2017-08-21 12:58 UTC (permalink / raw)
To: dev; +Cc: Harry van Haaren
This commit reworks the unregister API to accept an integer.
Signed-off-by: Harry van Haaren <harry.van.haaren@intel.com>
---
v2:
- Renamed unregister to rte_service_component_unregister
for consistency in APIs for components.
---
lib/librte_eal/bsdapp/eal/rte_eal_version.map | 2 +-
.../common/include/rte_service_component.h | 4 ++--
lib/librte_eal/common/rte_service.c | 25 ++++++----------------
lib/librte_eal/linuxapp/eal/rte_eal_version.map | 2 +-
test/test/test_service_cores.c | 10 +++------
5 files changed, 13 insertions(+), 30 deletions(-)
diff --git a/lib/librte_eal/bsdapp/eal/rte_eal_version.map b/lib/librte_eal/bsdapp/eal/rte_eal_version.map
index b336f54..507a38e 100644
--- a/lib/librte_eal/bsdapp/eal/rte_eal_version.map
+++ b/lib/librte_eal/bsdapp/eal/rte_eal_version.map
@@ -213,6 +213,7 @@ EXPERIMENTAL {
rte_eal_hotplug_add;
rte_eal_hotplug_remove;
rte_service_component_register;
+ rte_service_component_unregister;
rte_service_dump;
rte_service_get_by_id;
rte_service_get_by_name;
@@ -232,6 +233,5 @@ EXPERIMENTAL {
rte_service_runstate_set;
rte_service_set_stats_enable;
rte_service_start_with_defaults;
- rte_service_unregister;
} DPDK_17.08;
diff --git a/lib/librte_eal/common/include/rte_service_component.h b/lib/librte_eal/common/include/rte_service_component.h
index 6ef3ee4..af632c6 100644
--- a/lib/librte_eal/common/include/rte_service_component.h
+++ b/lib/librte_eal/common/include/rte_service_component.h
@@ -103,7 +103,7 @@ int32_t rte_service_component_register(const struct rte_service_spec *spec,
* @warning
* @b EXPERIMENTAL: this API may change without prior notice
*
- * Unregister a service.
+ * Unregister a service component.
*
* The service being removed must be stopped before calling this function.
*
@@ -111,7 +111,7 @@ int32_t rte_service_component_register(const struct rte_service_spec *spec,
* @retval -EBUSY The service is currently running, stop the service before
* calling unregister. No action has been taken.
*/
-int32_t rte_service_unregister(struct rte_service_spec *service);
+int32_t rte_service_component_unregister(uint32_t id);
/**
* @warning
diff --git a/lib/librte_eal/common/rte_service.c b/lib/librte_eal/common/rte_service.c
index ac1c90c..adcd860 100644
--- a/lib/librte_eal/common/rte_service.c
+++ b/lib/librte_eal/common/rte_service.c
@@ -260,35 +260,22 @@ rte_service_component_register(const struct rte_service_spec *spec,
}
int32_t
-rte_service_unregister(struct rte_service_spec *spec)
+rte_service_component_unregister(uint32_t id)
{
- struct rte_service_spec_impl *s = NULL;
- struct rte_service_spec_impl *spec_impl =
- (struct rte_service_spec_impl *)spec;
-
uint32_t i;
- uint32_t service_id;
- for (i = 0; i < RTE_SERVICE_NUM_MAX; i++) {
- if (&rte_services[i] == spec_impl) {
- s = spec_impl;
- service_id = i;
- break;
- }
- }
-
- if (!s)
- return -EINVAL;
+ struct rte_service_spec_impl *s;
+ SERVICE_VALID_GET_OR_ERR_RET(id, s, -EINVAL);
rte_service_count--;
rte_smp_wmb();
s->internal_flags &= ~(SERVICE_F_REGISTERED);
+ /* clear the run-bit in all cores */
for (i = 0; i < RTE_MAX_LCORE; i++)
- lcore_states[i].service_mask &= ~(UINT64_C(1) << service_id);
+ lcore_states[i].service_mask &= ~(UINT64_C(1) << id);
- memset(&rte_services[service_id], 0,
- sizeof(struct rte_service_spec_impl));
+ memset(&rte_services[id], 0, sizeof(struct rte_service_spec_impl));
return 0;
}
diff --git a/lib/librte_eal/linuxapp/eal/rte_eal_version.map b/lib/librte_eal/linuxapp/eal/rte_eal_version.map
index b2b2235..a7431da 100644
--- a/lib/librte_eal/linuxapp/eal/rte_eal_version.map
+++ b/lib/librte_eal/linuxapp/eal/rte_eal_version.map
@@ -218,6 +218,7 @@ EXPERIMENTAL {
rte_eal_hotplug_add;
rte_eal_hotplug_remove;
rte_service_component_register;
+ rte_service_component_unregister;
rte_service_dump;
rte_service_get_by_id;
rte_service_get_by_name;
@@ -237,6 +238,5 @@ EXPERIMENTAL {
rte_service_runstate_set;
rte_service_set_stats_enable;
rte_service_start_with_defaults;
- rte_service_unregister;
} DPDK_17.08;
diff --git a/test/test/test_service_cores.c b/test/test/test_service_cores.c
index d52fe49..e650b20 100644
--- a/test/test/test_service_cores.c
+++ b/test/test/test_service_cores.c
@@ -131,17 +131,13 @@ static int
unregister_all(void)
{
uint32_t i;
- struct rte_service_spec *dead = (struct rte_service_spec *)0xdead;
- TEST_ASSERT_EQUAL(-EINVAL, rte_service_unregister(0),
- "Unregistered NULL pointer");
- TEST_ASSERT_EQUAL(-EINVAL, rte_service_unregister(dead),
- "Unregistered invalid pointer");
+ TEST_ASSERT_EQUAL(-EINVAL, rte_service_component_unregister(1000),
+ "Unregistered invalid service id");
uint32_t c = rte_service_get_count();
for (i = 0; i < c; i++) {
- struct rte_service_spec *s = rte_service_get_by_id(i);
- TEST_ASSERT_EQUAL(0, rte_service_unregister(s),
+ TEST_ASSERT_EQUAL(0, rte_service_component_unregister(i),
"Error unregistering a valid service");
}
--
2.7.4
^ permalink raw reply [flat|nested] 32+ messages in thread
* [dpdk-dev] [PATCH v2 07/15] service: rework get by name function to use id
2017-08-21 12:58 ` [dpdk-dev] [PATCH v2 00/15] service: API improvements and updates Harry van Haaren
` (5 preceding siblings ...)
2017-08-21 12:58 ` [dpdk-dev] [PATCH v2 06/15] service: rework unregister api to use integers Harry van Haaren
@ 2017-08-21 12:58 ` Harry van Haaren
2017-08-21 12:58 ` [dpdk-dev] [PATCH v2 08/15] service: fix and refactor atomic service accesses Harry van Haaren
` (8 subsequent siblings)
15 siblings, 0 replies; 32+ messages in thread
From: Harry van Haaren @ 2017-08-21 12:58 UTC (permalink / raw)
To: dev; +Cc: Harry van Haaren
This commit reworks the service_get_by_name() function to
accept an integer, and removes the service_get_by_id() function.
All functions now accept an integer argument representing the
service, so it is no longer required to expose the service_spec
pointers to the application.
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 | 45 ++++++++++---------------
lib/librte_eal/common/rte_service.c | 24 ++++---------
lib/librte_eal/linuxapp/eal/rte_eal_version.map | 1 +
test/test/test_service_cores.c | 30 ++++++++++-------
5 files changed, 43 insertions(+), 58 deletions(-)
diff --git a/lib/librte_eal/bsdapp/eal/rte_eal_version.map b/lib/librte_eal/bsdapp/eal/rte_eal_version.map
index 507a38e..4b2c36b 100644
--- a/lib/librte_eal/bsdapp/eal/rte_eal_version.map
+++ b/lib/librte_eal/bsdapp/eal/rte_eal_version.map
@@ -218,6 +218,7 @@ EXPERIMENTAL {
rte_service_get_by_id;
rte_service_get_by_name;
rte_service_get_count;
+ rte_service_get_name;
rte_service_lcore_add;
rte_service_lcore_count;
rte_service_lcore_del;
diff --git a/lib/librte_eal/common/include/rte_service.h b/lib/librte_eal/common/include/rte_service.h
index afa6c0bf..e06b075 100644
--- a/lib/librte_eal/common/include/rte_service.h
+++ b/lib/librte_eal/common/include/rte_service.h
@@ -61,9 +61,6 @@ extern "C" {
#include <rte_lcore.h>
-/* forward declaration only. Definition in rte_service_private.h */
-struct rte_service_spec;
-
#define RTE_SERVICE_NAME_MAX 32
/* Capabilities of a service.
@@ -89,40 +86,32 @@ struct rte_service_spec;
*/
uint32_t rte_service_get_count(void);
-
/**
* @warning
* @b EXPERIMENTAL: this API may change without prior notice
*
- * Return the specification of a service by integer id.
- *
- * This function provides the specification of a service. This can be used by
- * the application to understand what the service represents. The service
- * must not be modified by the application directly, only passed to the various
- * rte_service_* functions.
- *
- * @param id The integer id of the service to retrieve
- * @retval non-zero A valid pointer to the service_spec
- * @retval NULL Invalid *id* provided.
- */
-struct rte_service_spec *rte_service_get_by_id(uint32_t id);
-
-/**
- * @warning
- * @b EXPERIMENTAL: this API may change without prior notice
+ * Return the id of a service by name.
*
- * Return the specification of a service by name.
+ * This function provides the id of the service using the service name as
+ * lookup key. The service id is to be passed to other functions in the
+ * rte_service_* API.
*
- * This function provides the specification of a service using the service name
- * as lookup key. This can be used by the application to understand what the
- * service represents. The service must not be modified by the application
- * directly, only passed to the various rte_service_* functions.
+ * Example usage:
+ * @code
+ * uint32_t service_id;
+ * int32_t ret = rte_service_get_by_name("service_X", &service_id);
+ * if (ret) {
+ * // handle error
+ * }
+ * @endcode
*
* @param name The name of the service to retrieve
- * @retval non-zero A valid pointer to the service_spec
- * @retval NULL Invalid *name* provided.
+ * @param[out] service_id A pointer to a uint32_t, to be filled in with the id.
+ * @retval 0 Success. The service id is provided in *service_id*.
+ * @retval -EINVAL Null *service_id* pointer provided
+ * @retval -ENODEV No such service registered
*/
-struct rte_service_spec *rte_service_get_by_name(const char *name);
+int32_t rte_service_get_by_name(const char *name, uint32_t *service_id);
/**
* @warning
diff --git a/lib/librte_eal/common/rte_service.c b/lib/librte_eal/common/rte_service.c
index adcd860..1bfd149 100644
--- a/lib/librte_eal/common/rte_service.c
+++ b/lib/librte_eal/common/rte_service.c
@@ -185,29 +185,21 @@ rte_service_get_count(void)
return rte_service_count;
}
-struct rte_service_spec *
-rte_service_get_by_id(uint32_t id)
+int32_t rte_service_get_by_name(const char *name, uint32_t *service_id)
{
- struct rte_service_spec *service = NULL;
- if (id < rte_service_count)
- service = (struct rte_service_spec *)&rte_services[id];
-
- return service;
-}
+ if (!service_id)
+ return -EINVAL;
-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;
+ *service_id = i;
+ return 0;
}
}
- return service;
+ return -ENODEV;
}
const char *
@@ -407,10 +399,6 @@ rte_service_start_with_defaults(void)
rte_service_lcore_start(ids[i]);
for (i = 0; i < count; i++) {
- struct rte_service_spec *s = rte_service_get_by_id(i);
- if (!s)
- return -EINVAL;
-
/* do 1:1 core mapping here, with each service getting
* assigned a single core by default. Adding multiple services
* should multiplex to a single core, or 1:1 if there are the
diff --git a/lib/librte_eal/linuxapp/eal/rte_eal_version.map b/lib/librte_eal/linuxapp/eal/rte_eal_version.map
index a7431da..f7a7352 100644
--- a/lib/librte_eal/linuxapp/eal/rte_eal_version.map
+++ b/lib/librte_eal/linuxapp/eal/rte_eal_version.map
@@ -223,6 +223,7 @@ EXPERIMENTAL {
rte_service_get_by_id;
rte_service_get_by_name;
rte_service_get_count;
+ rte_service_get_name;
rte_service_lcore_add;
rte_service_lcore_count;
rte_service_lcore_del;
diff --git a/test/test/test_service_cores.c b/test/test/test_service_cores.c
index e650b20..5ae7b20 100644
--- a/test/test/test_service_cores.c
+++ b/test/test/test_service_cores.c
@@ -178,9 +178,13 @@ service_get_by_name(void)
{
unregister_all();
- /* ensure with no services registered returns NULL */
- TEST_ASSERT_EQUAL(0, rte_service_get_by_name(DUMMY_SERVICE_NAME),
- "Service get by name should return NULL");
+ uint32_t sid;
+ TEST_ASSERT_EQUAL(-ENODEV,
+ rte_service_get_by_name(DUMMY_SERVICE_NAME, &sid),
+ "get by name with invalid name should return -ENODEV");
+ TEST_ASSERT_EQUAL(-EINVAL,
+ rte_service_get_by_name(DUMMY_SERVICE_NAME, 0x0),
+ "get by name with NULL ptr should return -ENODEV");
/* register service */
struct rte_service_spec service;
@@ -196,16 +200,19 @@ service_get_by_name(void)
TEST_ASSERT_EQUAL(0, rte_service_component_register(&service, NULL),
"Failed to register valid service");
- /* ensure with dummy services registered returns same ptr as ID */
- struct rte_service_spec *s_by_id = rte_service_get_by_id(0);
- TEST_ASSERT_EQUAL(s_by_id, rte_service_get_by_name(DUMMY_SERVICE_NAME),
- "Service get_by_name should equal get_by_id()");
+ /* we unregistered all service, now registering 1, should be id 0 */
+ uint32_t service_id_as_expected = 0;
+ TEST_ASSERT_EQUAL(0, rte_service_get_by_name(DUMMY_SERVICE_NAME, &sid),
+ "Service get_by_name should return 0 on valid inputs");
+ TEST_ASSERT_EQUAL(service_id_as_expected, sid,
+ "Service get_by_name should equal expected id");
unregister_all();
/* ensure after unregister, get_by_name returns NULL */
- TEST_ASSERT_EQUAL(0, rte_service_get_by_name(DUMMY_SERVICE_NAME),
- "get by name should return NULL after unregister");
+ TEST_ASSERT_EQUAL(-ENODEV,
+ rte_service_get_by_name(DUMMY_SERVICE_NAME, &sid),
+ "get by name should return -ENODEV after unregister");
return TEST_SUCCESS;
}
@@ -249,9 +256,8 @@ service_probe_capability(void)
static int
service_name(void)
{
- struct rte_service_spec *service = rte_service_get_by_id(0);
-
- int equal = strcmp(service->name, DUMMY_SERVICE_NAME);
+ const char *name = rte_service_get_name(0);
+ int equal = strcmp(name, DUMMY_SERVICE_NAME);
TEST_ASSERT_EQUAL(0, equal, "Error: Service name not correct");
return unregister_all();
--
2.7.4
^ permalink raw reply [flat|nested] 32+ messages in thread
* [dpdk-dev] [PATCH v2 08/15] service: fix and refactor atomic service accesses
2017-08-21 12:58 ` [dpdk-dev] [PATCH v2 00/15] service: API improvements and updates Harry van Haaren
` (6 preceding siblings ...)
2017-08-21 12:58 ` [dpdk-dev] [PATCH v2 07/15] service: rework get by name function to use id Harry van Haaren
@ 2017-08-21 12:58 ` Harry van Haaren
2017-08-21 12:58 ` [dpdk-dev] [PATCH v2 09/15] service: fix loops to always scan all services Harry van Haaren
` (7 subsequent siblings)
15 siblings, 0 replies; 32+ messages in thread
From: Harry van Haaren @ 2017-08-21 12:58 UTC (permalink / raw)
To: dev; +Cc: Harry van Haaren
This commit fixes an issue in the service runner function,
where the atomic value was not cleared on exiting the service
function. This resulted in future attempts to run the service
to appear like the function was running, however it was in
reality deadlocked.
This commit refactors the atomic handling to be more readable,
by splitting the implementation code into a new static inline
function. The remaining flow control of atomics in the existing
function is refactored for readability.
Fixes: 21698354c832 ("service: introduce service cores concept")
Signed-off-by: Harry van Haaren <harry.van.haaren@intel.com>
---
doc/guides/rel_notes/release_17_11.rst | 7 ++++++
lib/librte_eal/common/rte_service.c | 45 ++++++++++++++++++++--------------
2 files changed, 33 insertions(+), 19 deletions(-)
diff --git a/doc/guides/rel_notes/release_17_11.rst b/doc/guides/rel_notes/release_17_11.rst
index 170f4f9..6e6ba1c 100644
--- a/doc/guides/rel_notes/release_17_11.rst
+++ b/doc/guides/rel_notes/release_17_11.rst
@@ -65,6 +65,13 @@ Resolved Issues
EAL
~~~
+* **Service core fails to call service callback due to atomic lock**
+
+ In a specific configuration of multi-thread unsafe services and service
+ cores, a service core previously did not correctly release the atomic lock
+ on the service. This would result in the cores polling the service, but it
+ looked like another thread was executing the service callback. The logic for
+ atomic locking of the services has been fixed and refactored for readability.
Drivers
~~~~~~~
diff --git a/lib/librte_eal/common/rte_service.c b/lib/librte_eal/common/rte_service.c
index 1bfd149..6291c0c 100644
--- a/lib/librte_eal/common/rte_service.c
+++ b/lib/librte_eal/common/rte_service.c
@@ -296,6 +296,23 @@ rte_service_runstate_get(uint32_t id)
return (s->runstate == RUNSTATE_RUNNING) && (s->num_mapped_cores > 0);
}
+static inline void
+rte_service_runner_do_callback(struct rte_service_spec_impl *s,
+ struct core_state *cs, uint32_t service_idx)
+{
+ void *userdata = s->spec.callback_userdata;
+
+ if (service_stats_enabled(s)) {
+ uint64_t start = rte_rdtsc();
+ s->spec.callback(userdata);
+ uint64_t end = rte_rdtsc();
+ s->cycles_spent += end - start;
+ cs->calls_per_service[service_idx]++;
+ s->calls++;
+ } else
+ s->spec.callback(userdata);
+}
+
static int32_t
rte_service_runner_func(void *arg)
{
@@ -315,26 +332,16 @@ rte_service_runner_func(void *arg)
/* check do we need cmpset, if MT safe or <= 1 core
* mapped, atomic ops are not required.
*/
- const int need_cmpset = !((service_mt_safe(s) == 0) &&
- (s->num_mapped_cores > 1));
- uint32_t *lock = (uint32_t *)&s->execute_lock;
-
- if (need_cmpset || rte_atomic32_cmpset(lock, 0, 1)) {
- void *userdata = s->spec.callback_userdata;
-
- if (service_stats_enabled(s)) {
- 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
- s->spec.callback(userdata);
-
- if (need_cmpset)
+ 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);
}
rte_smp_rmb();
--
2.7.4
^ permalink raw reply [flat|nested] 32+ messages in thread
* [dpdk-dev] [PATCH v2 09/15] service: fix loops to always scan all services
2017-08-21 12:58 ` [dpdk-dev] [PATCH v2 00/15] service: API improvements and updates Harry van Haaren
` (7 preceding siblings ...)
2017-08-21 12:58 ` [dpdk-dev] [PATCH v2 08/15] service: fix and refactor atomic service accesses Harry van Haaren
@ 2017-08-21 12:58 ` Harry van Haaren
2017-08-21 12:58 ` [dpdk-dev] [PATCH v2 10/15] service: fix return values of functions to 0 or 1 Harry van Haaren
` (6 subsequent siblings)
15 siblings, 0 replies; 32+ messages in thread
From: Harry van Haaren @ 2017-08-21 12:58 UTC (permalink / raw)
To: dev; +Cc: Harry van Haaren
Services can be registered and unregistered, and "holes" can
appear in the contiguous array of services if a service is
unregistered. As a result, we must never iterate to the
number of services (as counted by rte_service_count), instead
scanning the service array and checking if the service is valid.
After this commit, the rte_service_count variable is only used
for its intended purpose; tracking the number of services that
are present.
Fixes: 21698354c832 ("service: introduce service cores concept")
Signed-off-by: Harry van Haaren <harry.van.haaren@intel.com>
---
lib/librte_eal/common/rte_service.c | 12 +++++++++---
1 file changed, 9 insertions(+), 3 deletions(-)
diff --git a/lib/librte_eal/common/rte_service.c b/lib/librte_eal/common/rte_service.c
index 6291c0c..a5fbcf9 100644
--- a/lib/librte_eal/common/rte_service.c
+++ b/lib/librte_eal/common/rte_service.c
@@ -323,7 +323,10 @@ rte_service_runner_func(void *arg)
while (lcore_states[lcore].runstate == RUNSTATE_RUNNING) {
const uint64_t service_mask = cs->service_mask;
- for (i = 0; i < rte_service_count; i++) {
+
+ 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->runstate != RUNSTATE_RUNNING ||
!(service_mask & (UINT64_C(1) << i)))
@@ -655,7 +658,8 @@ int32_t rte_service_dump(FILE *f, uint32_t id)
int print_one = (id != UINT32_MAX);
uint64_t total_cycles = 0;
- for (i = 0; i < rte_service_count; i++) {
+
+ for (i = 0; i < RTE_SERVICE_NUM_MAX; i++) {
if (!service_valid(i))
continue;
total_cycles += rte_services[i].cycles_spent;
@@ -673,7 +677,9 @@ int32_t rte_service_dump(FILE *f, uint32_t id)
/* print all services, as UINT32_MAX was passed as id */
fprintf(f, "Services Summary\n");
- for (i = 0; i < rte_service_count; i++) {
+ for (i = 0; i < RTE_SERVICE_NUM_MAX; i++) {
+ if (!service_valid(i))
+ continue;
uint32_t reset = 1;
rte_service_dump_one(f, &rte_services[i], total_cycles, reset);
}
--
2.7.4
^ permalink raw reply [flat|nested] 32+ messages in thread
* [dpdk-dev] [PATCH v2 10/15] service: fix return values of functions to 0 or 1
2017-08-21 12:58 ` [dpdk-dev] [PATCH v2 00/15] service: API improvements and updates Harry van Haaren
` (8 preceding siblings ...)
2017-08-21 12:58 ` [dpdk-dev] [PATCH v2 09/15] service: fix loops to always scan all services Harry van Haaren
@ 2017-08-21 12:58 ` Harry van Haaren
2017-08-21 12:58 ` [dpdk-dev] [PATCH v2 11/15] service: fix lcore in wait state in lcore add Harry van Haaren
` (5 subsequent siblings)
15 siblings, 0 replies; 32+ messages in thread
From: Harry van Haaren @ 2017-08-21 12:58 UTC (permalink / raw)
To: dev; +Cc: Harry van Haaren
Previously to this commit, the return value of the following
functions was the mask value, instead of a neat 0 or 1.
This commit fixes this using the "!!" trick, to force the
number to 1 instead of the bitmask value itself, bringing
the return value in line with the function documentation.
Fixes: 21698354c832 ("service: introduce service cores concept")
Signed-off-by: Harry van Haaren <harry.van.haaren@intel.com>
---
lib/librte_eal/common/rte_service.c | 6 +++---
1 file changed, 3 insertions(+), 3 deletions(-)
diff --git a/lib/librte_eal/common/rte_service.c b/lib/librte_eal/common/rte_service.c
index a5fbcf9..4034130 100644
--- a/lib/librte_eal/common/rte_service.c
+++ b/lib/librte_eal/common/rte_service.c
@@ -163,7 +163,7 @@ service_stats_enabled(struct rte_service_spec_impl *impl)
static inline int
service_mt_safe(struct rte_service_spec_impl *s)
{
- return s->spec.capabilities & RTE_SERVICE_CAP_MT_SAFE;
+ return !!(s->spec.capabilities & RTE_SERVICE_CAP_MT_SAFE);
}
int32_t rte_service_set_stats_enable(uint32_t id, int32_t enabled)
@@ -215,7 +215,7 @@ rte_service_probe_capability(uint32_t id, uint32_t capability)
{
struct rte_service_spec_impl *s;
SERVICE_VALID_GET_OR_ERR_RET(id, s, -EINVAL);
- return s->spec.capabilities & capability;
+ return !!(s->spec.capabilities & capability);
}
int32_t
@@ -463,7 +463,7 @@ service_update(struct rte_service_spec *service, uint32_t lcore,
}
if (enabled)
- *enabled = (lcore_states[lcore].service_mask & (sid_mask));
+ *enabled = !!(lcore_states[lcore].service_mask & (sid_mask));
rte_smp_wmb();
--
2.7.4
^ permalink raw reply [flat|nested] 32+ messages in thread
* [dpdk-dev] [PATCH v2 11/15] service: fix lcore in wait state in lcore add
2017-08-21 12:58 ` [dpdk-dev] [PATCH v2 00/15] service: API improvements and updates Harry van Haaren
` (9 preceding siblings ...)
2017-08-21 12:58 ` [dpdk-dev] [PATCH v2 10/15] service: fix return values of functions to 0 or 1 Harry van Haaren
@ 2017-08-21 12:58 ` Harry van Haaren
2017-08-21 12:58 ` [dpdk-dev] [PATCH v2 12/15] service: reset core call stats on dump Harry van Haaren
` (4 subsequent siblings)
15 siblings, 0 replies; 32+ messages in thread
From: Harry van Haaren @ 2017-08-21 12:58 UTC (permalink / raw)
To: dev; +Cc: Harry van Haaren
This commit ensures that after an lcore is added, that
it is in the WAIT state. Previously, adding an lcore did
not ensure that the core was ready for being relaunch, which
would cause errors during lcore_start(). Now that the lcore is
ensured to be in WAIT state by the lcore_add() function, this
is no longer an issue.
Fixes: 21698354c832 ("service: introduce service cores concept")
Signed-off-by: Harry van Haaren <harry.van.haaren@intel.com>
---
lib/librte_eal/common/rte_service.c | 3 ++-
1 file changed, 2 insertions(+), 1 deletion(-)
diff --git a/lib/librte_eal/common/rte_service.c b/lib/librte_eal/common/rte_service.c
index 4034130..789f63b 100644
--- a/lib/librte_eal/common/rte_service.c
+++ b/lib/librte_eal/common/rte_service.c
@@ -537,7 +537,8 @@ rte_service_lcore_add(uint32_t lcore)
lcore_states[lcore].runstate = RUNSTATE_STOPPED;
rte_smp_wmb();
- return 0;
+
+ return rte_eal_wait_lcore(lcore);
}
int32_t
--
2.7.4
^ permalink raw reply [flat|nested] 32+ messages in thread
* [dpdk-dev] [PATCH v2 12/15] service: reset core call stats on dump
2017-08-21 12:58 ` [dpdk-dev] [PATCH v2 00/15] service: API improvements and updates Harry van Haaren
` (10 preceding siblings ...)
2017-08-21 12:58 ` [dpdk-dev] [PATCH v2 11/15] service: fix lcore in wait state in lcore add Harry van Haaren
@ 2017-08-21 12:58 ` Harry van Haaren
2017-08-21 12:58 ` [dpdk-dev] [PATCH v2 13/15] service: add component runstate Harry van Haaren
` (3 subsequent siblings)
15 siblings, 0 replies; 32+ messages in thread
From: Harry van Haaren @ 2017-08-21 12:58 UTC (permalink / raw)
To: dev; +Cc: Harry van Haaren
This aligns with the service stats, which are currently
also reset on read. A generic statistics API would be
helpful for the service library in future.
Signed-off-by: Harry van Haaren <harry.van.haaren@intel.com>
---
lib/librte_eal/common/rte_service.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/lib/librte_eal/common/rte_service.c b/lib/librte_eal/common/rte_service.c
index 789f63b..58b53b1 100644
--- a/lib/librte_eal/common/rte_service.c
+++ b/lib/librte_eal/common/rte_service.c
@@ -690,7 +690,7 @@ int32_t rte_service_dump(FILE *f, uint32_t id)
if (lcore_config[i].core_role != ROLE_SERVICE)
continue;
- uint32_t reset = 0;
+ uint32_t reset = 1;
service_dump_calls_per_lcore(f, i, reset);
}
--
2.7.4
^ permalink raw reply [flat|nested] 32+ messages in thread
* [dpdk-dev] [PATCH v2 13/15] service: add component runstate
2017-08-21 12:58 ` [dpdk-dev] [PATCH v2 00/15] service: API improvements and updates Harry van Haaren
` (11 preceding siblings ...)
2017-08-21 12:58 ` [dpdk-dev] [PATCH v2 12/15] service: reset core call stats on dump Harry van Haaren
@ 2017-08-21 12:58 ` Harry van Haaren
2017-08-21 12:58 ` [dpdk-dev] [PATCH v2 14/15] service: clarify documentation for register Harry van Haaren
` (2 subsequent siblings)
15 siblings, 0 replies; 32+ messages in thread
From: Harry van Haaren @ 2017-08-21 12:58 UTC (permalink / raw)
To: dev; +Cc: Harry van Haaren
This commit adds a new flag that the component (or "backend")
can use to indicate readyness. The service function callback
will not be called until the component sets itself as ready.
The use-case behind adding this feature is eg: a service that
requires configuration before it can start. Any service that
emulates an ethdev will have rte_eth_dev_configure() called,
and only after that the service will know how many queues/etc
to allocate. Once that configuration is complete, the service
marks itself as ready using rte_service_component_runstate_set().
This feature request results from prototyping services, and
requiring a flag in each service to note "internal" readyness.
Instead that logic is now lifted to the service library.
The unit tests have been updated to test the component runstate.
Signed-off-by: Harry van Haaren <harry.van.haaren@intel.com>
---
lib/librte_eal/bsdapp/eal/rte_eal_version.map | 1 +
.../common/include/rte_service_component.h | 17 +++++++++++
lib/librte_eal/common/rte_service.c | 34 +++++++++++++++++-----
lib/librte_eal/linuxapp/eal/rte_eal_version.map | 1 +
test/test/test_service_cores.c | 32 +++++++++++++++-----
5 files changed, 70 insertions(+), 15 deletions(-)
diff --git a/lib/librte_eal/bsdapp/eal/rte_eal_version.map b/lib/librte_eal/bsdapp/eal/rte_eal_version.map
index 4b2c36b..05a3de1 100644
--- a/lib/librte_eal/bsdapp/eal/rte_eal_version.map
+++ b/lib/librte_eal/bsdapp/eal/rte_eal_version.map
@@ -214,6 +214,7 @@ EXPERIMENTAL {
rte_eal_hotplug_remove;
rte_service_component_register;
rte_service_component_unregister;
+ rte_service_component_runstate_set;
rte_service_dump;
rte_service_get_by_id;
rte_service_get_by_name;
diff --git a/lib/librte_eal/common/include/rte_service_component.h b/lib/librte_eal/common/include/rte_service_component.h
index af632c6..5e4573b 100644
--- a/lib/librte_eal/common/include/rte_service_component.h
+++ b/lib/librte_eal/common/include/rte_service_component.h
@@ -135,6 +135,23 @@ int32_t rte_service_start_with_defaults(void);
* @warning
* @b EXPERIMENTAL: this API may change without prior notice
*
+ * Set the backend runstate of a component.
+ *
+ * This function allows services to be registered at startup, but not yet
+ * enabled to run by default. When the service has been configured (via the
+ * usual method; eg rte_eventdev_configure, the service can mark itself as
+ * ready to run. The differentiation between backend runstate and
+ * service_runstate is that the backend runstate is set by the service
+ * component while the service runstate is reserved for application usage.
+ *
+ * @retval 0 Success
+ */
+int32_t rte_service_component_runstate_set(uint32_t id, uint32_t runstate);
+
+/**
+ * @warning
+ * @b EXPERIMENTAL: this API may change without prior notice
+ *
* Initialize the service library.
*
* In order to use the service library, it must be initialized. EAL initializes
diff --git a/lib/librte_eal/common/rte_service.c b/lib/librte_eal/common/rte_service.c
index 58b53b1..e7ed597 100644
--- a/lib/librte_eal/common/rte_service.c
+++ b/lib/librte_eal/common/rte_service.c
@@ -71,7 +71,8 @@ struct rte_service_spec_impl {
rte_atomic32_t execute_lock;
/* API set/get-able variables */
- int32_t runstate;
+ int8_t app_runstate;
+ int8_t comp_runstate;
uint8_t internal_flags;
/* per service statistics */
@@ -273,15 +274,30 @@ rte_service_component_unregister(uint32_t id)
}
int32_t
+rte_service_component_runstate_set(uint32_t id, uint32_t runstate)
+{
+ struct rte_service_spec_impl *s;
+ SERVICE_VALID_GET_OR_ERR_RET(id, s, -EINVAL);
+
+ if (runstate)
+ s->comp_runstate = RUNSTATE_RUNNING;
+ else
+ s->comp_runstate = RUNSTATE_STOPPED;
+
+ rte_smp_wmb();
+ return 0;
+}
+
+int32_t
rte_service_runstate_set(uint32_t id, uint32_t runstate)
{
struct rte_service_spec_impl *s;
SERVICE_VALID_GET_OR_ERR_RET(id, s, -EINVAL);
if (runstate)
- s->runstate = RUNSTATE_RUNNING;
+ s->app_runstate = RUNSTATE_RUNNING;
else
- s->runstate = RUNSTATE_STOPPED;
+ s->app_runstate = RUNSTATE_STOPPED;
rte_smp_wmb();
return 0;
@@ -292,8 +308,10 @@ rte_service_runstate_get(uint32_t id)
{
struct rte_service_spec_impl *s;
SERVICE_VALID_GET_OR_ERR_RET(id, s, -EINVAL);
-
- return (s->runstate == RUNSTATE_RUNNING) && (s->num_mapped_cores > 0);
+ rte_smp_rmb();
+ return (s->app_runstate == RUNSTATE_RUNNING) &&
+ (s->comp_runstate == RUNSTATE_RUNNING) &&
+ (s->num_mapped_cores > 0);
}
static inline void
@@ -328,7 +346,8 @@ rte_service_runner_func(void *arg)
if (!service_valid(i))
continue;
struct rte_service_spec_impl *s = &rte_services[i];
- if (s->runstate != RUNSTATE_RUNNING ||
+ if (s->comp_runstate != RUNSTATE_RUNNING ||
+ s->app_runstate != RUNSTATE_RUNNING ||
!(service_mask & (UINT64_C(1) << i)))
continue;
@@ -596,8 +615,7 @@ rte_service_lcore_stop(uint32_t lcore)
for (i = 0; i < RTE_SERVICE_NUM_MAX; i++) {
int32_t enabled =
lcore_states[i].service_mask & (UINT64_C(1) << i);
- int32_t service_running = rte_services[i].runstate !=
- RUNSTATE_STOPPED;
+ int32_t service_running = rte_service_runstate_get(i);
int32_t only_core = rte_services[i].num_mapped_cores == 1;
/* if the core is mapped, and the service is running, and this
diff --git a/lib/librte_eal/linuxapp/eal/rte_eal_version.map b/lib/librte_eal/linuxapp/eal/rte_eal_version.map
index f7a7352..0e1d09b 100644
--- a/lib/librte_eal/linuxapp/eal/rte_eal_version.map
+++ b/lib/librte_eal/linuxapp/eal/rte_eal_version.map
@@ -219,6 +219,7 @@ EXPERIMENTAL {
rte_eal_hotplug_remove;
rte_service_component_register;
rte_service_component_unregister;
+ rte_service_component_runstate_set;
rte_service_dump;
rte_service_get_by_id;
rte_service_get_by_name;
diff --git a/test/test/test_service_cores.c b/test/test/test_service_cores.c
index 5ae7b20..ddea7f0 100644
--- a/test/test/test_service_cores.c
+++ b/test/test/test_service_cores.c
@@ -166,9 +166,12 @@ dummy_register(void)
"Invalid name");
snprintf(service.name, sizeof(service.name), DUMMY_SERVICE_NAME);
- TEST_ASSERT_EQUAL(0, rte_service_component_register(&service, NULL),
+ uint32_t id;
+ TEST_ASSERT_EQUAL(0, rte_service_component_register(&service, &id),
"Failed to register valid service");
+ rte_service_component_runstate_set(id, 1);
+
return TEST_SUCCESS;
}
@@ -470,13 +473,11 @@ service_threaded_test(int mt_safe)
if (mt_safe) {
service.callback = dummy_mt_safe_cb;
service.capabilities |= RTE_SERVICE_CAP_MT_SAFE;
- } else {
- /* initialize to pass, see callback comment for details */
- test_params[1] = 1;
+ } else
service.callback = dummy_mt_unsafe_cb;
- }
- TEST_ASSERT_EQUAL(0, rte_service_component_register(&service, NULL),
+ uint32_t id;
+ TEST_ASSERT_EQUAL(0, rte_service_component_register(&service, &id),
"Register of MT SAFE service failed");
const uint32_t sid = 0;
@@ -494,9 +495,26 @@ service_threaded_test(int mt_safe)
rte_service_lcore_stop(slcore_1);
rte_service_lcore_stop(slcore_2);
+ TEST_ASSERT_EQUAL(0, test_params[1],
+ "Service run with component runstate = 0");
+
+ /* enable backend runstate: the service should run after this */
+ rte_service_component_runstate_set(id, 1);
+
+ /* initialize to pass, see callback comment for details */
+ if (!mt_safe)
+ test_params[1] = 1;
+
+ rte_service_lcore_start(slcore_1);
+ rte_service_lcore_start(slcore_2);
+
+ /* wait for the worker threads to run */
+ rte_delay_ms(500);
+ rte_service_lcore_stop(slcore_1);
+ rte_service_lcore_stop(slcore_2);
+
TEST_ASSERT_EQUAL(1, test_params[1],
"MT Safe service not run by two cores concurrently");
-
TEST_ASSERT_EQUAL(0, rte_service_runstate_set(sid, 0),
"Failed to stop MT Safe service");
--
2.7.4
^ permalink raw reply [flat|nested] 32+ messages in thread
* [dpdk-dev] [PATCH v2 14/15] service: clarify documentation for register
2017-08-21 12:58 ` [dpdk-dev] [PATCH v2 00/15] service: API improvements and updates Harry van Haaren
` (12 preceding siblings ...)
2017-08-21 12:58 ` [dpdk-dev] [PATCH v2 13/15] service: add component runstate Harry van Haaren
@ 2017-08-21 12:58 ` Harry van Haaren
2017-08-21 12:58 ` [dpdk-dev] [PATCH v2 15/15] docs: add notes on service cores API updates Harry van Haaren
2017-09-15 11:51 ` [dpdk-dev] [PATCH v2 00/15] service: API improvements and updates Thomas Monjalon
15 siblings, 0 replies; 32+ messages in thread
From: Harry van Haaren @ 2017-08-21 12:58 UTC (permalink / raw)
To: dev; +Cc: Harry van Haaren
This commit adds a section to the service register function
to make it clear that registering a service, must not configure
service-cores (eg: adding lcores or changing mappings).
Signed-off-by: Harry van Haaren <harry.van.haaren@intel.com>
---
lib/librte_eal/common/include/rte_service_component.h | 9 +++++++--
1 file changed, 7 insertions(+), 2 deletions(-)
diff --git a/lib/librte_eal/common/include/rte_service_component.h b/lib/librte_eal/common/include/rte_service_component.h
index 5e4573b..ac965cb 100644
--- a/lib/librte_eal/common/include/rte_service_component.h
+++ b/lib/librte_eal/common/include/rte_service_component.h
@@ -85,8 +85,13 @@ struct rte_service_spec {
*
* For example the eventdev SW PMD requires CPU cycles to perform its
* scheduling. This can be achieved by registering it as a service, and the
- * application can then assign CPU resources to it using
- * *rte_service_set_coremask*.
+ * application can then assign CPU resources to that service.
+ *
+ * Note that when a service component registers itself, it is not permitted to
+ * add or remove service-core threads, or modify lcore-to-service mappings. The
+ * only API that may be called by the service-component is
+ * *rte_service_component_runstate_set*, which indicates that the service
+ * component is ready to be executed.
*
* @param spec The specification of the service to register
* @param[out] service_id A pointer to a uint32_t, which will be filled in
--
2.7.4
^ permalink raw reply [flat|nested] 32+ messages in thread
* [dpdk-dev] [PATCH v2 15/15] docs: add notes on service cores API updates
2017-08-21 12:58 ` [dpdk-dev] [PATCH v2 00/15] service: API improvements and updates Harry van Haaren
` (13 preceding siblings ...)
2017-08-21 12:58 ` [dpdk-dev] [PATCH v2 14/15] service: clarify documentation for register Harry van Haaren
@ 2017-08-21 12:58 ` Harry van Haaren
2017-09-15 11:51 ` [dpdk-dev] [PATCH v2 00/15] service: API improvements and updates Thomas Monjalon
15 siblings, 0 replies; 32+ messages in thread
From: Harry van Haaren @ 2017-08-21 12:58 UTC (permalink / raw)
To: dev; +Cc: Harry van Haaren
Add a section on the service cores API changes to 17.11 release notes.
Suggested-by: Neil Horman <nhorman@tuxdriver.com>
Signed-off-by: Harry van Haaren <harry.van.haaren@intel.com>
---
doc/guides/rel_notes/release_17_11.rst | 13 +++++++++++++
1 file changed, 13 insertions(+)
diff --git a/doc/guides/rel_notes/release_17_11.rst b/doc/guides/rel_notes/release_17_11.rst
index 6e6ba1c..8bf91bd 100644
--- a/doc/guides/rel_notes/release_17_11.rst
+++ b/doc/guides/rel_notes/release_17_11.rst
@@ -117,6 +117,19 @@ API Changes
Also, make sure to start the actual text at the margin.
=========================================================
+* **Service cores API updated for usability**
+
+ The service cores API has been changed, removing pointers from the API
+ where possible, instead using integer IDs to identify each service. This
+ simplifed application code, aids debugging, and provides better
+ encapsulation. A summary of the main changes made is as follows:
+
+ * Services identified by ID not by ``rte_service_spec`` pointer
+ * Reduced API surface by using ``set`` functions instead of enable/disable
+ * Reworked ``rte_service_register`` to provide the service ID to registrar
+ * Rework start and stop APIs into ``rte_service_runstate_set``
+ * Added API to set runstate of service implementation to indicate readyness
+
ABI Changes
-----------
--
2.7.4
^ permalink raw reply [flat|nested] 32+ messages in thread
* Re: [dpdk-dev] [PATCH v2 01/15] service: rework probe and get name to use ids
2017-08-21 12:58 ` [dpdk-dev] [PATCH v2 01/15] service: rework probe and get name to use ids Harry van Haaren
@ 2017-08-30 19:25 ` Pavan Nikhilesh Bhagavatula
2017-09-04 14:32 ` Pavan Nikhilesh Bhagavatula
1 sibling, 0 replies; 32+ messages in thread
From: Pavan Nikhilesh Bhagavatula @ 2017-08-30 19:25 UTC (permalink / raw)
To: Harry van Haaren; +Cc: dev
On Mon, Aug 21, 2017 at 01:58:02PM +0100, Harry van Haaren wrote:
> This commit adds a macro to easily validate a service ID, and then
> lookup the service pointer, or return a user-specified error code.
> This marco will be heavily used in the following patches as it will
> be ID based instead of pointer-based.
>
> The probe_capability function is reworked to use an integer ID instead
> of a pointer. Rework the service_get_name() function is updated to use
> IDs. Unit tests are updated to keep things compiling after each commit.
>
> Signed-off-by: Harry van Haaren <harry.van.haaren@intel.com>
Acked-by: Pavan Nikhilesh <pbhagavatula@caviumnetworks.com>
^ permalink raw reply [flat|nested] 32+ messages in thread
* Re: [dpdk-dev] [PATCH v2 01/15] service: rework probe and get name to use ids
2017-08-21 12:58 ` [dpdk-dev] [PATCH v2 01/15] service: rework probe and get name to use ids Harry van Haaren
2017-08-30 19:25 ` Pavan Nikhilesh Bhagavatula
@ 2017-09-04 14:32 ` Pavan Nikhilesh Bhagavatula
1 sibling, 0 replies; 32+ messages in thread
From: Pavan Nikhilesh Bhagavatula @ 2017-09-04 14:32 UTC (permalink / raw)
To: Harry van Haaren; +Cc: dev
On Mon, Aug 21, 2017 at 01:58:02PM +0100, Harry van Haaren wrote:
> This commit adds a macro to easily validate a service ID, and then
> lookup the service pointer, or return a user-specified error code.
> This marco will be heavily used in the following patches as it will
> be ID based instead of pointer-based.
>
> The probe_capability function is reworked to use an integer ID instead
> of a pointer. Rework the service_get_name() function is updated to use
> IDs. Unit tests are updated to keep things compiling after each commit.
>
> Signed-off-by: Harry van Haaren <harry.van.haaren@intel.com>
Series-Acked-by: Pavan Nikhilesh <pbhagavatula@caviumnetworks.com>
^ permalink raw reply [flat|nested] 32+ messages in thread
* Re: [dpdk-dev] [PATCH v2 00/15] service: API improvements and updates
2017-08-21 12:58 ` [dpdk-dev] [PATCH v2 00/15] service: API improvements and updates Harry van Haaren
` (14 preceding siblings ...)
2017-08-21 12:58 ` [dpdk-dev] [PATCH v2 15/15] docs: add notes on service cores API updates Harry van Haaren
@ 2017-09-15 11:51 ` Thomas Monjalon
15 siblings, 0 replies; 32+ messages in thread
From: Thomas Monjalon @ 2017-09-15 11:51 UTC (permalink / raw)
To: Harry van Haaren; +Cc: dev
> Harry van Haaren (15):
> service: rework probe and get name to use ids
> service: rework lcore to service map functions
> service: rework register to return service id
> service: rework service start stop to runstate
> service: rework service stats functions
> service: rework unregister api to use integers
> service: rework get by name function to use id
> service: fix and refactor atomic service accesses
> service: fix loops to always scan all services
> service: fix return values of functions to 0 or 1
> service: fix lcore in wait state in lcore add
> service: reset core call stats on dump
> service: add component runstate
> service: clarify documentation for register
> docs: add notes on service cores API updates
Applied, thanks for the rewording on IRC
^ permalink raw reply [flat|nested] 32+ messages in thread
end of thread, other threads:[~2017-09-15 11:51 UTC | newest]
Thread overview: 32+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2017-08-15 12:32 [dpdk-dev] [PATCH 0/8] service: rework for usability Harry van Haaren
2017-08-15 12:32 ` [dpdk-dev] [PATCH 1/8] service: rework probe and get name to use ids Harry van Haaren
2017-08-15 12:32 ` [dpdk-dev] [PATCH 2/8] service: rework lcore to service map functions Harry van Haaren
2017-08-15 12:32 ` [dpdk-dev] [PATCH 3/8] service: rework register to return service id Harry van Haaren
2017-08-15 12:32 ` [dpdk-dev] [PATCH 4/8] service: rework service start stop to runstate Harry van Haaren
2017-08-15 12:32 ` [dpdk-dev] [PATCH 5/8] service: rework service stats functions Harry van Haaren
2017-08-15 12:32 ` [dpdk-dev] [PATCH 6/8] service: rework unregister api to use integers Harry van Haaren
2017-08-15 12:32 ` [dpdk-dev] [PATCH 7/8] service: rework get by name function to use id Harry van Haaren
2017-08-15 12:32 ` [dpdk-dev] [PATCH 8/8] service: clarify documentation for register Harry van Haaren
2017-08-16 11:16 ` [dpdk-dev] [PATCH 0/8] service: rework for usability Neil Horman
2017-08-16 11:31 ` Van Haaren, Harry
2017-08-16 12:07 ` Van Haaren, Harry
2017-08-16 13:27 ` Neil Horman
2017-08-21 12:58 ` [dpdk-dev] [PATCH v2 00/15] service: API improvements and updates Harry van Haaren
2017-08-21 12:58 ` [dpdk-dev] [PATCH v2 01/15] service: rework probe and get name to use ids Harry van Haaren
2017-08-30 19:25 ` Pavan Nikhilesh Bhagavatula
2017-09-04 14:32 ` Pavan Nikhilesh Bhagavatula
2017-08-21 12:58 ` [dpdk-dev] [PATCH v2 02/15] service: rework lcore to service map functions Harry van Haaren
2017-08-21 12:58 ` [dpdk-dev] [PATCH v2 03/15] service: rework register to return service id Harry van Haaren
2017-08-21 12:58 ` [dpdk-dev] [PATCH v2 04/15] service: rework service start stop to runstate Harry van Haaren
2017-08-21 12:58 ` [dpdk-dev] [PATCH v2 05/15] service: rework service stats functions Harry van Haaren
2017-08-21 12:58 ` [dpdk-dev] [PATCH v2 06/15] service: rework unregister api to use integers Harry van Haaren
2017-08-21 12:58 ` [dpdk-dev] [PATCH v2 07/15] service: rework get by name function to use id Harry van Haaren
2017-08-21 12:58 ` [dpdk-dev] [PATCH v2 08/15] service: fix and refactor atomic service accesses Harry van Haaren
2017-08-21 12:58 ` [dpdk-dev] [PATCH v2 09/15] service: fix loops to always scan all services Harry van Haaren
2017-08-21 12:58 ` [dpdk-dev] [PATCH v2 10/15] service: fix return values of functions to 0 or 1 Harry van Haaren
2017-08-21 12:58 ` [dpdk-dev] [PATCH v2 11/15] service: fix lcore in wait state in lcore add Harry van Haaren
2017-08-21 12:58 ` [dpdk-dev] [PATCH v2 12/15] service: reset core call stats on dump Harry van Haaren
2017-08-21 12:58 ` [dpdk-dev] [PATCH v2 13/15] service: add component runstate Harry van Haaren
2017-08-21 12:58 ` [dpdk-dev] [PATCH v2 14/15] service: clarify documentation for register Harry van Haaren
2017-08-21 12:58 ` [dpdk-dev] [PATCH v2 15/15] docs: add notes on service cores API updates Harry van Haaren
2017-09-15 11:51 ` [dpdk-dev] [PATCH v2 00/15] service: API improvements and updates Thomas Monjalon
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).