[PATCH v1] example: classifier: add queue and cos configurability#2339
[PATCH v1] example: classifier: add queue and cos configurability#2339TuomasTaipale wants to merge 1 commit intoOpenDataPlane:masterfrom
Conversation
Add new `-q`, `--cos_queue_param` option for configuring the CoS/queue that is defined with the policy destination queue parameter (`-p <dst queue>`). For a queue, priority, synchronization and a single packet aggregator with timeout and vector size can be configured. For the related CoS, aggregator enqueue profile and optional parameter for the custom profile can be configured. These now enable the testing of event aggregation in the context of classification with varying CoS aggregator enqueuing profiles. Signed-off-by: Tuomas Taipale <tuomas.taipale@nokia.com>
odpbuild
changed the title
| " <dst queue> Name of the destination queue (CoS).\n" | ||
| "\n" | ||
| " -c, --count <num> CPU count, 0=all available, default=1\n" | ||
| " -q, --cos_queue_param <name>:<queue prio>:<queue sync>:<cos enqueue profile type>:<optional custom value>:<optional tmo ns>:<optional size>\n" |
There was a problem hiding this comment.
I think the pre-existing implementation has some design issues that would be good to fix or improve before building more functionality on top.
The usage text and the command line parameters seem to conflate CoS objects and queues. They refer to source queues, although there are no such things, but source CoS:es. Then the "destination queue" term is used to refer both to the actual destination queue and the destination CoS. The implementation creates one CoS for each 'queue' and does not handle cleanly multiple policy rules with the same destination CoS/queue. The usage text does not really explain how the config model of this example app differs from the config model in ODP API, which is not helpful.
I think it would be good to improve the existing design before the new queue config things get added.
I suppose policy (or PMR rule) config could be separate from CoS config. CoS objects could be explicitly created (or on demand, as needed by PMR rules, if not existing already, i.e. not one-to-one with the policy rules. Similarly, queues could perhaps be separately created. Or if the queues would be implicitly created with 1:1 correspondence with CoS objects, that could be mentioned in the help text (and fixing the CoS / queue terminology would make things more clear too).
Then the queue config and CoS config for queue enqueuing that gets added in this commit could build on top of a cleaner base.
| " <optional tmo ns>\n" | ||
| " Aggregator configuration parameter timeout in nanoseconds. Optional,\n" | ||
| " if not passed, aggregation for queue disabled.\n" | ||
| " <optional size>\n" | ||
| " Aggregator configuration parameter vector size. Mandatory if 'optional tmo ns'\n" | ||
| " passed, optional otherwise.\n" |
There was a problem hiding this comment.
Linux-generic doesn't currently support aggregation timeouts, so it would be nice to support aggregation with only size.
| if (odp_event_type(ev) == ODP_EVENT_VECTOR) { | ||
| evv = odp_event_vector_from_event(ev); | ||
| odp_event_vector_tbl(evv, &ev_tbl); | ||
| vector_size = odp_event_vector_size(evv); |
There was a problem hiding this comment.
It would be good to check that max vector size (odp_pool_param_t::event_vector.max_size) doesn't exceed MAX_PKT_BURST.
| goto out; | ||
| } | ||
|
|
||
| odph_strcpy(cos_q_param->cos_name, tmp, ODP_COS_NAME_LEN); |
There was a problem hiding this comment.
Could check that there are no duplicate names.
Also, after parameter parsing it could be verified that there are no CoS queue parameters that didn't match any policy destination queue. Just to make detecting invalid user parameters easier.
| " <cos enqueue profile type>\n" | ||
| " Aggregator enqueue profile type for CoS. Use odp_aggr_enq_profile_t::type\n" | ||
| " names.\n" | ||
| " <optional custom value>\n" | ||
| " If ODP_AEP_TYPE_CUSTOM aggregator enqueue profile was used,\n" | ||
| " pass a hex value (without '0x') for odp_aggr_enq_profile_t::param.\n" |
There was a problem hiding this comment.
I'd perhaps make these the last arguments (and optional if possible), as they are likely the least commonly used ones.
| for (i = 0; i < args->num_cos_q_param; i++) | ||
| args->cos_q_param[i].q_aggr.pool = create_vector_pool(args); |
There was a problem hiding this comment.
Based on the helper text I got the understanding that -q option could be also used to configure queue sync type and priority without enabling aggregation. However, this looks like vector pool is always created.
| /* For now, singleton pool */ | ||
| static odp_pool_t pool = ODP_POOL_INVALID; |
There was a problem hiding this comment.
This can be a surprise to a user. Perhaps print something to log if implementing support for multiple pools is difficult.
There was a problem hiding this comment.
Copyright can be updated.
| odp_event_vector_tbl(evv, &ev_tbl); | ||
| vector_size = odp_event_vector_size(evv); |
There was a problem hiding this comment.
Could use odp_event_vector_tbl() return value directly and skip calling odp_event_vector_size().
3 participants
Add new
-q,--cos_queue_paramoption for configuring the CoS/queue that is defined with the policy destination queue parameter (-p <dst queue>).For a queue, priority, synchronization and a single packet aggregator with timeout and vector size can be configured. For the related CoS, aggregator enqueue profile and optional parameter for the custom profile can be configured.
These now enable the testing of event aggregation in the context of classification with varying CoS aggregator enqueuing profiles.