RE: [Xen-ia64-devel] [PATCH] pal_halt_light emulatefor domU TAKE3

["Xu, Anthony" <anthony.xu@xxxxxxxxx>]

I agree with Yamahata partially,
If I'm right, credit scheduler only schedules runnable vcpus,
So when context_switch is called, the scheduled in vcpu's hlt_timer must be 
Stopped, it is unnecessary to call migrate_timer in context_switch.
My suggestion is like following,

@@ -233,7 +233,10 @@ fw_hypercall (struct pt_regs *regs)
                        }
                        else {
                                perfc_incrc(pal_halt_light);
-                               do_sched_op_compat(SCHEDOP_yield, 0);
                <<<<<<< v->arch.hlt_timer.cpu=v->processor;>>>>>>>>>>>>>>>>>
+                               set_timer(&v->arch.hlt_timer,
+                                       vcpu_get_next_timer_ns(v));
+                               do_sched_op_compat(SCHEDOP_block, 0);
+                               stop_timer(&v->arch.hlt_timer);
                        }
                        regs->r8 = 0;
                        regs->r9 = 0;
>-----Original Message-----
>From: xen-ia64-devel-bounces@xxxxxxxxxxxxxxxxxxx
>[mailto:xen-ia64-devel-bounces@xxxxxxxxxxxxxxxxxxx] On Behalf Of Atsushi
>SAKAI
>Sent: 2006?8?23? 20:31
>To: Isaku Yamahata
>Cc: xen-ia64-devel@xxxxxxxxxxxxxxxxxxx
>Subject: Re: [Xen-ia64-devel] [PATCH] pal_halt_light emulatefor domU TAKE3
>
>Hi, Isaku
>
>  Thank you for your suggestion.
>I decided the location of migrate_timer function
>by seeing the other migrate_timer calling codes in Xen.
>I think your modification changes negiligible small performance effect.
>
>
>This migrate_timer purposes to solve
>the problem of timer is up during set_timer/do_block (vcpu hangs).
>
>So, the candidate location of migrate_timer is
>from context_switch (vcpu in) to pal_halt_light (vcpu out),
>(see below figure)
>because these positions are before set_timer/do_block.
>I select context_switch position because I think it is conservative (see line
>2).
>
>     vcpu in    ==================> pal_halt_light => set_timer/do_block(vcpu
>out)
>(context_switch)^^^^^^^^^^^^^^^^^|
>               candidate position|
>               of migrate timer  |
>                                 |========>by timer (vcpu out)
>
>Anyway, for considering performance issue, I consider 2 cases.
>1)CPU intensive process is rarely occur the context switch.
>  (Only timer interrupt occurs)
>  In this case, So performance is negligible for this function in 
> context_switch
>10^{-6}.
>
>2)For Heavy I/O issue, pal_halt_light frequently signals.
>  So position of migrate_timer in context_switch and pal_halt_light
>  is very small.
>  It means the number of context_switch and pal_halt_light event closes.
>
>In these two cases, migrate_timer location is not important for performance.
>
>
>Do you still think on moving this function?
>
>Anyway, thank you for your comments.
>It is clearify my idea by writing document.
>
>Thanks,
>Atsushi SAKAI
>
>
>
>>
>>Calling migrate_timer from context_switch() seems to introduce
>>unnecessary overhead.
>>Why did you choose to insert migrate_timer() to context_switch()
>>instead of inserting it ot the following position?
>>
>>diff -r 8c6bb45901e7 xen/arch/ia64/xen/hypercall.c
>>--- a/xen/arch/ia64/xen/hypercall.c     Wed Aug 16 14:28:57 2006 -0600
>>+++ b/xen/arch/ia64/xen/hypercall.c     Mon Aug 21 13:46:05 2006 +0900
>>@@ -233,7 +233,10 @@ fw_hypercall (struct pt_regs *regs)
>>                        }
>>                        else {
>>                                perfc_incrc(pal_halt_light);
>>-                               do_sched_op_compat(SCHEDOP_yield, 0);
>>    <<<<<<<<<<<<< migrate_timer() >>>>>>>>>>>>>>>>>>>>>>>>>>>
>>+                               set_timer(&v->arch.hlt_timer,
>>+                                       vcpu_get_next_timer_ns(v));
>>+                               do_sched_op_compat(SCHEDOP_block, 0);
>>+                               stop_timer(&v->arch.hlt_timer);
>>                        }
>>                        regs->r8 = 0;
>>                        regs->r9 = 0;
>>
>>
>>On Wed, Aug 23, 2006 at 07:29:11PM +0900, Atsushi SAKAI wrote:
>>> Hi, Isaku
>>>
>>> Sorry for confusing.
>>> It should replace from "for context_switch" to "to context_switch"
>>> migrate_timer is in context_switch.
>>>
>>> Thanks
>>> Atsushi
>>>
>>> >Hi Atsushi.
>>> >
>>> >On Wed, Aug 23, 2006 at 05:48:15PM +0900, Atsushi SAKAI wrote:
>>> >
>>> >> 1)migrate_timer for hlt_timer_fn is added for context_switch
>>> >>   This makes correct pCPU work for timer.
>>> >
>>> >Is it necessary to call migrate_timer() every context switch
>>> >instead of calling it right before set_timer(&hlt_timer)?
>>> >
>>> >--
>>> >yamahata
>>> >
>>>
>>>
>>>
>>>
>>>
>>>
>>>
>>> _______________________________________________
>>> Xen-ia64-devel mailing list
>>> Xen-ia64-devel@xxxxxxxxxxxxxxxxxxx
>>> http://lists.xensource.com/xen-ia64-devel
>>
>>--
>>yamahata
>>
>
>
>
>
>
>
>
>_______________________________________________
>Xen-ia64-devel mailing list
>Xen-ia64-devel@xxxxxxxxxxxxxxxxxxx
>http://lists.xensource.com/xen-ia64-devel

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