Re: [dpdk-dev] [RFC PATCH] rte_timer: Fix rte_timer_reset return value

[Robert Sanford <rsanford2@gmail.com>]

Hi Olivier,

Thanks for reviewing this patch.
Please see my responses to your comments, below.

I also have one request for you. You probably use git almost every day. For
people who only use git maybe once per year, could you please show us the
exact sequence of commands that you run to prepare a patch series? We know
there are man pages and online documents, etc, but it would be an extremely
valuable jumpstart if you just give us a snippet of your shell history
showing the exact commands that you run to prepare and email a patch
series. I would much rather spend time getting the code right, and less
time learning (by trial and error) the nuances of git apply, add, commit,
format-patch, send-email, etc.


>       /* 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) {
>         ...
>

Yes, I will change this.


> > @@ -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.
>

Yes, I will change this.


> > +             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"
>

Yes, I will split it into two patches: rte_timer and test_timer. But, I
don't see much benefit in splitting test_timer.c changes into separate
patches for each bug discovered.


> > 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()"
>
>
If you mean that we should have one patch for rte_timer_reset() and one for
rte_timer_reset_sync(), my response is: Come on, these are two one-line
fixes in a pair of related and adjacent functions. Let's not go overboard
by splitting them into two patches. :-)

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.
>
>
We said something to that effect: "Change API rte_timer_reset_sync() to
invoke rte_pause() while spin-waiting for rte_timer_reset() to succeed." I
can use different wording if you like.


>
> Thanks!
> Olivier
>

Thank you,
Robert