* [PATCH] clocksource: Add a helper fucntion to reduce code duplication @ 2023-05-24 4:07 Feng Tang 2023-05-24 4:39 ` John Stultz 0 siblings, 1 reply; 3+ messages in thread From: Feng Tang @ 2023-05-24 4:07 UTC (permalink / raw) To: John Stultz, Thomas Gleixner, Stephen Boyd, linux-kernel Cc: paulmck, Feng Tang 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> --- 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) +{ + 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); cs->cs_last = csnow; cs->wd_last = wdnow; -- 2.27.0 ^ permalink raw reply related [flat|nested] 3+ messages in thread
* Re: [PATCH] clocksource: Add a helper fucntion to reduce code duplication 2023-05-24 4:07 [PATCH] clocksource: Add a helper fucntion to reduce code duplication Feng Tang @ 2023-05-24 4:39 ` John Stultz 2023-05-24 4:54 ` Feng Tang 0 siblings, 1 reply; 3+ messages in thread From: John Stultz @ 2023-05-24 4:39 UTC (permalink / raw) To: Feng Tang; +Cc: Thomas Gleixner, Stephen Boyd, linux-kernel, paulmck 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. 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. Also, I might suggest naming it clocksource_cycle_interval_to_ns() ? That feels clearer to me as to what it's doing. > +{ > + 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 do appreciate you sending this out for consideration! thanks -john ^ permalink raw reply [flat|nested] 3+ messages in thread
* Re: [PATCH] clocksource: Add a helper fucntion to reduce code duplication 2023-05-24 4:39 ` John Stultz @ 2023-05-24 4:54 ` Feng Tang 0 siblings, 0 replies; 3+ messages in thread From: Feng Tang @ 2023-05-24 4:54 UTC (permalink / raw) To: John Stultz; +Cc: Thomas Gleixner, Stephen Boyd, linux-kernel, paulmck 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 ^ permalink raw reply [flat|nested] 3+ messages in thread
end of thread, other threads:[~2023-05-24 5:02 UTC | newest] Thread overview: 3+ messages (download: mbox.gz follow: Atom feed -- links below jump to the message on this page -- 2023-05-24 4:07 [PATCH] clocksource: Add a helper fucntion to reduce code duplication Feng Tang 2023-05-24 4:39 ` John Stultz 2023-05-24 4:54 ` Feng Tang
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox