Re: [Xen-ia64-devel] [PATCH][3/3] Steal time accounting for PVdomain/IA64 TAKE2

[Alex Williamson <alex.williamson@xxxxxx>]

Hi,

   Couple comments...

On Wed, 2007-03-07 at 19:04 +0900, Atsushi SAKAI wrote:
> +       if(stolen > 0) {
> +               do_div(stolen, NS_PER_TICK);
> +               for(i=0;i<stolen;i++){
> +                       if (time_after(delta_itm + new_itm,
> ia64_get_itc()))
> +                               break;
> +                       account_steal_time(NULL,
> jiffies_to_cputime(1)); 
> +                       run_local_timers();
> +                       if (rcu_pending(cpu))
> +                               rcu_check_callbacks(cpu,
> user_mode(regs));
> +                       scheduler_tick();
> +                       run_posix_cpu_timers(p);
> +                       delta_itm += local_cpu_data->itm_delta;
> +                       if(cpu==time_keeper_id){
> +                               write_seqlock(&xtime_lock);
> +                               do_timer(regs);
> +                               local_cpu_data->itm_next = delta_itm +
> new_itm;
> +                               write_sequnlock(&xtime_lock);
> +                       }else{
> +                               local_cpu_data->itm_next = delta_itm +
> new_itm;
> +                       }
> +                       per_cpu(processed_stolen_time,cpu) +=
> NS_PER_TICK;
> +                       local_irq_enable();
> +                       local_irq_disable();
> +               }
> +       }

Why do we have to do these one at a time, instead of in bulk.  Seems
very inefficient.  Also, please explain the local_irq_enable/disable at
the end of each.

> +/* taken from i386/kernel/time-xen.c */
> +static void init_missing_ticks_accounting(int cpu)
> +{
> +       struct vcpu_register_runstate_memory_area area;
> +       struct vcpu_runstate_info *runstate = &per_cpu(runstate, cpu);
> +
> +       memset(runstate, 0, sizeof(*runstate));
> +
> +       area.addr.v = runstate;
> +       HYPERVISOR_vcpu_op(VCPUOP_register_runstate_memory_area, cpu,
> &area);
> +
> +       per_cpu(processed_blocked_time, cpu) =
> +               runstate->time[RUNSTATE_blocked];
> +       per_cpu(processed_stolen_time, cpu) =
> +               runstate->time[RUNSTATE_runnable] +
> +               runstate->time[RUNSTATE_offline];
> +}

Shouldn't this be in an #ifdef CONFIG_XEN?  HYPERVISOR_vcpu_op isn't
going to compile otherwise.  It should probably have a stub for the
non-CONFIG_XEN case so you don't have to #ifdef out the caller.

> +
> +long
> +xencomm_hypercall_vcpu_op(int cmd, int cpu, void *arg)
> +{
> +       switch (cmd) {
> +               case VCPUOP_register_runstate_memory_area:
> +               xencommize_memory_reservation((xen_memory_reservation_t 
> *)arg);
> +               break;

Is there any reason to filter out the other defined vcpu_ops?  Maybe we
should add support for them now.  Thanks,

        Alex
  
-- 
Alex Williamson                             HP Open Source & Linux Org.


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