Received-SPF: pass (google.com: domain of linux-kernel+bounces-41602-linux.lists.archive=gmail.com@vger.kernel.org designates 2604:1380:45e3:2400::1 as permitted sender) client-ip=2604:1380:45e3:2400::1;
Date: Sun, 28 Jan 2024 19:36:21 +0800
From: Feng Tang <feng.tang@intel.com>
To: "Paul E. McKenney" <paulmck@kernel.org>
CC: Waiman Long <longman@redhat.com>, John Stultz <jstultz@google.com>, Thomas
 Gleixner <tglx@linutronix.de>, Stephen Boyd <sboyd@kernel.org>,
	<linux-kernel@vger.kernel.org>, Peter Zijlstra <peterz@infradead.org>, "Jin
 Wang" <jin1.wang@intel.com>
Subject: Re: [PATCH] clocksource: Scale the max retry number of watchdog read
 according to CPU numbers
Message-ID: <ZbY8NVT+BqTVGDq1@feng-clx.sh.intel.com>
References: <20240126091250.79985-1-feng.tang@intel.com>
 <3b3d9499-dc7a-4ea0-9615-526940f5fcf2@paulmck-laptop>
 <ZbO3bN3WEUvejRBp@feng-clx.sh.intel.com>
 <ebeff3e5-cd57-49ac-86d3-6ebe3c53fa40@redhat.com>
 <2955c8b0-cdb5-459e-9d98-9db7a252f0ff@paulmck-laptop>
Content-Type: text/plain; charset="us-ascii"
Content-Disposition: inline
In-Reply-To: <2955c8b0-cdb5-459e-9d98-9db7a252f0ff@paulmck-laptop>
Precedence: bulk
MIME-Version: 1.0
X-MS-Exchange-AntiSpam-MessageData-ChunkCount: 1
X-MS-Exchange-AntiSpam-MessageData-0: =?us-ascii?Q?k8oxg00lnfzbmxw16SAPKu6Dw0uVjOR4GltWOOAt6j2ziikFhbgMGGnsGLov?=
 =?us-ascii?Q?bGZ4pn+XUnuCWJJnbEctpcQYl22s5tb5lxSxtfFxZ6F2lhtX7Y+Y6uSB0dbw?=
 =?us-ascii?Q?eLuxrupyKUStO+MlnXqmiGbqFZaHyO9YBIxGTgYL2lDpjbA/HUWWmNFRZRJp?=
 =?us-ascii?Q?4t8QI/D6hv/fafOIuk7Et8yZmc5fxo6VPPW4sBFjXsvMPLN9YcsuAJ44NGDl?=
 =?us-ascii?Q?aINdSZEyNoXWsKlXA3s59tTuQPnFr0r0tU34fOj8gCq6ezcdJtIUUig9KVp2?=
 =?us-ascii?Q?/FJVX9Q4Mqr2YNhiD+LQcMIBWonx9zwjbLK7qCokKTFvaCLwhX+TNswHPFDj?=
 =?us-ascii?Q?+CAhUQdB9FypNoUQlhRAmBZpIjN5QozUsTk1hIFKSkba2jNIdtus3YrQAiMD?=
 =?us-ascii?Q?e1Y7tHcosinZf9nszbsiambf8AiCh9OfBSsiPQVVlZf4IziSfmCLqU/IrpKs?=
 =?us-ascii?Q?y57xX7oh0tMSEC+YAWuZAbrwU25Y6vx+JT5CfPAKyF/BBcLCSP6egEatIaUT?=
 =?us-ascii?Q?e6JxynZHn0VLFGmXGc5/7MkxsQW9W9Lk2E9X+SHLLQ08G2hbOEgdmCbf40gw?=
 =?us-ascii?Q?vpQ+YfXQYNfyhTQa6GD6Pc+HPRizmr21iiNpHeFIYcjuYhCIas7bhQ16tqX6?=
 =?us-ascii?Q?elwr31FzIqPcpe+n5dsSkdTCj6Zx5FyT3cSP/4scZ23JOxdk3a0h8TF3Z7aK?=
 =?us-ascii?Q?pkeYau8avH8CHlplDn8a1QCnQyQhASMksqk/VTJfHcYoebPAOjrM0sIW+Qzh?=
 =?us-ascii?Q?NyzbTmQ97Gtj5Z1BV4BHXSw4ozqHaNxiYVl9j52YMw8o5bpcnr/ssPjlUrBX?=
 =?us-ascii?Q?qNCvpgW3H4sS6xP3KDT7vcecWwD4a6TKvde8hq4VAI5OMQjWhLExtzybTm9K?=
 =?us-ascii?Q?EsCzBOVLWAIXXD4/7OrzMxkmudYnicmc4i06yluKH/dIwG4Nv1XZ0o2vRa9X?=
 =?us-ascii?Q?5HUjZFRVMxLVlwDhkoNJPHwpSUA9UxD+aeyfz+q3HAX6wMmtb/D0o3WrYqbT?=
 =?us-ascii?Q?0t3sNEazQWTgf+NQ12xvnOqbFR/PJix3Y5u56QyuqFZihrSeTxvIw/aT/l32?=
 =?us-ascii?Q?Q5wuXi1z+HKDPByOtOhxs/GcVev2UgGD4pIt9Xka6nq1ALQ5IB8RGKGL+rrY?=
 =?us-ascii?Q?IJUaNb0iUCOVw+oazMIp+B4jNlC451NWGX5m3HXasaW0ICpqeJRCRR3uJ8Dt?=
 =?us-ascii?Q?jwRReqnmNZccWHqP1Ff5Qz0KPL08Dpb3Y/RuO6PmV/roBiZiyCSWyMkWOngS?=
 =?us-ascii?Q?6nLrwSEAm3H1BLii4VKMEtUML/CqCmgCGaHPyTWiu1eLChb0NP3a4jYbDP9v?=
 =?us-ascii?Q?qIeoPKSamfDWwn2qvrzEheZvgvofFtlGqAdSJzJFeXo6RVNOmUIxzV6kjzek?=
 =?us-ascii?Q?nKPE6z2XelKxZySHvaG+mcdDOdpUs7uMEH6dJ4l3Tua9NhFB8asjH1eltPGr?=
 =?us-ascii?Q?W9t1DmSgMAhZrYnUeyNGBBb5Nw5O94tQ3lDHNjzQROSpi0dFl7QqikwaT85+?=
 =?us-ascii?Q?BPjJhFvMqvaDks2rf3xlUso8i6QhZLldC6FbIcY2GR7a7I4ohScDfhg0HZ+U?=
 =?us-ascii?Q?JHs8gI9S1Dc/aClgihpmRY5JNFN2dUY7U5sBp4l4?=
X-MS-Exchange-CrossTenant-Network-Message-Id: 31258f94-bcc5-4892-9313-08dc1ff68d33
X-MS-Exchange-CrossTenant-AuthSource: MN0PR11MB6304.namprd11.prod.outlook.com
X-MS-Exchange-CrossTenant-AuthAs: Internal
X-MS-Exchange-CrossTenant-OriginalArrivalTime: 28 Jan 2024 11:44:57.4644
 (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: cF6pkkuNT41bSP/JTEL0YFo2MCErrElkCOKANn18ZYjk4R0tEs8J64/DBBX28mPu13rL6aKl0PSyxAWXqchvSg==
X-MS-Exchange-Transport-CrossTenantHeadersStamped: PH8PR11MB7048
X-OriginatorOrg: intel.com

On Fri, Jan 26, 2024 at 11:28:36AM -0800, Paul E. McKenney wrote:
> On Fri, Jan 26, 2024 at 11:19:50AM -0500, Waiman Long wrote:
[...]
> > > > I also suggest doing the adjustment at boot time, for example, using
> > > > an early_initcall().  That way the test code also sees the scaled value.
> > > I also thought about doing the adjustment once in early boot phase
> > > using num_possible_cpus(), but gave up as that parameters could be
> > > changed runtime using sysfs's module parameter interface, and cpu
> > > runtime hotplugging.
> > > 
> > > Since the watchdog timer only happens (if not skipped) every 500 ms,
> > > how about doing the ilog2 math everytime, like below:
> > > 
> > > diff --git a/include/linux/clocksource.h b/include/linux/clocksource.h
> > > index 1d42d4b17327..9104bdecf34e 100644
> > > --- a/include/linux/clocksource.h
> > > +++ b/include/linux/clocksource.h
> > > @@ -291,7 +291,7 @@ static inline void timer_probe(void) {}
> > >   #define TIMER_ACPI_DECLARE(name, table_id, fn)		\
> > >   	ACPI_DECLARE_PROBE_ENTRY(timer, name, table_id, 0, NULL, 0, fn)
> > > -extern ulong max_cswd_read_retries;
> > > +extern long max_cswd_read_retries;
> > >   void clocksource_verify_percpu(struct clocksource *cs);
> > >   #endif /* _LINUX_CLOCKSOURCE_H */
> > > diff --git a/kernel/time/clocksource.c b/kernel/time/clocksource.c
> > > index c108ed8a9804..867bb36e6dad 100644
> > > --- a/kernel/time/clocksource.c
> > > +++ b/kernel/time/clocksource.c
> > > @@ -208,8 +208,8 @@ void clocksource_mark_unstable(struct clocksource *cs)
> > >   	spin_unlock_irqrestore(&watchdog_lock, flags);
> > >   }
> > > -ulong max_cswd_read_retries = 2;
> > > -module_param(max_cswd_read_retries, ulong, 0644);
> > > +long max_cswd_read_retries = -1;
> > > +module_param(max_cswd_read_retries, long, 0644);
> > >   EXPORT_SYMBOL_GPL(max_cswd_read_retries);
> > >   static int verify_n_cpus = 8;
> > >   module_param(verify_n_cpus, int, 0644);
> > > @@ -225,8 +225,17 @@ static enum wd_read_status cs_watchdog_read(struct clocksource *cs, u64 *csnow,
> > >   	unsigned int nretries;
> > >   	u64 wd_end, wd_end2, wd_delta;
> > >   	int64_t wd_delay, wd_seq_delay;
> > > +	long max_retries = max_cswd_read_retries;
> > > +	
> > > +	if (max_cswd_read_retries <= 0) {
> > > +		if (max_cswd_read_retries != -1)
> > > +			pr_warn_once("max_cswd_read_retries has been set a invalid number: %d\n",
> > > +				max_cswd_read_retries);
> > > -	for (nretries = 0; nretries <= max_cswd_read_retries; nretries++) {
> > > +		max_retries = ilog2(num_online_cpus()) + 1;
> > > +	} 		
> > > +
> > > +	for (nretries = 0; nretries <= max_retries; nretries++) {
> > >   		local_irq_disable();
> > >   		*wdnow = watchdog->read(watchdog);
> > >   		*csnow = cs->read(cs);
> > > @@ -238,7 +247,7 @@ static enum wd_read_status cs_watchdog_read(struct clocksource *cs, u64 *csnow,
> > >   		wd_delay = clocksource_cyc2ns(wd_delta, watchdog->mult,
> > >   					      watchdog->shift);
> > >   		if (wd_delay <= WATCHDOG_MAX_SKEW) {
> > > -			if (nretries > 1 || nretries >= max_cswd_read_retries) {
> > > +			if (nretries > 1 || nretries >= max_retries) {
> > >   				pr_warn("timekeeping watchdog on CPU%d: %s retried %d times before success\n",
> > >   					smp_processor_id(), watchdog->name, nretries);
> > >   			}
> > 
> > The max_cswd_read_retries value is also used in
> > kernel/time/clocksource-wdtest.c. You will have to apply similar logic to
> > clocksource-wdtest.c if it is not done once in early_init.
> 
> Good point!  If it is not done in an early_init(), could we please
> have a function for the common code?

Thanks Waiman for the catch! And sure, will add a new helper function
for that.

Thanks,
Feng

> 
> 							Thanx, Paul