[PATCH v2] test: timer_accuracy: introduce "concurrency" mode #2180
Conversation
Remove unnecessary setting of option values to zero when they have already been zeroed. Signed-off-by: Jere Leppänen <jere.leppanen@nokia.com>
…ime mode Set min_tmo to period / 10 only for one-shot absolute time mode, as it requires a min_tmo smaller than the period. For all other modes, set min_tmo to the same value as the period. Signed-off-by: Jere Leppänen <jere.leppanen@nokia.com>
…options Clearly separate mode-specific command line options from generic options in the help text. Signed-off-by: Jere Leppänen <jere.leppanen@nokia.com>
odpbuild
changed the title
| @@ -651,7 +701,7 @@ static int create_timers(test_global_t *test_global) | |||
| printf(" resolution: %" PRIu64 " nsec\n", timer_param.res_ns); | |||
| } | |||
|
|
|||
| timer_param.num_timers = alloc_timers; | |||
| timer_param.num_timers = alloc_timers * 2; | |||
There was a problem hiding this comment.
The odp_timer_capability_t.max_timers check above looks to be now invalid. Also, isn't this necessary only for concurrency mode?
There was a problem hiding this comment.
Yes, and it's not *2, the extra number of timers needed is actually equal to cpu_count.
| @@ -591,7 +640,8 @@ static int create_timers(test_global_t *test_global) | |||
|
|
|||
| odp_pool_param_init(&pool_param); | |||
| pool_param.type = ODP_POOL_TIMEOUT; | |||
| pool_param.tmo.num = alloc_timers; | |||
| pool_param.tmo.num = | |||
| alloc_timers * 2 + pool_param.tmo.cache_size * test_global->opt.cpu_count; | |||
There was a problem hiding this comment.
2 * necessary only for concurrency mode?
| uint64_t starts = ctx->starts; | ||
|
|
||
| if (events > starts) | ||
| ODPH_ERR("ctx %p timer %p time %" PRIu64 " starts %" PRIu64 |
There was a problem hiding this comment.
Does this suggest duplicate events? If so, probably best just to abort/exit right away so this won't go unnoticed even in CI.
There was a problem hiding this comment.
In v2, added an option to exit with _exit().
|
|
||
| if (test_global->opt.max_diff && | ||
| diff > (int64_t)test_global->opt.max_diff) { | ||
| ODPH_ERR("ctx %p timer %p time %" PRIu64 " diff %" PRIi64 "\n", ctx, |
There was a problem hiding this comment.
Could also count the number of events exceeding max_diff and print this later in print_stat().
There was a problem hiding this comment.
_exit() also here.
There was a problem hiding this comment.
Copyright can be updated.
| if (test_global->opt.cancel_start && | ||
| !(events % test_global->opt.cancel_start)) { | ||
| /* |
There was a problem hiding this comment.
Minor thing, but this could be saved to a const variable as the same condition is checked again below. Perhaps same could be done to all feature conditions back-to-back for readability.
| odp_timeout_t timeout = | ||
| odp_timeout_alloc(test_global->timeout_pool); | ||
| if (timeout == ODP_TIMEOUT_INVALID) { | ||
| printf("Timeout alloc failed\n"); |
| @@ -1449,6 +1642,41 @@ int main(int argc, char *argv[]) | |||
| odp_time_wait_ns(10 * ODP_TIME_MSEC_IN_NS); | |||
|
|
|||
| cancel_periodic_timers(test_global); | |||
| } else if (test_global->opt.mode == MODE_CONCURRENCY) { | |||
There was a problem hiding this comment.
Could be split into a separate function.
| printf("msec %" PRIu64 " total events %" PRIu64 " events %" PRIu64 | ||
| " events/s %" PRIu64 " stopped %d\n", | ||
| msec_total, events, e, e * 1000 / msec, stopped); |
There was a problem hiding this comment.
Could use seconds here as the print interval is always in seconds.
There was a problem hiding this comment.
It's not exactly seconds, but changed anyway.
| * interval. | ||
| */ | ||
| timer_ctx_t *ctx = &test_global->timer_ctx[i]; | ||
| uint64_t ctx_events = ctx->events; |
There was a problem hiding this comment.
Seems like a possible data race. Is this information that necessary to print anyway?
There was a problem hiding this comment.
It's definitely a data race. It's avoiding synchronization in order to have minimal effect on the test itself. The prints have been very useful in recent testing, especially when some timers stop generating events, some periodic check like this is needed to see that something stopped from happening.
On the other hand perhaps it's better to add prints ad hoc, when needed to investigate a specific problem. So this could just be removed.
For reporting errors, use ODPH_ERR() instead of printf(). Signed-off-by: Jere Leppänen <jere.leppanen@nokia.com>
Introduce a new "concurrency" mode that performs timer starts and cancels across multiple threads. This mode aims to uncover concurrency issues in the timer implementation, particularly with a short period, where timer expirations, starts, and cancels frequently involve the same timer bucket. Signed-off-by: Jere Leppänen <jere.leppanen@nokia.com>
Run all timer_accuracy modes, not just the default. Signed-off-by: Jere Leppänen <jere.leppanen@nokia.com>
JereLeppanen
force-pushed
the
dev/test-timer-concurrency
branch
from
bea0d63 to
2dae501
Compare
odpbuild
changed the title
|
v2:
|
No description provided.