-
Notifications
You must be signed in to change notification settings - Fork 135
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
[PATCH v2] API: add event vectors and event aggregators #2187
base: master
Are you sure you want to change the base?
Conversation
Add new generic event vectors that can contain events of any type, as opposed to the existing packet vectors, which can contain only packets. Signed-off-by: Matias Elo <[email protected]>
Introduce event aggregators that are associated with queues and have the same life time. Event aggregators are queues which try to aggregate multiple events into event vectors which are then enqueued to the underlying queue. There can be multiple event aggregators per queue. Add a function to get a queue handle for an event aggregator so that the queue handle can be used in most contexts where normal queue handles can be used. Dequeuing and certain queue metadata related actions are not possible with aggregator queues. Specify that event aggregators cannot be used simultaneously with the existing packet vector config in the same ODP application. The intention is to eventually deprecate the packet vector configuration as supporting the two aggregation mechanisms at the same time is redundant and may be difficult for ODP implementations. Signed-off-by: Matias Elo <[email protected]> Signed-off-by: Janne Peltonen <[email protected]>
v2:
|
#ifdef __cplusplus | ||
extern "C" { | ||
#endif | ||
|
||
#include <stdint.h> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
See 'extern "C"' usage clean-up PR (#2190). Same comment for all API/ABI headers.
/** Maximum number of event aggregators for this queue type */ | ||
uint32_t max_num_aggregators; | ||
|
||
/** Maximum number of event aggregators per queue */ | ||
uint32_t max_num_aggregators_per_queue; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
odp_event_aggregator_capability_t
is used within odp_queue_capability_t
and odp_schedule_capability_t
, where the field name already refers to aggregators (e.g. odp_queue_capability_t.plain.aggregator
). So, I'd remove aggregators
from these field names to avoid repetition.
/** Event vector generation capabilities */ | ||
odp_event_aggregator_capability_t aggregator; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Here aggregator capabilities are under aggregator
and in odp_schedule_capability_t
under vector
. Probably clearer to use same field name in both places.
* 'name' and 'param' and any configuration structures pointed to by 'param' | ||
* are used only during the call. They can be freed after queue creation. | ||
* |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is this necessary? The same holds true for many other APIs and now this single function would be a bit of an outlier. Potentially this could be stated somewhere in a single place, so it would cover all relevant APIs.
* the event aggregator. | ||
* | ||
* This function does not create a new queue but merely returns a reference | ||
* to an aggregator queue which has the same life time as the underlying |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
life time
-> lifetime
* When >= 1, configuration must be provided for each aggregator | ||
* through the 'aggregator' array. | ||
*/ | ||
uint32_t num_aggregators; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is it clear enough already or should it be still clarified that the source queue when calling e.g. odp_schedule()
is the base queue regardless of if an aggregator was used?
* The returned queue handle cannot be used for dequeuing events. It must | ||
* not be passed to odp_queue_deq() or similar. | ||
* | ||
* The returned queue handle must not be passed to any of the | ||
* odp_queue_context* or odp_queue_sched* functions and is never returned | ||
* by odp_queue_lookup(). |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Perhaps the forbidden functions could be listed explicitly in the same manner than for example in odp_init_global()
.
@@ -1,6 +1,6 @@ | |||
/* SPDX-License-Identifier: BSD-3-Clause | |||
* Copyright (c) 2017-2018 Linaro Limited | |||
* Copyright (c) 2022-2023 Nokia | |||
* Copyright (c) 2022-2024 Nokia |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Copyright years can be updated.
Introduce event vectors that, similar to packet vectors, are events that contain multiple events. An event vector can contain multiple types of events, not just packet events.
Introduce event aggregators which are associated with an underlying queue, get created when the underlying queue is created and which have their own queue handle that can be used for enqueueing purposes.
This PR replaces PR 2149. Event vectors are taken directly from that PR but put to a different commit from event aggregators. Event aggregators are based on the work in PR 2148, but now the aggregators can be referred to using queue handles gotten from odp_queue_aggregator_get() function. The start-of-vector/end-of-vector flags are not included in this PR at this point.