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

[Isaku Yamahata <yamahata@xxxxxxxxxxxxx>]

On Mon, May 05, 2008 at 11:33:26PM -0400, Aron Griffis wrote:
> Hello Atsushi and Isaku,
> 
> I'm considering filing a Red Hat bugzilla to request
> http://xenbits.xensource.com/xen-unstable.hg?rev/204046d99562 in
> RHEL5.x.  However I would like to understand the implication
> better so I can describe it in the bz.
> 
> Without 204046d99562, I think the steal time accounting will have
> stale values after a suspend/resume.  But could you explain what
> effect this ultimately has on the domain?  Is it only a problem
> in the top reporting or is there a deeper concern?
> 
> Also I noticed what appears to be a small error in the original
> consider_steal_time patch, see below:
> 
> Atsushi SAKAI wrote:  [Fri Mar 09 2007, 02:59:13AM EST]
> > +#ifdef CONFIG_XEN
> > +static unsigned long 
> > +consider_steal_time(unsigned long new_itm, struct pt_regs *regs)
> > +{
> > +   unsigned long delta_itm = 0;
> > +   unsigned long stolen, blocked;
> > +   unsigned long sched_time;
> > +   unsigned long stolentick = 0;
> > +   int cpu = smp_processor_id();
> > +   int i;
> > +   struct vcpu_runstate_info *runstate = &per_cpu(runstate, 
> > smp_processor_id());
> > +   struct task_struct *p = current;
> 
> Above delta_itm is initialized to 0
> 
> > +   do {
> > +           sched_time = runstate->state_entry_time;
> > +           mb();
> > +           stolen =runstate->time[RUNSTATE_runnable] + 
> > +                   runstate->time[RUNSTATE_offline] -
> > +                   per_cpu(processed_stolen_time,cpu);
> > +           blocked =runstate->time[RUNSTATE_blocked] -
> > +                   per_cpu(processed_blocked_time,cpu);
> > +           mb();
> > +   } while (sched_time != runstate->state_entry_time);
> > +
> > +   /* check for vcpu migration effect
> > +    * In this case, itc value is reversed.
> > +    * This causes huge stolen value.  
> > +    * This function just checks and reject this effect.
> > +    */
> > +   if(!time_after_eq(runstate->time[RUNSTATE_blocked],
> > +           per_cpu(processed_blocked_time,cpu)))
> > +                   blocked = 0;
> > +
> > +   if(!time_after_eq(runstate->time[RUNSTATE_runnable]+
> > +           runstate->time[RUNSTATE_offline],
> > +           per_cpu(processed_stolen_time,cpu)))
> > +                   stolen = 0;
> > +
> > +   if(!time_after(delta_itm + new_itm, ia64_get_itc()))
> > +           stolentick = ia64_get_itc() - delta_itm - new_itm;
> 
> Here delta_itm is used (twice), but I think it will always be
> zero here.  Is this a mistake?

stolentick is used to clip sotlen and blocked.
It looks like it should be the diff of now and the tick when
the timer interrupt handler was invoked, not new_itm.
Atsushi, any comment?

-- 
yamahata

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