Re: [PATCH] clocksource: Add a helper fucntion to reduce code duplication

[Feng Tang <feng.tang@intel.com>]

Hi John,

Thanks for the review!

On Tue, May 23, 2023 at 09:39:07PM -0700, John Stultz wrote:
> On Tue, May 23, 2023 at 9:08 PM Feng Tang <feng.tang@intel.com> wrote:
> >
> > Several places use the same pattern of 'clocksource_delta() +
> > clocksource_cyc2ns()' for calcualating the time delta in nanoseconds
> > from 2 counters read from a clocksource. Add a helper function to
> > simplify the code.
> >
> > signe-off-by: Feng Tang <feng.tang@intel.com>
> 
> Thanks for submitting this!
> 
> Can you fix your Signed-off-by: line? I would have thought checkpatch
> would have caught that for you.

Sorry. The Signed-off-by was automatically added, and I must have
messed it up during composing the change log.

> Additional thoughts below.
> 
> > ---
> >  kernel/time/clocksource.c | 36 +++++++++++++++++++++---------------
> >  1 file changed, 21 insertions(+), 15 deletions(-)
> >
> > diff --git a/kernel/time/clocksource.c b/kernel/time/clocksource.c
> > index 91836b727cef..9f9e25cf5b44 100644
> > --- a/kernel/time/clocksource.c
> > +++ b/kernel/time/clocksource.c
> > @@ -145,6 +145,18 @@ static inline void clocksource_watchdog_unlock(unsigned long *flags)
> >         spin_unlock_irqrestore(&watchdog_lock, *flags);
> >  }
> >
> > +
> > +/*
> > + * Calculate the delta of 2 counters read from a clocksource, and convert
> > + * it to nanoseconds. Intended only for short time interval calculation.
> > + */
> > +static inline u64 calc_counters_to_delta_ns(u64 new, u64 old, struct clocksource *cs)
> 
> Bikeshed nit:  I'd probably do  calc_counters_to_delta_ns(struct
> clocksource *cs, u64 new, u64 old) just to match the convention
> elsewhere of passing the clocksource first.

Yes, will do.

> Also, I might suggest naming it clocksource_cycle_interval_to_ns() ?
> That feels clearer to me as to what it's doing.

This is much better. Thanks! Naming the function was the most
difficult part for me on making the patch :). 

> > +{
> > +       u64 delta = clocksource_delta(new, old, cs->mask);
> > +
> > +       return clocksource_cyc2ns(delta, cs->mult, cs->shift);
> > +}
> > +
> >  static int clocksource_watchdog_kthread(void *data);
> >  static void __clocksource_change_rating(struct clocksource *cs, int rating);
> >
> > @@ -223,7 +235,7 @@ enum wd_read_status {
> >  static enum wd_read_status cs_watchdog_read(struct clocksource *cs, u64 *csnow, u64 *wdnow)
> >  {
> >         unsigned int nretries;
> > -       u64 wd_end, wd_end2, wd_delta;
> > +       u64 wd_end, wd_end2;
> >         int64_t wd_delay, wd_seq_delay;
> >
> >         for (nretries = 0; nretries <= max_cswd_read_retries; nretries++) {
> > @@ -234,9 +246,7 @@ static enum wd_read_status cs_watchdog_read(struct clocksource *cs, u64 *csnow,
> >                 wd_end2 = watchdog->read(watchdog);
> >                 local_irq_enable();
> >
> > -               wd_delta = clocksource_delta(wd_end, *wdnow, watchdog->mask);
> > -               wd_delay = clocksource_cyc2ns(wd_delta, watchdog->mult,
> > -                                             watchdog->shift);
> > +               wd_delay = calc_counters_to_delta_ns(wd_end, *wdnow, watchdog);
> >                 if (wd_delay <= WATCHDOG_MAX_SKEW) {
> >                         if (nretries > 1 || nretries >= max_cswd_read_retries) {
> >                                 pr_warn("timekeeping watchdog on CPU%d: %s retried %d times before success\n",
> > @@ -254,8 +264,8 @@ static enum wd_read_status cs_watchdog_read(struct clocksource *cs, u64 *csnow,
> >                  * report system busy, reinit the watchdog and skip the current
> >                  * watchdog test.
> >                  */
> > -               wd_delta = clocksource_delta(wd_end2, wd_end, watchdog->mask);
> > -               wd_seq_delay = clocksource_cyc2ns(wd_delta, watchdog->mult, watchdog->shift);
> > +
> > +               wd_seq_delay = calc_counters_to_delta_ns(wd_end2, wd_end, watchdog);
> >                 if (wd_seq_delay > WATCHDOG_MAX_SKEW/2)
> >                         goto skip_test;
> >         }
> > @@ -366,8 +376,8 @@ void clocksource_verify_percpu(struct clocksource *cs)
> >                 delta = (csnow_end - csnow_mid) & cs->mask;
> >                 if (delta < 0)
> >                         cpumask_set_cpu(cpu, &cpus_ahead);
> > -               delta = clocksource_delta(csnow_end, csnow_begin, cs->mask);
> > -               cs_nsec = clocksource_cyc2ns(delta, cs->mult, cs->shift);
> > +
> > +               cs_nsec = calc_counters_to_delta_ns(csnow_end, csnow_begin, cs);
> >                 if (cs_nsec > cs_nsec_max)
> >                         cs_nsec_max = cs_nsec;
> >                 if (cs_nsec < cs_nsec_min)
> > @@ -398,7 +408,7 @@ static inline void clocksource_reset_watchdog(void)
> >
> >  static void clocksource_watchdog(struct timer_list *unused)
> >  {
> > -       u64 csnow, wdnow, cslast, wdlast, delta;
> > +       u64 csnow, wdnow, cslast, wdlast;
> >         int next_cpu, reset_pending;
> >         int64_t wd_nsec, cs_nsec;
> >         struct clocksource *cs;
> > @@ -456,14 +466,10 @@ static void clocksource_watchdog(struct timer_list *unused)
> >                         continue;
> >                 }
> >
> > -               delta = clocksource_delta(wdnow, cs->wd_last, watchdog->mask);
> > -               wd_nsec = clocksource_cyc2ns(delta, watchdog->mult,
> > -                                            watchdog->shift);
> > -
> > -               delta = clocksource_delta(csnow, cs->cs_last, cs->mask);
> > -               cs_nsec = clocksource_cyc2ns(delta, cs->mult, cs->shift);
> >                 wdlast = cs->wd_last; /* save these in case we print them */
> >                 cslast = cs->cs_last;
> > +               wd_nsec = calc_counters_to_delta_ns(wdnow, wdlast, watchdog);
> > +               cs_nsec = calc_counters_to_delta_ns(csnow, cslast, cs);
> 
> So, I get it takes common lines and combines them, but as it's an
> inline function, you're likely not going to change the resulting
> binary code, so this is just about readability, correct?
> 
> Personally, I find it easier to read code where the primitives are
> fairly obvious/explicit, even if it's somewhat repetitive.
> 
> So combining these simpler operations means the function names are
> less descriptive.  I'm sure future me will likely have to go digging
> to find the consolidated logic to remind myself what it is actually
> doing (and to double check what side effects it might have - luckily
> none!).  For instance, the ordering of the two timestamps isn't always
> obvious, whereas I know clocksource_delta() is subtraction so it
> should be delta = new - old so the ordering is easy to remember.
> 
> So I'm not sure this is much of a win for readability in my mind?
> But this is all personal taste, so I'll leave it to Thomas and others
> to decide on.

I understand your point. If people all think it's better to keep
current way, I'm fine to drop the patch. 

> I do appreciate you sending this out for consideration!

Thank you for sharing your thought!

- Feng

> 
> thanks
> -john