On 2024-01-26 11:07, Morten Brørup wrote:
From: Mattias Rönnblom [mailto:hof...@lysator.liu.se]
Sent: Friday, 26 January 2024 09.28
On 2024-01-26 00:19, Morten Brørup wrote:
From: Mattias Rönnblom [mailto:mattias.ronnb...@ericsson.com]
Sent: Thursday, 25 January 2024 20.15
Add two new per-service counters.
RTE_SERVICE_ATTR_IDLE_CALL_COUNT tracks the number of service
function
invocations where no work was performed.
RTE_SERVICE_ATTR_ERROR_CALL_COUNT tracks the number invocations
resulting in an error.
The semantics of RTE_SERVICE_ATTR_CALL_COUNT remains the same (i.e.,
counting all invocations, regardless of return value).
The new statistics may be useful for both debugging and profiling
(e.g., calculate the average per-call processing latency for non-
idle
service calls).
Service core tests are extended to cover the new counters, and
coverage for RTE_SERVICE_ATTR_CALL_COUNT is improved.
OK to all of the above. Good stuff.
The documentation for the CYCLES attributes are updated to reflect
their actual semantics.
If this is intended behavior, then updating the documentation seems
appropriate - I would even go so far as considering it a bug fix.
However, quite a few cycles may be consumed by a service before it
can conclude that it had no work to do. Shouldn't that be considered
time spent by the service? I.e. should the code be fixed instead of the
documentation?
Generally, polling for new work in the service is cheap, in my
experience. But there's nothing in principle that prevents the
situation
your describe from occurring. You could add an "IDLE_CYCLES" counter in
case that would ever be a real-world problem.
That wouldn't be a fix, but rather just returning to the old, subtly
broken, (pre-22.11?) semantics.
Have a look at 809bd24 to see the rationale for the change. There's an
example in 4689c57.
The cause of this ambiguity is due to the fact that the platform/OS
(i.e., DPDK) doesn't know which service to "wake up" (which in turn is
the result of the platform not being able to tracking input sources,
like NIC RX queues or timer wheels) and thus must ask every service to
check if it has something to do.
OK. Makes good sense.
So definitely fix the documentation, not the code. :-)
Alternatively, keep the behavior (for backwards compatibility) and
fix the documentation, as this patch does, and add an IDLE_CYCLES
counter for time spent in idle calls.
PS: We're not using DPDK service cores in our applications, so I'm
not familiar with the details. We are using something somewhat similar
(but homegrown), also for profiling and power management purposes, and
my feedback is based on my experience with our own variant of service
cores.
When are you making the switch to service cores? :)
Our own "service cores" implementation has some slightly different properties,
which we are still experimenting with.
E.g. in addition to the special return value "idle (no work to do)", we also have a
special return value for "incomplete (more work urgently pending)" when a service
processed a full burst and still has more work pending its input queue.
We are also considering returning a value to inform what time it needs to be
called again. This concept is only an idea, and we haven't started
experimenting with it yet.
From a high level perspective, the service cores library is much like an
operating system's CPU scheduler, although based on voluntary time sharing.
Many algorithms and many parameters can be considered. It can also tie into
power management and prioritization of different tasks.
Service cores in their current form is more like a primitive kernel
bottom half implementation.
The primary work scheduler in DPDK is Eventdev, but even with Eventdev
you want some way to fit non-event kind of processing as well, and here
services cores serves a role.
rte_service.c is a good place for power management. I agree some means
for the services to convey what latency (e.g., sleep time) is acceptable
is needed. Returning a time per service function invocation would be a
flexible way to do so. Could also just be a per-lcore, or global
configuration, set by the application.
Either way:
Acked-by: Morten Brørup <m...@smartsharesystems.com>
Thanks for the review.
