| lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
|
Open Source and information security mailing list archives
| ||
|
Message-ID: <ZG2YlaxJhjfHel8K@feng-clx>
Date: Wed, 24 May 2023 12:54:45 +0800
From: Feng Tang <feng.tang@...el.com>
To: John Stultz <jstultz@...gle.com>
CC: Thomas Gleixner <tglx@...utronix.de>,
Stephen Boyd <sboyd@...nel.org>,
<linux-kernel@...r.kernel.org>, <paulmck@...nel.org>
Subject: Re: [PATCH] clocksource: Add a helper fucntion to reduce code
duplication
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@...el.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@...el.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
Powered by blists - more mailing lists