Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 23.128.96.18 as permitted sender) client-ip=23.128.96.18;
Date:   Tue, 1 Dec 2020 13:35:40 -0400
From:   Jason Gunthorpe <jgg@ziepe.ca>
To:     Miroslav Lichvar <mlichvar@redhat.com>
Cc:     linux-kernel@vger.kernel.org, Thomas Gleixner <tglx@linutronix.de>,
        John Stultz <john.stultz@linaro.org>,
        Prarit Bhargava <prarit@redhat.com>
Subject: Re: [PATCH] rtc: adapt allowed RTC update error
Message-ID: <20201201173540.GH5487@ziepe.ca>
References: <20201201143835.2054508-1-mlichvar@redhat.com>
 <20201201161224.GF5487@ziepe.ca>
 <20201201171420.GN1900232@localhost>
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <20201201171420.GN1900232@localhost>
Precedence: bulk

On Tue, Dec 01, 2020 at 06:14:20PM +0100, Miroslav Lichvar wrote:
> On Tue, Dec 01, 2020 at 12:12:24PM -0400, Jason Gunthorpe wrote:
> > On Tue, Dec 01, 2020 at 03:38:35PM +0100, Miroslav Lichvar wrote:
> > > +	unsigned long time_set_nsec_fuzz;
> > > +	static unsigned int attempt;
> > 
> > Adding a static value instide a static inline should not be done
> 
> Well, grepping through the other header files in include/linux, this
> would not be the first case.

Nevertheless, it should be avoided and has surprising behaviors.
 
> > I'm not sure using a static like this is the best idea anyhow, if you
> > want something like this it should be per-rtc, not global
>
> If there are multiple RTCs, are they all updated in this 11-minute
> sync?

Yes, but they have different offsets and thus different timers,
IIRC. Though only one gets to be the CONFIG_RTC_SYSTOHC_DEVICE

If you are going to put some static it is clearer to put it along side
the sync_rtc_clock()

> I found no good explanation. It seems to depend on what system is
> doing, if it's idle, etc. I suspect it's a property of the workqueues
> that they cannot generally guarantee the jobs to run exactly on time.
> I tried switching to the non-power-efficient and high priority
> workqueues and it didn't seem to make a big difference.

When I wrote it originally the workqueues got increasingly inaccurate
the longer the duration, so it does a very short sleep to get back on
track.

If you are missing that time target it must be constantly scheduling
new delayed work and none of it hits the target for long, long time
periods? This seems like a more fundamental problem someplace else in
the kernel.

Jason