Hi Robert,
Please see some comments below.
On 02/03/2015 09:42 PM, rsanford2 at gmail.com wrote:
> From: Robert Sanford <rsanford2 at gmail.com>
>
> - API rte_timer_reset() should return -1 when the timer is in the
> RUNNING or CONFIG state. Instead, it ignores the return value of
> internal function __rte_timer_reset() and always returns 0.
> We change rte_timer_reset() to return the value returned by
> __rte_timer_reset().
>
> - Change API rte_timer_reset_sync() to invoke rte_pause() while
> spin-waiting for rte_timer_reset() to succeed.
>
> - Enhance timer stress test 2 to report how many timer reset
> collisions occur, i.e., how many times rte_timer_reset() fails
> due to a timer being in the CONFIG state.
>
> Signed-off-by: Robert Sanford <rsanford2 at gmail.com>
>
> ---
> app/test/test_timer.c | 25 ++++++++++++++++++++++---
> lib/librte_timer/rte_timer.c | 7 +++----
> 2 files changed, 25 insertions(+), 7 deletions(-)
>
> diff --git a/app/test/test_timer.c b/app/test/test_timer.c
> index 4b4800b..2f27f84 100644
> --- a/app/test/test_timer.c
> +++ b/app/test/test_timer.c
> @@ -247,12 +247,15 @@ static int
> timer_stress2_main_loop(__attribute__((unused)) void *arg)
> {
> static struct rte_timer *timers;
> - int i;
> + int i, ret;
> static volatile int ready = 0;
> uint64_t delay = rte_get_timer_hz() / 4;
> unsigned lcore_id = rte_lcore_id();
> + int32_t my_collisions = 0;
> + static rte_atomic32_t collisions = RTE_ATOMIC32_INIT(0);
>
> if (lcore_id == rte_get_master_lcore()) {
> + cb_count = 0;
> timers = rte_malloc(NULL, sizeof(*timers) * NB_STRESS2_TIMERS,
> 0);
> if (timers == NULL) {
> printf("Test Failed\n");
> @@ -268,15 +271,24 @@ timer_stress2_main_loop(__attribute__((unused)) void
> *arg)
> }
>
> /* have all cores schedule all timers on master lcore */
> - for (i = 0; i < NB_STRESS2_TIMERS; i++)
> - rte_timer_reset(&timers[i], delay, SINGLE,
> rte_get_master_lcore(),
> + for (i = 0; i < NB_STRESS2_TIMERS; i++) {
> + ret = rte_timer_reset(&timers[i], delay, SINGLE,
> rte_get_master_lcore(),
> timer_stress2_cb, NULL);
> + /* there will be collisions when multiple cores simultaneously
> + * configure the same timers */
> + if (ret != 0)
> + my_collisions++;
> + }
> + if (my_collisions != 0)
> + rte_atomic32_add(&collisions, my_collisions);
>
> ready = 0;
> rte_delay_ms(500);
>
> /* now check that we get the right number of callbacks */
> if (lcore_id == rte_get_master_lcore()) {
> + if ((my_collisions = rte_atomic32_read(&collisions)) != 0)
> + printf("- %d timer reset collisions (OK)\n",
> my_collisions);
That's not very important, but I think avoiding affectation + comparison
at the same time is clearer:
my_collisions = rte_atomic32_read(&collisions);
if (my_collisions != 0) {
...
> rte_timer_manage();
> if (cb_count != NB_STRESS2_TIMERS) {
> printf("Test Failed\n");
> @@ -311,6 +323,13 @@ timer_stress2_main_loop(__attribute__((unused)) void
> *arg)
> /* now check that we get the right number of callbacks */
> if (lcore_id == rte_get_master_lcore()) {
> rte_timer_manage();
> +
> + /* clean up statics, in case we run again */
> + rte_free(timers);
> + timers = 0;
timers = NULL is better than timers = 0 as it's a pointer.
> + ready = 0;
The lines above should go in another patch as it fixes another problem
(+ a memory leek).
"testpmd: allow to restart timer stress tests"
> + rte_atomic32_set(&collisions, 0);
> +
> if (cb_count != NB_STRESS2_TIMERS) {
> printf("Test Failed\n");
> printf("- Stress test 2, part 2 failed\n");
> diff --git a/lib/librte_timer/rte_timer.c b/lib/librte_timer/rte_timer.c
> index 269a992..d18abf5 100644
> --- a/lib/librte_timer/rte_timer.c
> +++ b/lib/librte_timer/rte_timer.c
> @@ -424,10 +424,8 @@ rte_timer_reset(struct rte_timer *tim, uint64_t ticks,
> else
> period = 0;
>
> - __rte_timer_reset(tim, cur_time + ticks, period, tim_lcore,
> + return __rte_timer_reset(tim, cur_time + ticks, period, tim_lcore,
> fct, arg, 0);
> -
> - return 0;
> }
>
> /* loop until rte_timer_reset() succeed */
> @@ -437,7 +435,8 @@ rte_timer_reset_sync(struct rte_timer *tim, uint64_t
> ticks,
> rte_timer_cb_t fct, void *arg)
> {
> while (rte_timer_reset(tim, ticks, type, tim_lcore,
> - fct, arg) != 0);
> + fct, arg) != 0)
> + rte_pause();
> }
Maybe the lines above could go to another patch too.
"timers: relax cpu in rte_timer_reset_sync()"
Also, I think the commit log should highlight the fact that
your patch also fixes rte_timer_reset_sync() that was not
working at all.
Thanks!
Olivier
