Xen devel - Sep 2009 - [PATCH] x86 hvm: fix missing ticks issue in c/s 20218

Although c/s 20229 fixes missing ticks issue in c/s 20218,
it''s not enough.

- set a timer to the next period more straightforwardly.
- fix an unexpected behavior of both timer_mode=2 and timer_mode=3.
  extra interrupts might be delivered to a guest.

Signed-off-by: Kouya Shimura <kouya@jp.fujitsu.com>



_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xensource.com
http://lists.xensource.com/xen-devel

On Friday 25 September 2009 06:37:41 Kouya Shimura
wrote:
> Although c/s 20229 fixes missing ticks issue in c/s 20218,
> it''s not enough.
>
> - set a timer to the next period more straightforwardly.
> - fix an unexpected behavior of both timer_mode=2 and timer_mode=3.
>   extra interrupts might be delivered to a guest.
>
> Signed-off-by: Kouya Shimura <kouya@jp.fujitsu.com>
I can confirm, this patch works.

Christoph

-- 
---to satisfy European Law for business letters:
Advanced Micro Devices GmbH
Karl-Hammerschmidt-Str. 34, 85609 Dornach b. Muenchen
Geschaeftsfuehrer: Andrew Bowd, Thomas M. McCoy, Giuliano Meroni
Sitz: Dornach, Gemeinde Aschheim, Landkreis Muenchen
Registergericht Muenchen, HRB Nr. 43632


_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xensource.com
http://lists.xensource.com/xen-devel

This patch is very dubious. It appears to break pt_process_missed_ticks()
logic for timer modes 0 and 1. When a VCPU gets rescheduled it will have
missed ticks folded into ->scheduled and ->pending_intr_nr fields, but
since
you stomp on the logic for tracking pending_intr_nr, most of those missed
ticks will never get delivered.

I think the fix is to move the line ''pt->scheduled +=
pt->period'' out of
pt_intr_post() and back into pt_timer_fn(). This is because you have
otherwise broken the invariant that pt->scheduled always gets incremented
along with pt->pending_intr_nr. All the rest of your
''fixes'' are I think
merely working around that fundamental brokeness.

Please try moving that one line and see if the bug goes away.

 -- Keir
 
On 25/09/2009 05:37, "Kouya Shimura" <kouya@jp.fujitsu.com>
wrote:
> Although c/s 20229 fixes missing ticks issue in c/s 20218,
> it''s not enough.
> 
> - set a timer to the next period more straightforwardly.
> - fix an unexpected behavior of both timer_mode=2 and timer_mode=3.
>   extra interrupts might be delivered to a guest.
> 
> Signed-off-by: Kouya Shimura <kouya@jp.fujitsu.com>
> 


_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xensource.com
http://lists.xensource.com/xen-devel

Keir Fraser writes:
> This patch is very dubious. It appears to break pt_process_missed_ticks()
> logic for timer modes 0 and 1. When a VCPU gets rescheduled it will have
> missed ticks folded into ->scheduled and ->pending_intr_nr fields,
but since
> you stomp on the logic for tracking pending_intr_nr, most of those missed
> ticks will never get delivered.
I''ve forgotten VCPU switch. Indeed, it''s pretty fragile. :-)
What about modifying pt_restore_timer() not to advance pt->scheduled?
For example,

diff -r 0f8376078dc1 xen/arch/x86/hvm/vpt.c
--- a/xen/arch/x86/hvm/vpt.c	Wed Sep 23 18:19:30 2009 +0100
+++ b/xen/arch/x86/hvm/vpt.c	Fri Sep 25 18:12:12 2009 +0900
@@ -189,8 +189,10 @@ void pt_restore_timer(struct vcpu *v)
     {
         if ( pt->pending_intr_nr == 0 )
         {
-            pt_process_missed_ticks(pt);
-            set_timer(&pt->timer, pt->scheduled);
+            if ( pt->scheduled > NOW() )
+                set_timer(&pt->timer, pt->scheduled);
+            else
+                pt->pending_intr_nr = 1;
         }
     }

We should rename pending_intr_nr to pending_intr(boolean).

Thanks,
Kouya
> I think the fix is to move the line ''pt->scheduled +=
pt->period'' out of
> pt_intr_post() and back into pt_timer_fn(). This is because you have
> otherwise broken the invariant that pt->scheduled always gets
incremented
> along with pt->pending_intr_nr. All the rest of your
''fixes'' are I think
> merely working around that fundamental brokeness.
> 
> Please try moving that one line and see if the bug goes away.
> 
>  -- Keir
>  
> On 25/09/2009 05:37, "Kouya Shimura" <kouya@jp.fujitsu.com>
wrote:
> 
> > Although c/s 20229 fixes missing ticks issue in c/s 20218,
> > it''s not enough.
> > 
> > - set a timer to the next period more straightforwardly.
> > - fix an unexpected behavior of both timer_mode=2 and timer_mode=3.
> >   extra interrupts might be delivered to a guest.
> > 
> > Signed-off-by: Kouya Shimura <kouya@jp.fujitsu.com>
> > 
> 
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xensource.com
http://lists.xensource.com/xen-devel

On 25/09/2009 10:32, "Kouya Shimura" <kouya@jp.fujitsu.com>
wrote:
> I''ve forgotten VCPU switch. Indeed, it''s pretty fragile.
:-)
> What about modifying pt_restore_timer() not to advance pt->scheduled?
> We should rename pending_intr_nr to pending_intr(boolean).
It''s not clear to me that the old logic around pt->pending_intr_nr
and
pt->scheduled really needs to change that drastically. The more we mess with
this code the more likely we are to break stuff, as it''s hard to get
good
coverage of the timer_modes and various ways guest Oses may manage their
timers.

I would prefer to see whether moving the update of pt->scheduled out of
pt_intr_post() and back into the timer_fn() works. That moves us back
towards what we had before your patches, and therefore I''m more
comfortable
with it.

 -- Keir



_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xensource.com
http://lists.xensource.com/xen-devel

Keir, 
Okay, attached is a new patch per your advice. 
I tested it carefully. It works for me.

Christoph,
Could you please test it?

Thanks,
Kouya

Keir Fraser writes:
> On 25/09/2009 10:32, "Kouya Shimura" <kouya@jp.fujitsu.com>
wrote:
> 
> > I''ve forgotten VCPU switch. Indeed, it''s pretty
fragile. :-)
> > What about modifying pt_restore_timer() not to advance
pt->scheduled?
> > We should rename pending_intr_nr to pending_intr(boolean).
> 
> It''s not clear to me that the old logic around
pt->pending_intr_nr and
> pt->scheduled really needs to change that drastically. The more we mess
with
> this code the more likely we are to break stuff, as it''s hard to
get good
> coverage of the timer_modes and various ways guest Oses may manage their
> timers.
> 
> I would prefer to see whether moving the update of pt->scheduled out of
> pt_intr_post() and back into the timer_fn() works. That moves us back
> towards what we had before your patches, and therefore I''m more
comfortable
> with it.
> 
>  -- Keir
> 
Signed-off-by: Kouya Shimura <kouya@jp.fujitsu.com>



_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xensource.com
http://lists.xensource.com/xen-devel

On Friday 25 September 2009 14:25:11 Kouya Shimura
wrote:
> Keir,
> Okay, attached is a new patch per your advice.
> I tested it carefully. It works for me.
>
> Christoph,
> Could you please test it?
Works for me. Thanks for fixing it.

Christoph

> Thanks,
> Kouya
>
> Keir Fraser writes:
> > On 25/09/2009 10:32, "Kouya Shimura"
<kouya@jp.fujitsu.com> wrote:
> > > I''ve forgotten VCPU switch. Indeed, it''s pretty
fragile. :-)
> > > What about modifying pt_restore_timer() not to advance
pt->scheduled?
> > > We should rename pending_intr_nr to pending_intr(boolean).
> >
> > It''s not clear to me that the old logic around
pt->pending_intr_nr and
> > pt->scheduled really needs to change that drastically. The more we
mess
> > with this code the more likely we are to break stuff, as it''s
hard to get
> > good coverage of the timer_modes and various ways guest Oses may
manage
> > their timers.
> >
> > I would prefer to see whether moving the update of pt->scheduled
out of
> > pt_intr_post() and back into the timer_fn() works. That moves us back
> > towards what we had before your patches, and therefore I''m
more
> > comfortable with it.
> >
> >  -- Keir
>
> Signed-off-by: Kouya Shimura <kouya@jp.fujitsu.com>


-- 
---to satisfy European Law for business letters:
Advanced Micro Devices GmbH
Karl-Hammerschmidt-Str. 34, 85609 Dornach b. Muenchen
Geschaeftsfuehrer: Andrew Bowd, Thomas M. McCoy, Giuliano Meroni
Sitz: Dornach, Gemeinde Aschheim, Landkreis Muenchen
Registergericht Muenchen, HRB Nr. 43632


_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xensource.com
http://lists.xensource.com/xen-devel

Applied as c/s 20255. I messed with it some more, so perhaps worth checking
the bug still is gone. ;-)

 -- Keir

On 25/09/2009 13:25, "Kouya Shimura" <kouya@jp.fujitsu.com>
wrote:
> Keir, 
> Okay, attached is a new patch per your advice.
> I tested it carefully. It works for me.
> 
> Christoph,
> Could you please test it?
> 
> Thanks,
> Kouya
> 
> Keir Fraser writes:
>> On 25/09/2009 10:32, "Kouya Shimura"
<kouya@jp.fujitsu.com> wrote:
>> 
>>> I''ve forgotten VCPU switch. Indeed, it''s pretty
fragile. :-)
>>> What about modifying pt_restore_timer() not to advance
pt->scheduled?
>>> We should rename pending_intr_nr to pending_intr(boolean).
>> 
>> It''s not clear to me that the old logic around
pt->pending_intr_nr and
>> pt->scheduled really needs to change that drastically. The more we
mess with
>> this code the more likely we are to break stuff, as it''s hard
to get good
>> coverage of the timer_modes and various ways guest Oses may manage
their
>> timers.
>> 
>> I would prefer to see whether moving the update of pt->scheduled out
of
>> pt_intr_post() and back into the timer_fn() works. That moves us back
>> towards what we had before your patches, and therefore I''m
more comfortable
>> with it.
>> 
>>  -- Keir
>> 
> 
> Signed-off-by: Kouya Shimura <kouya@jp.fujitsu.com>
> 


_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xensource.com
http://lists.xensource.com/xen-devel

On Friday 25 September 2009 16:22:39 Keir Fraser wrote:
> Applied as c/s 20255. I messed with it some more, so perhaps worth checking
> the bug still is gone. ;-)
I retested and the bug is still gone.

Christoph
>  -- Keir
>
> On 25/09/2009 13:25, "Kouya Shimura" <kouya@jp.fujitsu.com>
wrote:
> > Keir,
> > Okay, attached is a new patch per your advice.
> > I tested it carefully. It works for me.
> >
> > Christoph,
> > Could you please test it?
> >
> > Thanks,
> > Kouya
> >
> > Keir Fraser writes:
> >> On 25/09/2009 10:32, "Kouya Shimura"
<kouya@jp.fujitsu.com> wrote:
> >>> I''ve forgotten VCPU switch. Indeed, it''s
pretty fragile. :-)
> >>> What about modifying pt_restore_timer() not to advance
pt->scheduled?
> >>> We should rename pending_intr_nr to pending_intr(boolean).
> >>
> >> It''s not clear to me that the old logic around
pt->pending_intr_nr and
> >> pt->scheduled really needs to change that drastically. The more
we mess
> >> with this code the more likely we are to break stuff, as
it''s hard to
> >> get good coverage of the timer_modes and various ways guest Oses
may
> >> manage their timers.
> >>
> >> I would prefer to see whether moving the update of
pt->scheduled out of
> >> pt_intr_post() and back into the timer_fn() works. That moves us
back
> >> towards what we had before your patches, and therefore
I''m more
> >> comfortable with it.
> >>
> >>  -- Keir
> >
> > Signed-off-by: Kouya Shimura <kouya@jp.fujitsu.com>


-- 
---to satisfy European Law for business letters:
Advanced Micro Devices GmbH
Karl-Hammerschmidt-Str. 34, 85609 Dornach b. Muenchen
Geschaeftsfuehrer: Andrew Bowd, Thomas M. McCoy, Giuliano Meroni
Sitz: Dornach, Gemeinde Aschheim, Landkreis Muenchen
Registergericht Muenchen, HRB Nr. 43632


_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xensource.com
http://lists.xensource.com/xen-devel

c/s 20255 passed my test, too.

Thanks a lot,
Kouya

Christoph Egger writes:
> On Friday 25 September 2009 16:22:39 Keir Fraser wrote:
> > Applied as c/s 20255. I messed with it some more, so perhaps worth
checking
> > the bug still is gone. ;-)
> 
> I retested and the bug is still gone.
> 
> Christoph
> 
> >  -- Keir
> >
> > On 25/09/2009 13:25, "Kouya Shimura"
<kouya@jp.fujitsu.com> wrote:
> > > Keir,
> > > Okay, attached is a new patch per your advice.
> > > I tested it carefully. It works for me.
> > >
> > > Christoph,
> > > Could you please test it?
> > >
> > > Thanks,
> > > Kouya
> > >
> > > Keir Fraser writes:
> > >> On 25/09/2009 10:32, "Kouya Shimura"
<kouya@jp.fujitsu.com> wrote:
> > >>> I''ve forgotten VCPU switch. Indeed,
it''s pretty fragile. :-)
> > >>> What about modifying pt_restore_timer() not to advance
pt->scheduled?
> > >>> We should rename pending_intr_nr to
pending_intr(boolean).
> > >>
> > >> It''s not clear to me that the old logic around
pt->pending_intr_nr and
> > >> pt->scheduled really needs to change that drastically. The
more we mess
> > >> with this code the more likely we are to break stuff, as
it''s hard to
> > >> get good coverage of the timer_modes and various ways guest
Oses may
> > >> manage their timers.
> > >>
> > >> I would prefer to see whether moving the update of
pt->scheduled out of
> > >> pt_intr_post() and back into the timer_fn() works. That moves
us back
> > >> towards what we had before your patches, and therefore
I''m more
> > >> comfortable with it.
> > >>
> > >>  -- Keir
> > >
> > > Signed-off-by: Kouya Shimura <kouya@jp.fujitsu.com>
> 
> 
> 
> -- 
> ---to satisfy European Law for business letters:
> Advanced Micro Devices GmbH
> Karl-Hammerschmidt-Str. 34, 85609 Dornach b. Muenchen
> Geschaeftsfuehrer: Andrew Bowd, Thomas M. McCoy, Giuliano Meroni
> Sitz: Dornach, Gemeinde Aschheim, Landkreis Muenchen
> Registergericht Muenchen, HRB Nr. 43632
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xensource.com
http://lists.xensource.com/xen-devel