[PATCH v1] api: event: add generic event vector API #2149
Conversation
Add new generic event vector API for generating event vectors from all event types, as opposed to the existing packet vector API, which only supports packets. Signed-off-by: Matias Elo <[email protected]>
odpbuild
changed the title
| * ODP_EVENT_PACKET_VECTOR) to a queue with vector aggregation enabled | ||
| * (i.e. vectors within vectors). | ||
| */ | ||
| odp_event_type_t event_type; |
There was a problem hiding this comment.
Would this configuratiuon expect fastpath checks? Or is it just a warning for application?
There was a problem hiding this comment.
It's undefined behavior if application operates agains the specification, so no checks are required in the fast path. Nevertheless, it may be good to add checks behind debug flags etc. to ease debugging.
| @@ -52,6 +55,8 @@ extern "C" { | |||
| * - Timeout event (odp_timeout_t) from a timer | |||
| * - ODP_EVENT_IPSEC_STATUS | |||
| * - IPSEC status update event (odp_ipsec_status_t) | |||
| * - ODP_EVENT_VECTOR | |||
There was a problem hiding this comment.
Would the scheduled event always be ODP_EVENT_VECTOR or if odp_event_vector_config_t.event_type == ODP_EVENT_PACKET is it expeceted to be ODP_EVENT_PACKET_VECTOR
There was a problem hiding this comment.
In this case the event type would always be ODP_EVENT_VECTOR. I'd like to keep the event and packet vector APIs separate. Optimally, the whole old packet vector API would be deprecated, but we cannot do that without breaking backwards compatibility.
|
Overall, the specification looks good to me, with a few minor clarifications.
Either is fine with me, but supporting multiple aggregators per queue gives the application more options.
A max_vector_aggregators_per_queue capability should be good. |
| * When vector generation is enabled, events may be delivered both as | ||
| * event vector events and normal events. Implementation won't split up | ||
| * event vectors successfully enqueued by the application. */ | ||
| odp_event_vector_config_t vector[ODP_QUEUE_MAX_VECTOR_AGGR]; |
There was a problem hiding this comment.
Other option could be to create vector generator profile, and give handle to that here instead of full set of config params ? That would enable easy reuse of the same set of params for many queues (that have the same aggregation needs).
| * When vector generation is enabled, events may be delivered both as | ||
| * event vector events and normal events. Implementation won't split up | ||
| * event vectors successfully enqueued by the application. */ | ||
| odp_event_vector_config_t vector[ODP_QUEUE_MAX_VECTOR_AGGR]; |
There was a problem hiding this comment.
Having multiple vector aggregators per queue adds flexibility and might be useful for decoupling vector aggregation from scheduling contexts (i.e. one could aggregate pktio and crypto packets separately but still have them go through the same scheduled queue, e.g. for atomicity).
However, configuring the aggregators this way in a fixed manner at queue creation time is rather inflexible. It would not be possible to e.g. have a new aggregator for a newly created crypto session attached to an existing queue. If we want to support multiple aggregators per queue, I think it would be nice to be able to create the aggregators dynamically or at least associate them to queues dynamically.
| * When vector generation is enabled, events may be delivered both as | ||
| * event vector events and normal events. Implementation won't split up | ||
| * event vectors successfully enqueued by the application. */ | ||
| odp_event_vector_config_t vector[ODP_QUEUE_MAX_VECTOR_AGGR]; |
There was a problem hiding this comment.
Having multiple aggregators per queue raises also the question on how to select with aggregator is to be used for a packet coming for pktio/classifier/crypto/ipsec etc. This patch set adds a new SW sending function with aggregator index. something similar would be needed for all the other cases. This feels unclean to me.
Maybe we could think that aggregators are just another level of queues (which they really are) and treat them as queues in the API so that for each aggregator we would have a similar queue handle as for other queues. This queue handle could then be used in all existing API functions to specify the aggregator to be used. And if aggregators were created dynamically (see my other comment), then the aggregator creation mechanism could quite naturally return such a queue handle.
If we do not wish to support multiple aggregators per queue just yet but want to leave the door open for it, then maybe we could have a vector aggregator config as part of queue config (and the aggregator would affect everything going through that queue) and then later optionally introduce a way to create additional "auxiliary" queues for the main queue for alternate aggregators. Or something like that.
|
Replaced by #2187. |
Add new generic event vector API for generating event vectors from all event types, as opposed to the existing packet vector API, which only supports packets.
Open questions