Re: [PATCH] x86/xen: Add "xen_timer_slop" command line option

From: Ryan Thibodeaux <thibodux@gmail.com>
To: luca abeni <luca.abeni@santannapisa.it>
Cc: Dario Faggioli <dfaggioli@suse.com>,
	Boris Ostrovsky <boris.ostrovsky@oracle.com>,
	xen-devel@lists.xenproject.org, linux-kernel@vger.kernel.org,
	oleksandr_andrushchenko@epam.com, tglx@linutronix.de,
	jgross@suse.com, ryan.thibodeaux@starlab.io
Subject: Re: [PATCH] x86/xen: Add "xen_timer_slop" command line option
Date: Tue, 26 Mar 2019 07:41:32 -0400	[thread overview]
Message-ID: <20190326114132.GA85724@centos-dev.localdomain> (raw)
In-Reply-To: <20190326121256.76ceed71@luca64>

On Tue, Mar 26, 2019 at 12:12:56PM +0100, luca abeni wrote:
> Hi all,
> 
> On Tue, 26 Mar 2019 10:13:32 +0100
> Dario Faggioli <dfaggioli@suse.com> wrote:
> 
> > On Mon, 2019-03-25 at 09:43 -0400, Boris Ostrovsky wrote:
> > > On 3/25/19 8:05 AM, luca abeni wrote:  
> > > > 
> > > > The picture shows the latencies measured with an unpatched guest
> > > > kernel
> > > > and with a guest kernel having TIMER_SLOP set to 1000 (arbitrary
> > > > small
> > > > value :).
> > > > All the experiments have been performed booting the hypervisor
> > > > with a
> > > > small timer_slop (the hypervisor's one) value. So, they show that
> > > > decreasing the hypervisor's timer_slop is not enough to measure
> > > > low latencies with cyclictest.  
> > > 
> > > I have a couple of questions:
> > > * Does it make sense to make this a tunable for other clockevent
> > > devices
> > > as well?
> > >  
> > So, AFAIUI, the thing is as follows. In clockevents_program_event(),
> > we keep the delta between now and the next timer event within
> > dev->max_delta_ns and dev->min_delta_ns:
> > 
> >   delta = min(delta, (int64_t) dev->max_delta_ns);
> >   delta = max(delta, (int64_t) dev->min_delta_ns);
> > 
> > For Xen (well, for the Xen clock) we have:
> > 
> >   .max_delta_ns = 0xffffffff,
> >   .min_delta_ns = TIMER_SLOP,
> > 
> > which means a guest can't ask for a timer to fire earlier than 100us
> [...]
> 
> I know this is not fully related with the current discussion, but in
> these days I had a look at the code again, and...
> The comment for TIMER_SLOP in arch/x86/xen/time.c says:
> 	/* Xen may fire a timer up to this many ns early */
> 
> Isn't the comment wrong? shouldn't it be "...many ns late" instead of
> "early"?

I would say is something else entirely. 

If you look at "clockevents_program_event()" in kernel/time/clockevents.c,
you see that the min_delta_ns value sets the limit or granulariy for the
clock's sleep time.

Basically, it is the minimum amount of sleep one can set for the next
event for the clock in question.

- Ryan