DPDK patches and discussions
 help / color / mirror / Atom feed
* [dpdk-dev] [PATCH 1/2] service: add component useful work attribute
@ 2020-09-07 16:21 Harry van Haaren
  2020-09-07 16:21 ` [dpdk-dev] [PATCH 2/2] event/sw: add useful work done attribute Harry van Haaren
                   ` (2 more replies)
  0 siblings, 3 replies; 16+ messages in thread
From: Harry van Haaren @ 2020-09-07 16:21 UTC (permalink / raw)
  To: dev; +Cc: Harry van Haaren

This commit adds a new attribute which allows the service to indicate
if the previous iteration of work was "useful". Useful work here implies
forward progress was made.

Exposing this information via an attribute to the application allows
tracking of CPU cycles as being useful or not-useful, and a CPU load
estimate can be deduced from that information.

Signed-off-by: Harry van Haaren <harry.van.haaren@intel.com>
---
 lib/librte_eal/common/rte_service.c           | 19 +++++++++++++++++++
 lib/librte_eal/include/rte_service.h          |  5 +++++
 .../include/rte_service_component.h           | 12 ++++++++++++
 lib/librte_eal/rte_eal_version.map            |  3 +++
 4 files changed, 39 insertions(+)

diff --git a/lib/librte_eal/common/rte_service.c b/lib/librte_eal/common/rte_service.c
index 6a0e0ff65d..f9792a138b 100644
--- a/lib/librte_eal/common/rte_service.c
+++ b/lib/librte_eal/common/rte_service.c
@@ -58,6 +58,7 @@ struct rte_service_spec_impl {
 	uint32_t num_mapped_cores;
 	uint64_t calls;
 	uint64_t cycles_spent;
+	uint8_t useful_work_last_iter;
 } __rte_cache_aligned;
 
 /* the internal values of a service core */
@@ -293,6 +294,21 @@ rte_service_component_unregister(uint32_t id)
 	return 0;
 }
 
+int32_t
+rte_service_component_attr_set(uint32_t id, uint32_t attr, uint64_t value)
+{
+	struct rte_service_spec_impl *s;
+	SERVICE_VALID_GET_OR_ERR_RET(id, s, -EINVAL);
+
+	switch (attr) {
+	case RTE_SERVICE_ATTR_USEFUL_WORK_LAST_ITER:
+		s->useful_work_last_iter = value;
+		return 0;
+	default:
+		return -EINVAL;
+	};
+}
+
 int32_t
 rte_service_component_runstate_set(uint32_t id, uint32_t runstate)
 {
@@ -778,6 +794,9 @@ rte_service_attr_get(uint32_t id, uint32_t attr_id, uint64_t *attr_value)
 		return -EINVAL;
 
 	switch (attr_id) {
+	case RTE_SERVICE_ATTR_USEFUL_WORK_LAST_ITER:
+		*attr_value = s->useful_work_last_iter;
+		return 0;
 	case RTE_SERVICE_ATTR_CYCLES:
 		*attr_value = s->cycles_spent;
 		return 0;
diff --git a/lib/librte_eal/include/rte_service.h b/lib/librte_eal/include/rte_service.h
index e2d0a6dd32..e9836a1a68 100644
--- a/lib/librte_eal/include/rte_service.h
+++ b/lib/librte_eal/include/rte_service.h
@@ -370,6 +370,11 @@ int32_t rte_service_dump(FILE *f, uint32_t id);
  */
 #define RTE_SERVICE_ATTR_CALL_COUNT 1
 
+/**
+ * Returns if the last iteration of the service resulted in useful work done.
+ */
+#define RTE_SERVICE_ATTR_USEFUL_WORK_LAST_ITER 2
+
 /**
  * Get an attribute from a service.
  *
diff --git a/lib/librte_eal/include/rte_service_component.h b/lib/librte_eal/include/rte_service_component.h
index 9e66ee7e29..2caa7981b5 100644
--- a/lib/librte_eal/include/rte_service_component.h
+++ b/lib/librte_eal/include/rte_service_component.h
@@ -87,6 +87,18 @@ int32_t rte_service_component_register(const struct rte_service_spec *spec,
  */
 int32_t rte_service_component_unregister(uint32_t id);
 
+/**
+ * Set an attribute for this service.
+ *
+ * Note this API is to be called by the service implementation, to make the
+ * statistic available via the usual attr_get() service APIs.
+ *
+ * @retval 0 Success
+ * @retval -EINVAL Invalid service id or attribute provided
+ */
+int32_t rte_service_component_attr_set(uint32_t id, uint32_t attr,
+		uint64_t value);
+
 /**
  * Private function to allow EAL to initialized default mappings.
  *
diff --git a/lib/librte_eal/rte_eal_version.map b/lib/librte_eal/rte_eal_version.map
index 0b18e2ef85..bb5e19ae9e 100644
--- a/lib/librte_eal/rte_eal_version.map
+++ b/lib/librte_eal/rte_eal_version.map
@@ -397,6 +397,9 @@ EXPERIMENTAL {
 	rte_mp_disable;
 	rte_thread_register;
 	rte_thread_unregister;
+
+	# added in 20.11
+	rte_service_component_attr_set;
 };
 
 INTERNAL {
-- 
2.17.1


^ permalink raw reply	[flat|nested] 16+ messages in thread

* [dpdk-dev] [PATCH 2/2] event/sw: add useful work done attribute
  2020-09-07 16:21 [dpdk-dev] [PATCH 1/2] service: add component useful work attribute Harry van Haaren
@ 2020-09-07 16:21 ` Harry van Haaren
  2020-09-14  8:37 ` [dpdk-dev] [PATCH 1/2] service: add component useful work attribute David Marchand
  2020-09-14 14:37 ` [dpdk-dev] [PATCH v2 " Harry van Haaren
  2 siblings, 0 replies; 16+ messages in thread
From: Harry van Haaren @ 2020-09-07 16:21 UTC (permalink / raw)
  To: dev; +Cc: Harry van Haaren, jerinj

This commit exposes if useful work is done to the service
instance. The normal service_attr_get() API can be used to
retrieve the value of the attribute.

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

---

Cc: jerinj@marvell.com
CC due to event/sw change, could be picked up by main tree as depends on 1/2?

 drivers/event/sw/sw_evdev_scheduler.c | 7 +++++++
 1 file changed, 7 insertions(+)

diff --git a/drivers/event/sw/sw_evdev_scheduler.c b/drivers/event/sw/sw_evdev_scheduler.c
index cff747da89..4e932612d5 100644
--- a/drivers/event/sw/sw_evdev_scheduler.c
+++ b/drivers/event/sw/sw_evdev_scheduler.c
@@ -5,6 +5,9 @@
 #include <rte_ring.h>
 #include <rte_hash_crc.h>
 #include <rte_event_ring.h>
+
+#include <rte_service_component.h>
+
 #include "sw_evdev.h"
 #include "iq_chunk.h"
 
@@ -554,6 +557,10 @@ sw_event_schedule(struct rte_eventdev *dev)
 	sw->sched_no_iq_enqueues += (in_pkts_total == 0);
 	sw->sched_no_cq_enqueues += (out_pkts_total == 0);
 
+	uint64_t work_done = (in_pkts_total + out_pkts_total) != 0;
+	rte_service_component_attr_set(sw->service_id,
+			RTE_SERVICE_ATTR_USEFUL_WORK_LAST_ITER, work_done);
+
 	/* push all the internal buffered QEs in port->cq_ring to the
 	 * worker cores: aka, do the ring transfers batched.
 	 */
-- 
2.17.1


^ permalink raw reply	[flat|nested] 16+ messages in thread

* Re: [dpdk-dev] [PATCH 1/2] service: add component useful work attribute
  2020-09-07 16:21 [dpdk-dev] [PATCH 1/2] service: add component useful work attribute Harry van Haaren
  2020-09-07 16:21 ` [dpdk-dev] [PATCH 2/2] event/sw: add useful work done attribute Harry van Haaren
@ 2020-09-14  8:37 ` David Marchand
  2020-09-14 14:37   ` Van Haaren, Harry
  2020-09-14 14:37 ` [dpdk-dev] [PATCH v2 " Harry van Haaren
  2 siblings, 1 reply; 16+ messages in thread
From: David Marchand @ 2020-09-14  8:37 UTC (permalink / raw)
  To: Harry van Haaren; +Cc: dev

On Mon, Sep 7, 2020 at 6:20 PM Harry van Haaren
<harry.van.haaren@intel.com> wrote:
> diff --git a/lib/librte_eal/include/rte_service_component.h b/lib/librte_eal/include/rte_service_component.h
> index 9e66ee7e29..2caa7981b5 100644
> --- a/lib/librte_eal/include/rte_service_component.h
> +++ b/lib/librte_eal/include/rte_service_component.h
> @@ -87,6 +87,18 @@ int32_t rte_service_component_register(const struct rte_service_spec *spec,
>   */
>  int32_t rte_service_component_unregister(uint32_t id);
>
> +/**
> + * Set an attribute for this service.
> + *
> + * Note this API is to be called by the service implementation, to make the
> + * statistic available via the usual attr_get() service APIs.
> + *
> + * @retval 0 Success
> + * @retval -EINVAL Invalid service id or attribute provided
> + */
> +int32_t rte_service_component_attr_set(uint32_t id, uint32_t attr,
> +               uint64_t value);
> +

The experimental tag is missing for this declaration.

https://travis-ci.com/ovsrobot/dpdk/builds/183185060
http://mails.dpdk.org/archives/test-report/2020-September/150152.html

>  /**
>   * Private function to allow EAL to initialized default mappings.
>   *
> diff --git a/lib/librte_eal/rte_eal_version.map b/lib/librte_eal/rte_eal_version.map
> index 0b18e2ef85..bb5e19ae9e 100644
> --- a/lib/librte_eal/rte_eal_version.map
> +++ b/lib/librte_eal/rte_eal_version.map
> @@ -397,6 +397,9 @@ EXPERIMENTAL {
>         rte_mp_disable;
>         rte_thread_register;
>         rte_thread_unregister;
> +
> +       # added in 20.11
> +       rte_service_component_attr_set;
>  };
>
>  INTERNAL {
> --
> 2.17.1
>


-- 
David Marchand


^ permalink raw reply	[flat|nested] 16+ messages in thread

* [dpdk-dev] [PATCH v2 1/2] service: add component useful work attribute
  2020-09-07 16:21 [dpdk-dev] [PATCH 1/2] service: add component useful work attribute Harry van Haaren
  2020-09-07 16:21 ` [dpdk-dev] [PATCH 2/2] event/sw: add useful work done attribute Harry van Haaren
  2020-09-14  8:37 ` [dpdk-dev] [PATCH 1/2] service: add component useful work attribute David Marchand
@ 2020-09-14 14:37 ` Harry van Haaren
  2020-09-14 14:37   ` [dpdk-dev] [PATCH v2 2/2] event/sw: add useful work done attribute Harry van Haaren
                     ` (2 more replies)
  2 siblings, 3 replies; 16+ messages in thread
From: Harry van Haaren @ 2020-09-14 14:37 UTC (permalink / raw)
  To: dev; +Cc: david.marchand, Harry van Haaren

This commit adds a new attribute which allows the service to indicate
if the previous iteration of work was "useful". Useful work here implies
forward progress was made.

Exposing this information via an attribute to the application allows
tracking of CPU cycles as being useful or not-useful, and a CPU load
estimate can be deduced from that information.

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

---

v2:
- Add experimental tag to new function.

---
 lib/librte_eal/common/rte_service.c           | 19 +++++++++++++++++++
 lib/librte_eal/include/rte_service.h          |  5 +++++
 .../include/rte_service_component.h           | 13 +++++++++++++
 lib/librte_eal/rte_eal_version.map            |  3 +++
 4 files changed, 40 insertions(+)

diff --git a/lib/librte_eal/common/rte_service.c b/lib/librte_eal/common/rte_service.c
index 6a0e0ff65d..f9792a138b 100644
--- a/lib/librte_eal/common/rte_service.c
+++ b/lib/librte_eal/common/rte_service.c
@@ -58,6 +58,7 @@ struct rte_service_spec_impl {
 	uint32_t num_mapped_cores;
 	uint64_t calls;
 	uint64_t cycles_spent;
+	uint8_t useful_work_last_iter;
 } __rte_cache_aligned;
 
 /* the internal values of a service core */
@@ -293,6 +294,21 @@ rte_service_component_unregister(uint32_t id)
 	return 0;
 }
 
+int32_t
+rte_service_component_attr_set(uint32_t id, uint32_t attr, uint64_t value)
+{
+	struct rte_service_spec_impl *s;
+	SERVICE_VALID_GET_OR_ERR_RET(id, s, -EINVAL);
+
+	switch (attr) {
+	case RTE_SERVICE_ATTR_USEFUL_WORK_LAST_ITER:
+		s->useful_work_last_iter = value;
+		return 0;
+	default:
+		return -EINVAL;
+	};
+}
+
 int32_t
 rte_service_component_runstate_set(uint32_t id, uint32_t runstate)
 {
@@ -778,6 +794,9 @@ rte_service_attr_get(uint32_t id, uint32_t attr_id, uint64_t *attr_value)
 		return -EINVAL;
 
 	switch (attr_id) {
+	case RTE_SERVICE_ATTR_USEFUL_WORK_LAST_ITER:
+		*attr_value = s->useful_work_last_iter;
+		return 0;
 	case RTE_SERVICE_ATTR_CYCLES:
 		*attr_value = s->cycles_spent;
 		return 0;
diff --git a/lib/librte_eal/include/rte_service.h b/lib/librte_eal/include/rte_service.h
index e2d0a6dd32..e9836a1a68 100644
--- a/lib/librte_eal/include/rte_service.h
+++ b/lib/librte_eal/include/rte_service.h
@@ -370,6 +370,11 @@ int32_t rte_service_dump(FILE *f, uint32_t id);
  */
 #define RTE_SERVICE_ATTR_CALL_COUNT 1
 
+/**
+ * Returns if the last iteration of the service resulted in useful work done.
+ */
+#define RTE_SERVICE_ATTR_USEFUL_WORK_LAST_ITER 2
+
 /**
  * Get an attribute from a service.
  *
diff --git a/lib/librte_eal/include/rte_service_component.h b/lib/librte_eal/include/rte_service_component.h
index 9e66ee7e29..534f41f531 100644
--- a/lib/librte_eal/include/rte_service_component.h
+++ b/lib/librte_eal/include/rte_service_component.h
@@ -87,6 +87,19 @@ int32_t rte_service_component_register(const struct rte_service_spec *spec,
  */
 int32_t rte_service_component_unregister(uint32_t id);
 
+/**
+ * Set an attribute for this service.
+ *
+ * Note this API is to be called by the service implementation, to make the
+ * statistic available via the usual attr_get() service APIs.
+ *
+ * @retval 0 Success
+ * @retval -EINVAL Invalid service id or attribute provided
+ */
+__rte_experimental
+int32_t rte_service_component_attr_set(uint32_t id, uint32_t attr,
+		uint64_t value);
+
 /**
  * Private function to allow EAL to initialized default mappings.
  *
diff --git a/lib/librte_eal/rte_eal_version.map b/lib/librte_eal/rte_eal_version.map
index 0b18e2ef85..bb5e19ae9e 100644
--- a/lib/librte_eal/rte_eal_version.map
+++ b/lib/librte_eal/rte_eal_version.map
@@ -397,6 +397,9 @@ EXPERIMENTAL {
 	rte_mp_disable;
 	rte_thread_register;
 	rte_thread_unregister;
+
+	# added in 20.11
+	rte_service_component_attr_set;
 };
 
 INTERNAL {
-- 
2.17.1


^ permalink raw reply	[flat|nested] 16+ messages in thread

* [dpdk-dev] [PATCH v2 2/2] event/sw: add useful work done attribute
  2020-09-14 14:37 ` [dpdk-dev] [PATCH v2 " Harry van Haaren
@ 2020-09-14 14:37   ` Harry van Haaren
  2020-09-21 14:49   ` [dpdk-dev] [PATCH v2 1/2] service: add component useful work attribute David Marchand
  2020-09-23 11:39   ` Mattias Rönnblom
  2 siblings, 0 replies; 16+ messages in thread
From: Harry van Haaren @ 2020-09-14 14:37 UTC (permalink / raw)
  To: dev; +Cc: david.marchand, Harry van Haaren

This commit exposes if useful work is done to the service
instance. The normal service_attr_get() API can be used to
retrieve the value of the attribute.

Signed-off-by: Harry van Haaren <harry.van.haaren@intel.com>
---
 drivers/event/sw/sw_evdev_scheduler.c | 7 +++++++
 1 file changed, 7 insertions(+)

diff --git a/drivers/event/sw/sw_evdev_scheduler.c b/drivers/event/sw/sw_evdev_scheduler.c
index cff747da89..4e932612d5 100644
--- a/drivers/event/sw/sw_evdev_scheduler.c
+++ b/drivers/event/sw/sw_evdev_scheduler.c
@@ -5,6 +5,9 @@
 #include <rte_ring.h>
 #include <rte_hash_crc.h>
 #include <rte_event_ring.h>
+
+#include <rte_service_component.h>
+
 #include "sw_evdev.h"
 #include "iq_chunk.h"
 
@@ -554,6 +557,10 @@ sw_event_schedule(struct rte_eventdev *dev)
 	sw->sched_no_iq_enqueues += (in_pkts_total == 0);
 	sw->sched_no_cq_enqueues += (out_pkts_total == 0);
 
+	uint64_t work_done = (in_pkts_total + out_pkts_total) != 0;
+	rte_service_component_attr_set(sw->service_id,
+			RTE_SERVICE_ATTR_USEFUL_WORK_LAST_ITER, work_done);
+
 	/* push all the internal buffered QEs in port->cq_ring to the
 	 * worker cores: aka, do the ring transfers batched.
 	 */
-- 
2.17.1


^ permalink raw reply	[flat|nested] 16+ messages in thread

* Re: [dpdk-dev] [PATCH 1/2] service: add component useful work attribute
  2020-09-14  8:37 ` [dpdk-dev] [PATCH 1/2] service: add component useful work attribute David Marchand
@ 2020-09-14 14:37   ` Van Haaren, Harry
  0 siblings, 0 replies; 16+ messages in thread
From: Van Haaren, Harry @ 2020-09-14 14:37 UTC (permalink / raw)
  To: David Marchand; +Cc: dev

> -----Original Message-----
> From: David Marchand <david.marchand@redhat.com>
> Sent: Monday, September 14, 2020 9:37 AM
> To: Van Haaren, Harry <harry.van.haaren@intel.com>
> Cc: dev <dev@dpdk.org>
> Subject: Re: [dpdk-dev] [PATCH 1/2] service: add component useful work attribute
> 
> On Mon, Sep 7, 2020 at 6:20 PM Harry van Haaren
> <harry.van.haaren@intel.com> wrote:
> > diff --git a/lib/librte_eal/include/rte_service_component.h
> b/lib/librte_eal/include/rte_service_component.h
> > index 9e66ee7e29..2caa7981b5 100644
> > --- a/lib/librte_eal/include/rte_service_component.h
> > +++ b/lib/librte_eal/include/rte_service_component.h
> > @@ -87,6 +87,18 @@ int32_t rte_service_component_register(const struct
> rte_service_spec *spec,
> >   */
> >  int32_t rte_service_component_unregister(uint32_t id);
> >
> > +/**
> > + * Set an attribute for this service.
> > + *
> > + * Note this API is to be called by the service implementation, to make the
> > + * statistic available via the usual attr_get() service APIs.
> > + *
> > + * @retval 0 Success
> > + * @retval -EINVAL Invalid service id or attribute provided
> > + */
> > +int32_t rte_service_component_attr_set(uint32_t id, uint32_t attr,
> > +               uint64_t value);
> > +
> 
> The experimental tag is missing for this declaration.
> 
> https://travis-ci.com/ovsrobot/dpdk/builds/183185060
> http://mails.dpdk.org/archives/test-report/2020-September/150152.html

Fixed, and v2 sent; thanks; http://patches.dpdk.org/project/dpdk/list/?series=12201

<snip>

^ permalink raw reply	[flat|nested] 16+ messages in thread

* Re: [dpdk-dev] [PATCH v2 1/2] service: add component useful work attribute
  2020-09-14 14:37 ` [dpdk-dev] [PATCH v2 " Harry van Haaren
  2020-09-14 14:37   ` [dpdk-dev] [PATCH v2 2/2] event/sw: add useful work done attribute Harry van Haaren
@ 2020-09-21 14:49   ` David Marchand
  2020-09-21 15:17     ` Van Haaren, Harry
  2020-09-21 23:47     ` Honnappa Nagarahalli
  2020-09-23 11:39   ` Mattias Rönnblom
  2 siblings, 2 replies; 16+ messages in thread
From: David Marchand @ 2020-09-21 14:49 UTC (permalink / raw)
  To: Harry van Haaren; +Cc: dev, Lukasz Wojciechowski, Honnappa Nagarahalli

Hello Harry,

On Mon, Sep 14, 2020 at 4:36 PM Harry van Haaren
<harry.van.haaren@intel.com> wrote:
>
> This commit adds a new attribute which allows the service to indicate
> if the previous iteration of work was "useful". Useful work here implies
> forward progress was made.
>
> Exposing this information via an attribute to the application allows
> tracking of CPU cycles as being useful or not-useful, and a CPU load
> estimate can be deduced from that information.
>
> Signed-off-by: Harry van Haaren <harry.van.haaren@intel.com>

This is missing a unit test.
Getting others' opinion on this addition would be good too.

Thanks.


-- 
David Marchand


^ permalink raw reply	[flat|nested] 16+ messages in thread

* Re: [dpdk-dev] [PATCH v2 1/2] service: add component useful work attribute
  2020-09-21 14:49   ` [dpdk-dev] [PATCH v2 1/2] service: add component useful work attribute David Marchand
@ 2020-09-21 15:17     ` Van Haaren, Harry
  2020-09-21 23:47     ` Honnappa Nagarahalli
  1 sibling, 0 replies; 16+ messages in thread
From: Van Haaren, Harry @ 2020-09-21 15:17 UTC (permalink / raw)
  To: David Marchand
  Cc: dev, Lukasz Wojciechowski, Honnappa Nagarahalli, jerinj,
	mattias.ronnblom

> -----Original Message-----
> From: David Marchand <david.marchand@redhat.com>
> Sent: Monday, September 21, 2020 3:49 PM
> To: Van Haaren, Harry <harry.van.haaren@intel.com>
> Cc: dev <dev@dpdk.org>; Lukasz Wojciechowski
> <l.wojciechow@partner.samsung.com>; Honnappa Nagarahalli
> <Honnappa.Nagarahalli@arm.com>
> Subject: Re: [dpdk-dev] [PATCH v2 1/2] service: add component useful work
> attribute
> 
> Hello Harry,
> 
> On Mon, Sep 14, 2020 at 4:36 PM Harry van Haaren
> <harry.van.haaren@intel.com> wrote:
> >
> > This commit adds a new attribute which allows the service to indicate
> > if the previous iteration of work was "useful". Useful work here implies
> > forward progress was made.
> >
> > Exposing this information via an attribute to the application allows
> > tracking of CPU cycles as being useful or not-useful, and a CPU load
> > estimate can be deduced from that information.
> >
> > Signed-off-by: Harry van Haaren <harry.van.haaren@intel.com>
> 
> This is missing a unit test.
> Getting others' opinion on this addition would be good too.

Thanks for review, sure will add a unit test.

+CC Jerin and Mattias; Eventdev SW PMD usage, I'd like to indicate if there has
been "useful work" done last iteration. Do you see any easy improvements? Concerns?

Regards, -Harry


^ permalink raw reply	[flat|nested] 16+ messages in thread

* Re: [dpdk-dev] [PATCH v2 1/2] service: add component useful work attribute
  2020-09-21 14:49   ` [dpdk-dev] [PATCH v2 1/2] service: add component useful work attribute David Marchand
  2020-09-21 15:17     ` Van Haaren, Harry
@ 2020-09-21 23:47     ` Honnappa Nagarahalli
  1 sibling, 0 replies; 16+ messages in thread
From: Honnappa Nagarahalli @ 2020-09-21 23:47 UTC (permalink / raw)
  To: David Marchand, Harry van Haaren
  Cc: dev, Lukasz Wojciechowski, nd, Honnappa Nagarahalli, nd

<snip>

> 
> Hello Harry,
> 
> On Mon, Sep 14, 2020 at 4:36 PM Harry van Haaren
> <harry.van.haaren@intel.com> wrote:
> >
> > This commit adds a new attribute which allows the service to indicate
> > if the previous iteration of work was "useful". Useful work here
> > implies forward progress was made.
> >
> > Exposing this information via an attribute to the application allows
> > tracking of CPU cycles as being useful or not-useful, and a CPU load
> > estimate can be deduced from that information.
> >
> > Signed-off-by: Harry van Haaren <harry.van.haaren@intel.com>
> 
> This is missing a unit test.
> Getting others' opinion on this addition would be good too.
Yes, adding unit tests would be good.

> 
> Thanks.
> 
> 
> --
> David Marchand


^ permalink raw reply	[flat|nested] 16+ messages in thread

* Re: [dpdk-dev] [PATCH v2 1/2] service: add component useful work attribute
  2020-09-14 14:37 ` [dpdk-dev] [PATCH v2 " Harry van Haaren
  2020-09-14 14:37   ` [dpdk-dev] [PATCH v2 2/2] event/sw: add useful work done attribute Harry van Haaren
  2020-09-21 14:49   ` [dpdk-dev] [PATCH v2 1/2] service: add component useful work attribute David Marchand
@ 2020-09-23 11:39   ` Mattias Rönnblom
  2020-09-23 14:16     ` Van Haaren, Harry
  2 siblings, 1 reply; 16+ messages in thread
From: Mattias Rönnblom @ 2020-09-23 11:39 UTC (permalink / raw)
  To: Harry van Haaren, dev; +Cc: david.marchand

On 2020-09-14 16:37, Harry van Haaren wrote:
> This commit adds a new attribute which allows the service to indicate
> if the previous iteration of work was "useful". Useful work here implies
> forward progress was made.

> Exposing this information via an attribute to the application allows
> tracking of CPU cycles as being useful or not-useful, and a CPU load
> estimate can be deduced from that information.


How would that tracking be implemented? rte_service.c already keeps 
track of the amount of busy cycles per service. Would it be possible to 
reuse that mechanism to achieve the same goal?


We did some prototyping on dynamic load balancing for the service core 
framework, and then we extended the API is such a way that the service 
callback would return a bool indicating if forward progress was made, if 
I recall correctly. Sampling these counters allowed for tracking load on 
both a per-lcore and per-service basis.


> Signed-off-by: Harry van Haaren <harry.van.haaren@intel.com>
>
> ---
>
> v2:
> - Add experimental tag to new function.
>
> ---
>   lib/librte_eal/common/rte_service.c           | 19 +++++++++++++++++++
>   lib/librte_eal/include/rte_service.h          |  5 +++++
>   .../include/rte_service_component.h           | 13 +++++++++++++
>   lib/librte_eal/rte_eal_version.map            |  3 +++
>   4 files changed, 40 insertions(+)
>
> diff --git a/lib/librte_eal/common/rte_service.c b/lib/librte_eal/common/rte_service.c
> index 6a0e0ff65d..f9792a138b 100644
> --- a/lib/librte_eal/common/rte_service.c
> +++ b/lib/librte_eal/common/rte_service.c
> @@ -58,6 +58,7 @@ struct rte_service_spec_impl {
>   	uint32_t num_mapped_cores;
>   	uint64_t calls;
>   	uint64_t cycles_spent;
> +	uint8_t useful_work_last_iter;
>   } __rte_cache_aligned;
>   
>   /* the internal values of a service core */
> @@ -293,6 +294,21 @@ rte_service_component_unregister(uint32_t id)
>   	return 0;
>   }
>   
> +int32_t
> +rte_service_component_attr_set(uint32_t id, uint32_t attr, uint64_t value)
> +{
> +	struct rte_service_spec_impl *s;
> +	SERVICE_VALID_GET_OR_ERR_RET(id, s, -EINVAL);
> +
> +	switch (attr) {
> +	case RTE_SERVICE_ATTR_USEFUL_WORK_LAST_ITER:
> +		s->useful_work_last_iter = value;
> +		return 0;
> +	default:
> +		return -EINVAL;
> +	};
> +}
> +
>   int32_t
>   rte_service_component_runstate_set(uint32_t id, uint32_t runstate)
>   {
> @@ -778,6 +794,9 @@ rte_service_attr_get(uint32_t id, uint32_t attr_id, uint64_t *attr_value)
>   		return -EINVAL;
>   
>   	switch (attr_id) {
> +	case RTE_SERVICE_ATTR_USEFUL_WORK_LAST_ITER:
> +		*attr_value = s->useful_work_last_iter;
> +		return 0;
>   	case RTE_SERVICE_ATTR_CYCLES:
>   		*attr_value = s->cycles_spent;
>   		return 0;
> diff --git a/lib/librte_eal/include/rte_service.h b/lib/librte_eal/include/rte_service.h
> index e2d0a6dd32..e9836a1a68 100644
> --- a/lib/librte_eal/include/rte_service.h
> +++ b/lib/librte_eal/include/rte_service.h
> @@ -370,6 +370,11 @@ int32_t rte_service_dump(FILE *f, uint32_t id);
>    */
>   #define RTE_SERVICE_ATTR_CALL_COUNT 1
>   
> +/**
> + * Returns if the last iteration of the service resulted in useful work done.
> + */
> +#define RTE_SERVICE_ATTR_USEFUL_WORK_LAST_ITER 2
> +
>   /**
>    * Get an attribute from a service.
>    *
> diff --git a/lib/librte_eal/include/rte_service_component.h b/lib/librte_eal/include/rte_service_component.h
> index 9e66ee7e29..534f41f531 100644
> --- a/lib/librte_eal/include/rte_service_component.h
> +++ b/lib/librte_eal/include/rte_service_component.h
> @@ -87,6 +87,19 @@ int32_t rte_service_component_register(const struct rte_service_spec *spec,
>    */
>   int32_t rte_service_component_unregister(uint32_t id);
>   
> +/**
> + * Set an attribute for this service.
> + *
> + * Note this API is to be called by the service implementation, to make the
> + * statistic available via the usual attr_get() service APIs.
> + *
> + * @retval 0 Success
> + * @retval -EINVAL Invalid service id or attribute provided
> + */
> +__rte_experimental
> +int32_t rte_service_component_attr_set(uint32_t id, uint32_t attr,
> +		uint64_t value);
> +
>   /**
>    * Private function to allow EAL to initialized default mappings.
>    *
> diff --git a/lib/librte_eal/rte_eal_version.map b/lib/librte_eal/rte_eal_version.map
> index 0b18e2ef85..bb5e19ae9e 100644
> --- a/lib/librte_eal/rte_eal_version.map
> +++ b/lib/librte_eal/rte_eal_version.map
> @@ -397,6 +397,9 @@ EXPERIMENTAL {
>   	rte_mp_disable;
>   	rte_thread_register;
>   	rte_thread_unregister;
> +
> +	# added in 20.11
> +	rte_service_component_attr_set;
>   };
>   
>   INTERNAL {



^ permalink raw reply	[flat|nested] 16+ messages in thread

* Re: [dpdk-dev] [PATCH v2 1/2] service: add component useful work attribute
  2020-09-23 11:39   ` Mattias Rönnblom
@ 2020-09-23 14:16     ` Van Haaren, Harry
  2020-10-05 16:45       ` Van Haaren, Harry
  0 siblings, 1 reply; 16+ messages in thread
From: Van Haaren, Harry @ 2020-09-23 14:16 UTC (permalink / raw)
  To: Mattias Rönnblom, dev; +Cc: david.marchand

> -----Original Message-----
> From: Mattias Rönnblom <mattias.ronnblom@ericsson.com>
> Sent: Wednesday, September 23, 2020 12:40 PM
> To: Van Haaren, Harry <harry.van.haaren@intel.com>; dev@dpdk.org
> Cc: david.marchand@dpdk.org
> Subject: Re: [dpdk-dev] [PATCH v2 1/2] service: add component useful work
> attribute

Hi Mattias,

Thanks for taking time to review & prompt reply!

> On 2020-09-14 16:37, Harry van Haaren wrote:
> > This commit adds a new attribute which allows the service to indicate
> > if the previous iteration of work was "useful". Useful work here implies
> > forward progress was made.
> 
> > Exposing this information via an attribute to the application allows
> > tracking of CPU cycles as being useful or not-useful, and a CPU load
> > estimate can be deduced from that information.
> 
> 
> How would that tracking be implemented? rte_service.c already keeps
> track of the amount of busy cycles per service. Would it be possible to
> reuse that mechanism to achieve the same goal?

Tracking "busy cycles" is not exactly the same - Eventdev SW PMD can spend
cycles polling, and trying to move packets around its internal queues, but make
no forward progress. Measuring cycles spent in the service would not indicate
the correct "busyness" in that case.

In the suggested patchset, each service (e.g Eventdev SW PMD) can update
a statistic itself, pushing an attribute value into the service-cores layer.
This method allows each PMD to define "useful work" in its own way.

> We did some prototyping on dynamic load balancing for the service core
> framework, and then we extended the API is such a way that the service
> callback would return a bool indicating if forward progress was made, if
> I recall correctly. Sampling these counters allowed for tracking load on
> both a per-lcore and per-service basis.

The service callback return value can be stored/inspected on the service-core
itself, but how to show that to the application? It still requires an attribute API
like proposed below re-using "attr_get" API I think.

So really the only difference in the prototype you mention is how the
service itself communicates business to the service-cores infrastructure in EAL.

Perhaps re-purposing return-value is simpler, but it limits statistics from the
service to just business, and the API change requires all services to change.

Pros of adding an API as this patchset proposes is to push attribute values to
service-core in EAL is extensibility, and no API breakage.

Given that context, Ack / push-back to this suggested approach?

> > Signed-off-by: Harry van Haaren <harry.van.haaren@intel.com>
> >
> > ---
> >
> > v2:
> > - Add experimental tag to new function.
> >
> > ---
> >   lib/librte_eal/common/rte_service.c           | 19 +++++++++++++++++++
> >   lib/librte_eal/include/rte_service.h          |  5 +++++
> >   .../include/rte_service_component.h           | 13 +++++++++++++
> >   lib/librte_eal/rte_eal_version.map            |  3 +++
> >   4 files changed, 40 insertions(+)
> >
> > diff --git a/lib/librte_eal/common/rte_service.c
> b/lib/librte_eal/common/rte_service.c
> > index 6a0e0ff65d..f9792a138b 100644
> > --- a/lib/librte_eal/common/rte_service.c
> > +++ b/lib/librte_eal/common/rte_service.c
> > @@ -58,6 +58,7 @@ struct rte_service_spec_impl {
> >   	uint32_t num_mapped_cores;
> >   	uint64_t calls;
> >   	uint64_t cycles_spent;
> > +	uint8_t useful_work_last_iter;
> >   } __rte_cache_aligned;
> >
> >   /* the internal values of a service core */
> > @@ -293,6 +294,21 @@ rte_service_component_unregister(uint32_t id)
> >   	return 0;
> >   }
> >
> > +int32_t
> > +rte_service_component_attr_set(uint32_t id, uint32_t attr, uint64_t value)
> > +{
> > +	struct rte_service_spec_impl *s;
> > +	SERVICE_VALID_GET_OR_ERR_RET(id, s, -EINVAL);
> > +
> > +	switch (attr) {
> > +	case RTE_SERVICE_ATTR_USEFUL_WORK_LAST_ITER:
> > +		s->useful_work_last_iter = value;
> > +		return 0;
> > +	default:
> > +		return -EINVAL;
> > +	};
> > +}
> > +
> >   int32_t
> >   rte_service_component_runstate_set(uint32_t id, uint32_t runstate)
> >   {
> > @@ -778,6 +794,9 @@ rte_service_attr_get(uint32_t id, uint32_t attr_id,
> uint64_t *attr_value)
> >   		return -EINVAL;
> >
> >   	switch (attr_id) {
> > +	case RTE_SERVICE_ATTR_USEFUL_WORK_LAST_ITER:
> > +		*attr_value = s->useful_work_last_iter;
> > +		return 0;
> >   	case RTE_SERVICE_ATTR_CYCLES:
> >   		*attr_value = s->cycles_spent;
> >   		return 0;
> > diff --git a/lib/librte_eal/include/rte_service.h
> b/lib/librte_eal/include/rte_service.h
> > index e2d0a6dd32..e9836a1a68 100644
> > --- a/lib/librte_eal/include/rte_service.h
> > +++ b/lib/librte_eal/include/rte_service.h
> > @@ -370,6 +370,11 @@ int32_t rte_service_dump(FILE *f, uint32_t id);
> >    */
> >   #define RTE_SERVICE_ATTR_CALL_COUNT 1
> >
> > +/**
> > + * Returns if the last iteration of the service resulted in useful work done.
> > + */
> > +#define RTE_SERVICE_ATTR_USEFUL_WORK_LAST_ITER 2
> > +

<snip>

^ permalink raw reply	[flat|nested] 16+ messages in thread

* Re: [dpdk-dev] [PATCH v2 1/2] service: add component useful work attribute
  2020-09-23 14:16     ` Van Haaren, Harry
@ 2020-10-05 16:45       ` Van Haaren, Harry
  2020-10-16 12:20         ` David Marchand
  0 siblings, 1 reply; 16+ messages in thread
From: Van Haaren, Harry @ 2020-10-05 16:45 UTC (permalink / raw)
  To: Van Haaren, Harry, Mattias Rönnblom, dev; +Cc: David Marchand

> -----Original Message-----
> From: dev <dev-bounces@dpdk.org> On Behalf Of Van Haaren, Harry
> Sent: Wednesday, September 23, 2020 3:17 PM
> To: Mattias Rönnblom <mattias.ronnblom@ericsson.com>; dev@dpdk.org
> Cc: david.marchand@dpdk.org
> Subject: Re: [dpdk-dev] [PATCH v2 1/2] service: add component useful work
> attribute
> 
> > -----Original Message-----
> > From: Mattias Rönnblom <mattias.ronnblom@ericsson.com>
> > Sent: Wednesday, September 23, 2020 12:40 PM
> > To: Van Haaren, Harry <harry.van.haaren@intel.com>; dev@dpdk.org
> > Cc: david.marchand@dpdk.org
> > Subject: Re: [dpdk-dev] [PATCH v2 1/2] service: add component useful work
> > attribute
> 
> Hi Mattias,
> 
> Thanks for taking time to review & prompt reply!

Ping Mattias - any input based on follow up discussion below?


> > On 2020-09-14 16:37, Harry van Haaren wrote:
> > > This commit adds a new attribute which allows the service to indicate
> > > if the previous iteration of work was "useful". Useful work here implies
> > > forward progress was made.
> >
> > > Exposing this information via an attribute to the application allows
> > > tracking of CPU cycles as being useful or not-useful, and a CPU load
> > > estimate can be deduced from that information.
> >
> >
> > How would that tracking be implemented? rte_service.c already keeps
> > track of the amount of busy cycles per service. Would it be possible to
> > reuse that mechanism to achieve the same goal?
> 
> Tracking "busy cycles" is not exactly the same - Eventdev SW PMD can spend
> cycles polling, and trying to move packets around its internal queues, but make
> no forward progress. Measuring cycles spent in the service would not indicate
> the correct "busyness" in that case.
> 
> In the suggested patchset, each service (e.g Eventdev SW PMD) can update
> a statistic itself, pushing an attribute value into the service-cores layer.
> This method allows each PMD to define "useful work" in its own way.
> 
> > We did some prototyping on dynamic load balancing for the service core
> > framework, and then we extended the API is such a way that the service
> > callback would return a bool indicating if forward progress was made, if
> > I recall correctly. Sampling these counters allowed for tracking load on
> > both a per-lcore and per-service basis.
> 
> The service callback return value can be stored/inspected on the service-core
> itself, but how to show that to the application? It still requires an attribute API
> like proposed below re-using "attr_get" API I think.
> 
> So really the only difference in the prototype you mention is how the
> service itself communicates business to the service-cores infrastructure in EAL.
> 
> Perhaps re-purposing return-value is simpler, but it limits statistics from the
> service to just business, and the API change requires all services to change.
> 
> Pros of adding an API as this patchset proposes is to push attribute values to
> service-core in EAL is extensibility, and no API breakage.
> 
> Given that context, Ack / push-back to this suggested approach?


<snip patch contents>


^ permalink raw reply	[flat|nested] 16+ messages in thread

* Re: [dpdk-dev] [PATCH v2 1/2] service: add component useful work attribute
  2020-10-05 16:45       ` Van Haaren, Harry
@ 2020-10-16 12:20         ` David Marchand
  2020-10-16 12:57           ` Van Haaren, Harry
  0 siblings, 1 reply; 16+ messages in thread
From: David Marchand @ 2020-10-16 12:20 UTC (permalink / raw)
  To: Van Haaren, Harry, Mattias Rönnblom; +Cc: dev

On Mon, Oct 5, 2020 at 8:59 PM Van Haaren, Harry
<harry.van.haaren@intel.com> wrote:
> > -----Original Message-----
> > From: dev <dev-bounces@dpdk.org> On Behalf Of Van Haaren, Harry
> > Sent: Wednesday, September 23, 2020 3:17 PM
> > To: Mattias Rönnblom <mattias.ronnblom@ericsson.com>; dev@dpdk.org
> > Cc: david.marchand@dpdk.org
> > Subject: Re: [dpdk-dev] [PATCH v2 1/2] service: add component useful work
> > attribute
> >
> > > -----Original Message-----
> > > From: Mattias Rönnblom <mattias.ronnblom@ericsson.com>
> > > Sent: Wednesday, September 23, 2020 12:40 PM
> > > To: Van Haaren, Harry <harry.van.haaren@intel.com>; dev@dpdk.org
> > > Cc: david.marchand@dpdk.org
> > > Subject: Re: [dpdk-dev] [PATCH v2 1/2] service: add component useful work
> > > attribute
> >
> > Hi Mattias,
> >
> > Thanks for taking time to review & prompt reply!
>
> Ping Mattias - any input based on follow up discussion below?
>
>
> > > On 2020-09-14 16:37, Harry van Haaren wrote:
> > > > This commit adds a new attribute which allows the service to indicate
> > > > if the previous iteration of work was "useful". Useful work here implies
> > > > forward progress was made.
> > >
> > > > Exposing this information via an attribute to the application allows
> > > > tracking of CPU cycles as being useful or not-useful, and a CPU load
> > > > estimate can be deduced from that information.
> > >
> > >
> > > How would that tracking be implemented? rte_service.c already keeps
> > > track of the amount of busy cycles per service. Would it be possible to
> > > reuse that mechanism to achieve the same goal?
> >
> > Tracking "busy cycles" is not exactly the same - Eventdev SW PMD can spend
> > cycles polling, and trying to move packets around its internal queues, but make
> > no forward progress. Measuring cycles spent in the service would not indicate
> > the correct "busyness" in that case.
> >
> > In the suggested patchset, each service (e.g Eventdev SW PMD) can update
> > a statistic itself, pushing an attribute value into the service-cores layer.
> > This method allows each PMD to define "useful work" in its own way.
> >
> > > We did some prototyping on dynamic load balancing for the service core
> > > framework, and then we extended the API is such a way that the service
> > > callback would return a bool indicating if forward progress was made, if
> > > I recall correctly. Sampling these counters allowed for tracking load on
> > > both a per-lcore and per-service basis.
> >
> > The service callback return value can be stored/inspected on the service-core
> > itself, but how to show that to the application? It still requires an attribute API
> > like proposed below re-using "attr_get" API I think.
> >
> > So really the only difference in the prototype you mention is how the
> > service itself communicates business to the service-cores infrastructure in EAL.
> >
> > Perhaps re-purposing return-value is simpler, but it limits statistics from the
> > service to just business, and the API change requires all services to change.
> >
> > Pros of adding an API as this patchset proposes is to push attribute values to
> > service-core in EAL is extensibility, and no API breakage.
> >
> > Given that context, Ack / push-back to this suggested approach?

I need a conclusion.
Is this required for 20.11?


-- 
David Marchand


^ permalink raw reply	[flat|nested] 16+ messages in thread

* Re: [dpdk-dev] [PATCH v2 1/2] service: add component useful work attribute
  2020-10-16 12:20         ` David Marchand
@ 2020-10-16 12:57           ` Van Haaren, Harry
  2021-01-13 14:42             ` David Marchand
  0 siblings, 1 reply; 16+ messages in thread
From: Van Haaren, Harry @ 2020-10-16 12:57 UTC (permalink / raw)
  To: David Marchand, Mattias Rönnblom; +Cc: dev

> -----Original Message-----
> From: David Marchand <david.marchand@redhat.com>
> Sent: Friday, October 16, 2020 1:20 PM
> To: Van Haaren, Harry <harry.van.haaren@intel.com>; Mattias Rönnblom
> <mattias.ronnblom@ericsson.com>
> Cc: dev@dpdk.org
> Subject: Re: [dpdk-dev] [PATCH v2 1/2] service: add component useful work
> attribute
> 
> On Mon, Oct 5, 2020 at 8:59 PM Van Haaren, Harry
> <harry.van.haaren@intel.com> wrote:
> > > -----Original Message-----
> > > From: dev <dev-bounces@dpdk.org> On Behalf Of Van Haaren, Harry
> > > Sent: Wednesday, September 23, 2020 3:17 PM
> > > To: Mattias Rönnblom <mattias.ronnblom@ericsson.com>; dev@dpdk.org
> > > Cc: david.marchand@dpdk.org
> > > Subject: Re: [dpdk-dev] [PATCH v2 1/2] service: add component useful work
> > > attribute
> > >
> > > > -----Original Message-----
> > > > From: Mattias Rönnblom <mattias.ronnblom@ericsson.com>
> > > > Sent: Wednesday, September 23, 2020 12:40 PM
> > > > To: Van Haaren, Harry <harry.van.haaren@intel.com>; dev@dpdk.org
> > > > Cc: david.marchand@dpdk.org
> > > > Subject: Re: [dpdk-dev] [PATCH v2 1/2] service: add component useful
> work
> > > > attribute
> > >
> > > Hi Mattias,
> > >
> > > Thanks for taking time to review & prompt reply!
> >
> > Ping Mattias - any input based on follow up discussion below?
> >
> >
> > > > On 2020-09-14 16:37, Harry van Haaren wrote:
> > > > > This commit adds a new attribute which allows the service to indicate
> > > > > if the previous iteration of work was "useful". Useful work here implies
> > > > > forward progress was made.
> > > >
> > > > > Exposing this information via an attribute to the application allows
> > > > > tracking of CPU cycles as being useful or not-useful, and a CPU load
> > > > > estimate can be deduced from that information.
> > > >
> > > >
> > > > How would that tracking be implemented? rte_service.c already keeps
> > > > track of the amount of busy cycles per service. Would it be possible to
> > > > reuse that mechanism to achieve the same goal?
> > >
> > > Tracking "busy cycles" is not exactly the same - Eventdev SW PMD can spend
> > > cycles polling, and trying to move packets around its internal queues, but
> make
> > > no forward progress. Measuring cycles spent in the service would not indicate
> > > the correct "busyness" in that case.
> > >
> > > In the suggested patchset, each service (e.g Eventdev SW PMD) can update
> > > a statistic itself, pushing an attribute value into the service-cores layer.
> > > This method allows each PMD to define "useful work" in its own way.
> > >
> > > > We did some prototyping on dynamic load balancing for the service core
> > > > framework, and then we extended the API is such a way that the service
> > > > callback would return a bool indicating if forward progress was made, if
> > > > I recall correctly. Sampling these counters allowed for tracking load on
> > > > both a per-lcore and per-service basis.
> > >
> > > The service callback return value can be stored/inspected on the service-core
> > > itself, but how to show that to the application? It still requires an attribute API
> > > like proposed below re-using "attr_get" API I think.
> > >
> > > So really the only difference in the prototype you mention is how the
> > > service itself communicates business to the service-cores infrastructure in
> EAL.
> > >
> > > Perhaps re-purposing return-value is simpler, but it limits statistics from the
> > > service to just business, and the API change requires all services to change.
> > >
> > > Pros of adding an API as this patchset proposes is to push attribute values to
> > > service-core in EAL is extensibility, and no API breakage.
> > >
> > > Given that context, Ack / push-back to this suggested approach?
> 
> I need a conclusion.
> Is this required for 20.11?

Given timeline - lets leave this until 21.02 release.
I think the above solution is adequate, but don't want to rush folks.

Thanks for following up, chat next release. -Harry


^ permalink raw reply	[flat|nested] 16+ messages in thread

* Re: [dpdk-dev] [PATCH v2 1/2] service: add component useful work attribute
  2020-10-16 12:57           ` Van Haaren, Harry
@ 2021-01-13 14:42             ` David Marchand
  2021-01-14 12:15               ` Van Haaren, Harry
  0 siblings, 1 reply; 16+ messages in thread
From: David Marchand @ 2021-01-13 14:42 UTC (permalink / raw)
  To: Van Haaren, Harry, Mattias Rönnblom; +Cc: dev

Hello Harry, Mattias,

On Fri, Oct 16, 2020 at 2:57 PM Van Haaren, Harry
<harry.van.haaren@intel.com> wrote:
> > > > > On 2020-09-14 16:37, Harry van Haaren wrote:
> > > > > > This commit adds a new attribute which allows the service to indicate
> > > > > > if the previous iteration of work was "useful". Useful work here implies
> > > > > > forward progress was made.
> > > > >
> > > > > > Exposing this information via an attribute to the application allows
> > > > > > tracking of CPU cycles as being useful or not-useful, and a CPU load
> > > > > > estimate can be deduced from that information.
> > > > >
> > > > >
> > > > > How would that tracking be implemented? rte_service.c already keeps
> > > > > track of the amount of busy cycles per service. Would it be possible to
> > > > > reuse that mechanism to achieve the same goal?
> > > >
> > > > Tracking "busy cycles" is not exactly the same - Eventdev SW PMD can spend
> > > > cycles polling, and trying to move packets around its internal queues, but
> > make
> > > > no forward progress. Measuring cycles spent in the service would not indicate
> > > > the correct "busyness" in that case.
> > > >
> > > > In the suggested patchset, each service (e.g Eventdev SW PMD) can update
> > > > a statistic itself, pushing an attribute value into the service-cores layer.
> > > > This method allows each PMD to define "useful work" in its own way.
> > > >
> > > > > We did some prototyping on dynamic load balancing for the service core
> > > > > framework, and then we extended the API is such a way that the service
> > > > > callback would return a bool indicating if forward progress was made, if
> > > > > I recall correctly. Sampling these counters allowed for tracking load on
> > > > > both a per-lcore and per-service basis.
> > > >
> > > > The service callback return value can be stored/inspected on the service-core
> > > > itself, but how to show that to the application? It still requires an attribute API
> > > > like proposed below re-using "attr_get" API I think.
> > > >
> > > > So really the only difference in the prototype you mention is how the
> > > > service itself communicates business to the service-cores infrastructure in
> > EAL.
> > > >
> > > > Perhaps re-purposing return-value is simpler, but it limits statistics from the
> > > > service to just business, and the API change requires all services to change.
> > > >
> > > > Pros of adding an API as this patchset proposes is to push attribute values to
> > > > service-core in EAL is extensibility, and no API breakage.
> > > >
> > > > Given that context, Ack / push-back to this suggested approach?
> >
> > I need a conclusion.
> > Is this required for 20.11?
>
> Given timeline - lets leave this until 21.02 release.
> I think the above solution is adequate, but don't want to rush folks.
>
> Thanks for following up, chat next release. -Harry
>

Did some discussion happen?
21.02-rc1 is coming soon.



-- 
David Marchand


^ permalink raw reply	[flat|nested] 16+ messages in thread

* Re: [dpdk-dev] [PATCH v2 1/2] service: add component useful work attribute
  2021-01-13 14:42             ` David Marchand
@ 2021-01-14 12:15               ` Van Haaren, Harry
  0 siblings, 0 replies; 16+ messages in thread
From: Van Haaren, Harry @ 2021-01-14 12:15 UTC (permalink / raw)
  To: David Marchand, mattias.ronnblom; +Cc: dev

> -----Original Message-----
> From: David Marchand <david.marchand@redhat.com>
> Sent: Wednesday, January 13, 2021 2:43 PM
> To: Van Haaren, Harry <harry.van.haaren@intel.com>; mattias.ronnblom
> <mattias.ronnblom@ericsson.com>
> Cc: dev@dpdk.org
> Subject: Re: [dpdk-dev] [PATCH v2 1/2] service: add component useful work
> attribute
> 
> Hello Harry, Mattias,
> 
> On Fri, Oct 16, 2020 at 2:57 PM Van Haaren, Harry
> <harry.van.haaren@intel.com> wrote:
> > > > > > On 2020-09-14 16:37, Harry van Haaren wrote:
> > > > > > > This commit adds a new attribute which allows the service to indicate
> > > > > > > if the previous iteration of work was "useful". Useful work here implies
> > > > > > > forward progress was made.
> > > > > >
> > > > > > > Exposing this information via an attribute to the application allows
> > > > > > > tracking of CPU cycles as being useful or not-useful, and a CPU load
> > > > > > > estimate can be deduced from that information.
> > > > > >
> > > > > >
> > > > > > How would that tracking be implemented? rte_service.c already keeps
> > > > > > track of the amount of busy cycles per service. Would it be possible to
> > > > > > reuse that mechanism to achieve the same goal?
> > > > >
> > > > > Tracking "busy cycles" is not exactly the same - Eventdev SW PMD can spend
> > > > > cycles polling, and trying to move packets around its internal queues, but
> > > make
> > > > > no forward progress. Measuring cycles spent in the service would not indicate
> > > > > the correct "busyness" in that case.
> > > > >
> > > > > In the suggested patchset, each service (e.g Eventdev SW PMD) can update
> > > > > a statistic itself, pushing an attribute value into the service-cores layer.
> > > > > This method allows each PMD to define "useful work" in its own way.
> > > > >
> > > > > > We did some prototyping on dynamic load balancing for the service core
> > > > > > framework, and then we extended the API is such a way that the service
> > > > > > callback would return a bool indicating if forward progress was made, if
> > > > > > I recall correctly. Sampling these counters allowed for tracking load on
> > > > > > both a per-lcore and per-service basis.
> > > > >
> > > > > The service callback return value can be stored/inspected on the service-core
> > > > > itself, but how to show that to the application? It still requires an attribute
> API
> > > > > like proposed below re-using "attr_get" API I think.
> > > > >
> > > > > So really the only difference in the prototype you mention is how the
> > > > > service itself communicates business to the service-cores infrastructure in
> > > EAL.
> > > > >
> > > > > Perhaps re-purposing return-value is simpler, but it limits statistics from the
> > > > > service to just business, and the API change requires all services to change.
> > > > >
> > > > > Pros of adding an API as this patchset proposes is to push attribute values to
> > > > > service-core in EAL is extensibility, and no API breakage.
> > > > >
> > > > > Given that context, Ack / push-back to this suggested approach?
> > >
> > > I need a conclusion.
> > > Is this required for 20.11?
> >
> > Given timeline - lets leave this until 21.02 release.
> > I think the above solution is adequate, but don't want to rush folks.
> >
> > Thanks for following up, chat next release. -Harry
> >
> 
> Did some discussion happen?
> 21.02-rc1 is coming soon.

Thanks for flagging, I'm not aware of any continuation of the discussions.
Mattias, do you have any updates?

This v2 currently fails to apply on a version.map file conflict, but otherwise is still fine IMO.

^ permalink raw reply	[flat|nested] 16+ messages in thread

end of thread, other threads:[~2021-01-14 12:15 UTC | newest]

Thread overview: 16+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2020-09-07 16:21 [dpdk-dev] [PATCH 1/2] service: add component useful work attribute Harry van Haaren
2020-09-07 16:21 ` [dpdk-dev] [PATCH 2/2] event/sw: add useful work done attribute Harry van Haaren
2020-09-14  8:37 ` [dpdk-dev] [PATCH 1/2] service: add component useful work attribute David Marchand
2020-09-14 14:37   ` Van Haaren, Harry
2020-09-14 14:37 ` [dpdk-dev] [PATCH v2 " Harry van Haaren
2020-09-14 14:37   ` [dpdk-dev] [PATCH v2 2/2] event/sw: add useful work done attribute Harry van Haaren
2020-09-21 14:49   ` [dpdk-dev] [PATCH v2 1/2] service: add component useful work attribute David Marchand
2020-09-21 15:17     ` Van Haaren, Harry
2020-09-21 23:47     ` Honnappa Nagarahalli
2020-09-23 11:39   ` Mattias Rönnblom
2020-09-23 14:16     ` Van Haaren, Harry
2020-10-05 16:45       ` Van Haaren, Harry
2020-10-16 12:20         ` David Marchand
2020-10-16 12:57           ` Van Haaren, Harry
2021-01-13 14:42             ` David Marchand
2021-01-14 12:15               ` Van Haaren, Harry

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