Re: [dpdk-dev] [PATCH v2 1/2] service: add component useful work attribute

["Van Haaren, Harry" <harry.van.haaren@intel.com>]

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