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

[Boris Ostrovsky]

On 3/25/19 10:11 AM, Ryan Thibodeaux wrote:
> On Mon, Mar 25, 2019 at 09:43:20AM -0400, Boris Ostrovsky wrote:
>> On 3/25/19 8:05 AM, luca abeni wrote:
>>> Hi all,
>>>
>>> On Sat, 23 Mar 2019 11:41:51 +0100
>>> luca abeni <luca.abeni@xxxxxxxxxxxxxxx> wrote:
>>> [...]
>>>>>> Is there any data that shows effects of using this new parameter?
>>>>>>     
>>>>> Yes, I've done some research and experiments on this. I did it
>>>>> together with a friend, which I'm Cc-ing, as I'm not sure we're
>>>>> ready/capable to share the results, yet (Luca?).  
>>>> I think we can easily share the experimental data (cyclictest output
>>>> and plots).
>>>>
>>>> Moreover, we can share the scripts and tools for running the
>>>> experiments (so, everyone can easily reproduce the numbers by simply
>>>> typing "make" and waiting for some time :)
>>>>
>>>>
>>>> I'll try to package the results and the scripts/tools this evening,
>>>> and I'll send them.
>>> Sorry for the delay. I put some quick results here:
>>> http://retis.santannapisa.it/luca/XenTimers/
>>> (there also is a link to the scripts to be used for reproducing the
>>> results). The latencies have been measured by running cyclictest in the
>>> guest (see the scripts for details).
>>>
>>> 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?
> I gather that would be on a case-by-case basis for very specific 
> ones.
>
> For many timers in the kernel, the minimums are determined by the
> actual hardware  backing the timer, and the minimum can be
> adjusted by the clockevent code. This case is special since it 
> is entirely a software-based implementation in the kernel, where 
> the actual timer implementation is in the Xen hypervisor.
>
>> * This patch adjusts min value. Could max value (ever) need a similar
>> adjustment?
> I cannot think of such a case where that would be helpful, but I
> cannot rule that out or speak as an authority.


I am asking mostly because you are introducing new interface and I don't
want it to change in the future. I suppose if later we decide to add
control for the max value we could just expand your current proposal to
xen_timer_slop=[min],[max] and keep it to be back-compatible.

For the patch:

Reviewed-by: Boris Ostrovsky <boris.ostrovsky@xxxxxxxxxx>