Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) client-ip=2620:137:e000::1:20;
Date:   Wed, 24 May 2023 12:54:45 +0800
From:   Feng Tang <feng.tang@intel.com>
To:     John Stultz <jstultz@google.com>
CC:     Thomas Gleixner <tglx@linutronix.de>,
        Stephen Boyd <sboyd@kernel.org>,
        <linux-kernel@vger.kernel.org>, <paulmck@kernel.org>
Subject: Re: [PATCH] clocksource: Add a helper fucntion to reduce code
 duplication
Message-ID: <ZG2YlaxJhjfHel8K@feng-clx>
References: <20230524040733.66946-1-feng.tang@intel.com>
 <CANDhNCpN8YucULaTJs9YGhWSw7KHO22TdFE171XwV3fw-xM_Yw@mail.gmail.com>
Content-Type: text/plain; charset="utf-8"
Content-Disposition: inline
Content-Transfer-Encoding: 8bit
In-Reply-To: <CANDhNCpN8YucULaTJs9YGhWSw7KHO22TdFE171XwV3fw-xM_Yw@mail.gmail.com>
MIME-Version: 1.0
X-MS-Exchange-AntiSpam-MessageData-ChunkCount: 1
X-MS-Exchange-AntiSpam-MessageData-0: =?utf-8?B?NlZZeUY4RFIzS1Y0WTJVUWNSdk5oTERJNlI1a3p0Uk5rdUVpL3JkdlRsTVNL?=
 =?utf-8?B?YzZPcVNNbUE5WFNHclJKQ0Z6NjV5eDdYVHh1TEQvQVRsY3VSS1Ezei9TVzZU?=
 =?utf-8?B?U2h1UkFtVGI4b054T0JoSVk5bjJhUGd3L0VDN1JKQlJ0WU5MdHYwL0xiYkVa?=
 =?utf-8?B?a3NnTSthM2xPcDdsUXhmVWw5d2lROVJSOFBrYU5lbXFXSjlPMDM3YWpUQ29p?=
 =?utf-8?B?Z1UwNHg3NHBzZWFuaFdGbjJDVVM5WjJsbU9wUnlnajRodUN6TFJPalNrNnVI?=
 =?utf-8?B?MUhFbS84N2JBVCtFSUNsOEY4dXVrOE5tdmxXcUJmL3orOWZtRkU4Qk4zOG9C?=
 =?utf-8?B?ZDFxVGo2Mnc0UnlUakxqU0F4SmFYbkVudE9sS3l2clFaSE1waU10N205b3VH?=
 =?utf-8?B?V0hXaXI3N2p4R0hoTEtlYlBBSEd4MWZKWU53MUhwbFFzSjJreWF1c0xwd0Qw?=
 =?utf-8?B?bnUvcHgyN3pBdGNOalQ1cHZTVThUTnc4WUl4YjRaaU1ZVmVvMC9jZW5Bbnkz?=
 =?utf-8?B?amdhY2Y3OUNrV1IrU0YyWm5TblRXd1VPZlR1dGsyRHZpTTd3dm0vZm5LdXNP?=
 =?utf-8?B?Z0ZuWU5rNVJIZTQwaFdvNDNVSTNubzVxZlFLcnQrSUlhSUtOcGI2a2VObU10?=
 =?utf-8?B?RHZQYUxHZXl4SFRyNHZRVjJYQTdBdE90RDh3Wnpjdnl6aVRsdGgzM2lOTnBK?=
 =?utf-8?B?Z0VmVGpyaDJnWVFVYndCdjFWNkpFVDdDZ2s1WThaSzZUYUt6ZmpGcGxFWUVv?=
 =?utf-8?B?cWFyUTNjam9rZFFaN0Zha2xzM1JRNXpuMXk3aVI1b1lLUFlaUEU2UkcwMmlC?=
 =?utf-8?B?Ym1lVmJuRFMyZC9DcGV0MnNPY0ZhVDAvSFAxc295MEFzZW1XWFluNkRKYSsy?=
 =?utf-8?B?MzdPQ1pnQXhnWGQzbE82RlJEa2JIMHNLNlBSN0ZDZnQ0QXR1UzFpVmt1SEJp?=
 =?utf-8?B?ZGpzUkZ4QnEzSHhtTTc5SmhlbTNlWnRldDNGZ2dNU01Nc240TC9pUlRZSlpP?=
 =?utf-8?B?cDh2R1FUZm5UWncrbm5YZHNQaC9pR2xXYTJwMkRvbUJNbjloNXU2MlAvcWNO?=
 =?utf-8?B?WUw2MHJ2eVRJTE9ZOW56MXFhZk54MUJGaFJoSWYxMFN5VnhEb00rNTRhbUx6?=
 =?utf-8?B?ak9STW12Qklmc29TQXZIYVl2NlZzbnViTm55TzZrdWtwcS9Ea3dHaHVHcGx0?=
 =?utf-8?B?TkdId3d6VFowenl3K2JuNStLUG1ibHNxSHp6bWRsbGh5bVRITm04MlA3OFZi?=
 =?utf-8?B?a08xWmlrcVJubGtOc2xaYVozZ1QzMkI0YytCdzhQS0o1K0RSVjJlYWQvSzd3?=
 =?utf-8?B?Z09lYmZ6RTJPQjdzV2ZkUVUwV0pMTWc0UmNiYlp3TWFCcCtoSmQvRUJHV3BL?=
 =?utf-8?B?QmdKUnZTLy95OHFZNlI2dzlBQnVLSEIxUjZPNmZNUzlyUUNXNU5aaDVqczBT?=
 =?utf-8?B?alIzaENIekVuS2ZyWWJzTWszQklmUjJpL2VyQ0JpSXBNMlVzRnVBQ3RHVkpy?=
 =?utf-8?B?ejIzQkJUVUplejZjWGpvaDhoWUlyV3ZSUXd5UURONnpOSlJ5QjFOYWI1cXhq?=
 =?utf-8?B?NEVoZmxjT09ycUthS0gwNm9xK2VpN0ZadjMrOGdLbzRNK2JZL3FsL0NlbitQ?=
 =?utf-8?B?YWJLTVMxeVB5THpwMWRDYjhDRmhJL0daMzNsRWR2UXRBUDdEaG1PaTZCMHRB?=
 =?utf-8?B?UXRXMmVXVWJVUS9YWnVGbmJZMzRFb0JONG93clpiOFlZOCtzR0QzejltaHBY?=
 =?utf-8?B?ZXVlUndxb2Y2RXIvOUxIU3VieXM2aWd0d2lodjJ3ZjBaZjRDeFdQQ1lKNHFB?=
 =?utf-8?B?RUV2K3ZKTEt3TWJ0eEVFL0pFcGVFa0oxNEtHMXdTenhUNXA5WUpLblU1Y2hX?=
 =?utf-8?B?UGdST0FyVmJtV21VNUlOR1RibmpFRDZHM2Fsa3BaOTk3aWJndkkyMXZEV3hi?=
 =?utf-8?B?NUs3VUZiRG04OGprbTBhYk1xYU0xZ1hHenZWanBzaHIwUURNOXN4N2FYY3FD?=
 =?utf-8?B?YmZjT0lOcEVncWNKU0ptalFtVWtwK0VBZk00VW5kcmFPSFgyeXd3aFJISHFu?=
 =?utf-8?B?VFJpcVF1QlJ5SGtINDdDYlhEeEZ2cmZRaVllSVBmdkYrQkx6eXBKcE55RkdL?=
 =?utf-8?Q?hDBOTO/lm+Iav94zBaVylVEaA?=
X-MS-Exchange-CrossTenant-Network-Message-Id: 0a0fc470-5d0b-4b65-f254-08db5c13f2a4
X-MS-Exchange-CrossTenant-AuthSource: MN0PR11MB6304.namprd11.prod.outlook.com
X-MS-Exchange-CrossTenant-AuthAs: Internal
X-MS-Exchange-CrossTenant-OriginalArrivalTime: 24 May 2023 05:01:35.0015
 (UTC)
X-MS-Exchange-CrossTenant-FromEntityHeader: Hosted
X-MS-Exchange-CrossTenant-Id: 46c98d88-e344-4ed4-8496-4ed7712e255d
X-MS-Exchange-CrossTenant-MailboxType: HOSTED
X-MS-Exchange-CrossTenant-UserPrincipalName: nAjdS0AMrHVHjsttkSCzWqCn2DCUjPPIInbHkTNjlHEWKI/+IS4nZ5Nj+IpF5Gz3gTX5XMI3jfeJ7t8EMw8GWw==
X-MS-Exchange-Transport-CrossTenantHeadersStamped: CO1PR11MB4772
X-OriginatorOrg: intel.com
X-Spam-Status: No, score=-4.4 required=5.0 tests=BAYES_00,DKIMWL_WL_HIGH,
        DKIM_SIGNED,DKIM_VALID,DKIM_VALID_AU,DKIM_VALID_EF,RCVD_IN_DNSWL_MED,
        SPF_HELO_NONE,SPF_NONE,T_SCC_BODY_TEXT_LINE autolearn=ham
        autolearn_force=no version=3.4.6
X-Spam-Checker-Version: SpamAssassin 3.4.6 (2021-04-09) on
        lindbergh.monkeyblade.net
Precedence: bulk
List-ID: <linux-kernel.vger.kernel.org>
X-Mailing-List: linux-kernel@vger.kernel.org

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