Re: [PATCH v2] service: add service maintenance callback

[Piotr Krzewinski <krzewinskip@gmail.com>]

On 1/7/2025 11:01 AM, Mattias Rönnblom wrote:
> On 2024-12-31 11:02, Piotr Krzewinski wrote:
>> Add option to register a callback running on service lcores
>> along regular services, which gets information about the service loop.
>> It enables doing maintenance work or power saving during periods when
>> all registered services are idling.
>>
>> As an example application that is doing dequeue from multiple
>> event ports using single service lcore (e.g. using rte dispatcher library)
>> may want to wait for new events inside the maintenance callback when
>> there is no work available on ANY of the ports.
>> This is not possible using non zero dequeue timeout without increasing
>> latency of work that is scheduled to other event ports.
>>
> 
> If the purpose of this mechanism is to allow user-defined power management, we should try to find a more specific name. In a UNIX kernel, this kind of thing happens in the "idle loop" (or "idle task"). The user would be responsible for implementing the "idle governor" (to use Linux terminology).
> 
> "idle hook", "idle callback", or "idle handler" maybe.
> 

My initial idea, apart of power management aspects, was that such a hook could allow for some more complex but not time sensitive maintenance work to be done in periods of low traffic / low service core usage.
Though it may be a bit far fetched and not a real use case.
'Idle hook/callback' name would fit this intention as well.

> For an app using both eventdev+dispatcher lib and *other* non-trivial RTE services, the issue is really that the work scheduler (i.e., the event device) does not know about all work being performed.
> 
> That said, a solution to that larger issue likely involves some extensive rework of such an app, and potentially DPDK changes as well. The kind of callback suggested in this RFC may well serve as a stop gap solution which allows the implementation of some basic power management support.
> 

Well, we have a deployment using discussed mechanism currently due to the limitations you point out, so I figured that there may be other users that would benefit from that option. 

> In the light of we (or at least I) don't really know what we are doing here, maybe it's better to have this as a pure "iteration hook/callback", without any particular opionion on how it should be used.
> 
> Such a solution, with arrays of service call result codes and service ids, would come with a little bit more complexity/overhead.
> 
> Stephen and Jerin, your input would be greatly appreciated on this matter. Especially the "bigger picture" question.
> 

I am a bit afraid of the amount of refactoring in service framework required for this approach and that it would perhaps introduce significant overhead.
I feel that tracking return codes from all the various services inside the hook would be a bit more troublesome from 
application perspective and does not enable many more usecases.
But if there is general agreement that it is better option I can try to do some prototyping in this direction.

> 
> The existence of this new API should probably be touched upon in the user guide as well. And the release change log.

Good idea, will fix in the next version once the naming/purpose and general idea is agreed upon.

> 
> It should be made clear which thread (the service lcore's) runs this callback, and when (after each iteration).
> 
> It should be clear if multiple callbacks are allowed per lcore.
> 
> What happens if a callback is already registered?
>

Thanks, will try to clarify in v3.
 
>> + * @param callback Function callback to register
>> + * @param lcore Id of the service core.
> 
> It could be useful to have shorthand for "all current service cores". Either a separate function, or a special lcore id for the above function.
> 
> LCORE_ID_ANY could be used, but would make it look like you registered the hook on any *one* service lcore, which wouldn't be the case.
> 
> Maybe not worth the trouble.
> 

Hard to say if there is any similar notion of SERVICE_LCORE_ALL anywhere and I didn't really see a need for it.

>> + * @retval 0 Successfully registered the callback.
>> + *         -EINVAL Attempted to register an invalid callback or the
> 
> What is an "invalid callback"? NULL?
> 

Yes, NULL is the only invalid case.

Best Regards,
Piotr