linux: {start, stop}_hz_timer() not really affecting periodic timer?

linux: {start, stop}_hz_timer() not really affecting periodic timer?

[Jan Beulich]

Shouldn't these two functions call VCPUOP_set_periodic_timer/
VCPUOP_stop_periodic_timer to actually do what their names promise?

And shouldn't the call to VCPUOP_set_singleshot_timer permit -ETIME
without BUG()?

Jan

Re: linux: {start, stop}_hz_timer() not really affecting periodic timer?

[Keir Fraser]

No, no, and no. :-)

start/stop_hz_timer() refer to Linux's own hz ticker. Xen does not deliver
periodic ticks when a guest is descheduled, so the Xen side of things is
implicitly handled already. There is no need for start/stop_hz_timer to
execute hypercalls to enact this.

The call to VCPUOP_set_singleshot_timer cannot return -ETIME because the
kernel does not specify the VCPU_SSHOTTMR_future flag.

All this assume you are looking at linux-2.6.18-xen.hg.

 -- Keir

On 16/1/08 14:32, "Jan Beulich" <jbeulich@novell.com> wrote:

> Shouldn't these two functions call VCPUOP_set_periodic_timer/
> VCPUOP_stop_periodic_timer to actually do what their names promise?
> 
> And shouldn't the call to VCPUOP_set_singleshot_timer permit -ETIME
> without BUG()?
> 
> Jan
> 
> 
> _______________________________________________
> Xen-devel mailing list
> Xen-devel@lists.xensource.com
> http://lists.xensource.com/xen-devel

Re: linux: {start, stop}_hz_timer() not really affecting periodic timer?

[Jan Beulich]

>>> Keir Fraser <Keir.Fraser@cl.cam.ac.uk> 16.01.08 15:37 >>>
>No, no, and no. :-)
>
>start/stop_hz_timer() refer to Linux's own hz ticker. Xen does not deliver
>periodic ticks when a guest is descheduled, so the Xen side of things is
>implicitly handled already. There is no need for start/stop_hz_timer to
>execute hypercalls to enact this.

Okay, okay, I didn't pay attention to this. But then
VCPU_stop_periodic_timer seems a rather academic operation?

>The call to VCPUOP_set_singleshot_timer cannot return -ETIME because the
>kernel does not specify the VCPU_SSHOTTMR_future flag.

I noticed this after pushing the send button. Nevertheless, the whole
construct in stop_hz_timer() seems to assume that it is called with
interrupts disabled, which might be the case now but nothing enforces
xen_safe_halt() to only be called in such contexts... For that reason it
would seem safer to set the flag, check for -ETIME, and avoid
HYPERVISOR_block() altogether in that case.

>All this assume you are looking at linux-2.6.18-xen.hg.

I am, with one eye.

Jan

Re: linux: {start, stop}_hz_timer() not really affecting periodic timer?

[Keir Fraser]

On 16/1/08 15:50, "Jan Beulich" <jbeulich@novell.com> wrote:

>> start/stop_hz_timer() refer to Linux's own hz ticker. Xen does not deliver
>> periodic ticks when a guest is descheduled, so the Xen side of things is
>> implicitly handled already. There is no need for start/stop_hz_timer to
>> execute hypercalls to enact this.
> 
> Okay, okay, I didn't pay attention to this. But then
> VCPU_stop_periodic_timer seems a rather academic operation?

The default periodic timer is 10ms. If a guest does not want a periodic
timer at all, it can use VCPU_stop_periodic_timer. Clearly this is not
applicable to Linux.

>> The call to VCPUOP_set_singleshot_timer cannot return -ETIME because the
>> kernel does not specify the VCPU_SSHOTTMR_future flag.
> 
> I noticed this after pushing the send button. Nevertheless, the whole
> construct in stop_hz_timer() seems to assume that it is called with
> interrupts disabled, which might be the case now but nothing enforces
> xen_safe_halt() to only be called in such contexts... For that reason it
> would seem safer to set the flag, check for -ETIME, and avoid
> HYPERVISOR_block() altogether in that case.

If the time is in the past then the singleshot timer will fire immediately.
So you'll take a slower path than necessary, but the code as-is will work
fine.

 -- Keir

Re: linux: {start, stop}_hz_timer() not really affecting periodic timer?

[Jan Beulich]

>>> The call to VCPUOP_set_singleshot_timer cannot return -ETIME because the
>>> kernel does not specify the VCPU_SSHOTTMR_future flag.
>> 
>> I noticed this after pushing the send button. Nevertheless, the whole
>> construct in stop_hz_timer() seems to assume that it is called with
>> interrupts disabled, which might be the case now but nothing enforces
>> xen_safe_halt() to only be called in such contexts... For that reason it
>> would seem safer to set the flag, check for -ETIME, and avoid
>> HYPERVISOR_block() altogether in that case.
>
>If the time is in the past then the singleshot timer will fire immediately.
>So you'll take a slower path than necessary, but the code as-is will work
>fine.

Immediately would mean to me that it would fire on the return path
from VCPUOP_set_singleshot_timer, so HYPERVISOR_block() would
not (necessarily) find any pending events and hence block when it
shouldn't. Or am I missing some magic by which this is being avoided?

Jan

Re: linux: {start, stop}_hz_timer() not really affecting periodic timer?

[Keir Fraser]

On 16/1/08 16:36, "Jan Beulich" <jbeulich@novell.com> wrote:

>> If the time is in the past then the singleshot timer will fire immediately.
>> So you'll take a slower path than necessary, but the code as-is will work
>> fine.
> 
> Immediately would mean to me that it would fire on the return path
> from VCPUOP_set_singleshot_timer, so HYPERVISOR_block() would
> not (necessarily) find any pending events and hence block when it
> shouldn't. Or am I missing some magic by which this is being avoided?

The function should only be called with interrupts disabled.

 -- Keir

Re: linux: {start, stop}_hz_timer() not really affecting periodic timer?

[Jan Beulich]

>>> Keir Fraser <Keir.Fraser@cl.cam.ac.uk> 16.01.08 17:46 >>>
>On 16/1/08 16:36, "Jan Beulich" <jbeulich@novell.com> wrote:
>
>>> If the time is in the past then the singleshot timer will fire immediately.
>>> So you'll take a slower path than necessary, but the code as-is will work
>>> fine.
>> 
>> Immediately would mean to me that it would fire on the return path
>> from VCPUOP_set_singleshot_timer, so HYPERVISOR_block() would
>> not (necessarily) find any pending events and hence block when it
>> shouldn't. Or am I missing some magic by which this is being avoided?
>
>The function should only be called with interrupts disabled.

That goes back to what I stated first - there's nothing really requiring
this, it just happens to be that way at present.

Jan

Re: linux: {start, stop}_hz_timer() not really affecting periodic timer?

[Keir Fraser]

On 16/1/08 17:17, "Jan Beulich" <jbeulich@novell.com> wrote:

>>> Immediately would mean to me that it would fire on the return path
>>> from VCPUOP_set_singleshot_timer, so HYPERVISOR_block() would
>>> not (necessarily) find any pending events and hence block when it
>>> shouldn't. Or am I missing some magic by which this is being avoided?
>> 
>> The function should only be called with interrupts disabled.
> 
> That goes back to what I stated first - there's nothing really requiring
> this, it just happens to be that way at present.

If you don't do this then the function doesn't work properly, so in that
sense it is a requirement! If you want it more explicit add a comment or
maybe even a BUG_ON().

 -- Keir