Re: [RFC PATCH v1 2/6] sched: track time spent in hypervisor tasks

[Jürgen Groß <jgross@xxxxxxxx>]

On 12.06.20 13:30, Volodymyr Babchuk wrote:
> On Fri, 2020-06-12 at 06:43 +0200, Jürgen Groß wrote:
> > On 12.06.20 02:22, Volodymyr Babchuk wrote:
> > > +void vcpu_end_hyp_task(struct vcpu *v)
> > > +{
> > > +    int delta;
> > > +
> > > +    if ( is_idle_vcpu(v) )
> > > +        return;
> > > +
> > > +    ASSERT(v->in_hyp_task);
> > > +
> > > +    /* We assume that hypervisor task time will not overflow int */
> > This will definitely happen for long running VMs. Please use a 64-bit
> > variable.
> It is not suposed to hold long time spans, as I described in the reply
> to previous email.
>
> > > +    delta = NOW() - v->hyp_entry_time;
> > > +    atomic_add(delta, &v->sched_unit->hyp_time);
> > > +
> > > +#ifndef NDEBUG
> > > +    v->in_hyp_task = false;
> > > +#endif
> > > +}
> > > +
> > >    /*
> > >     * Do the actual movement of an unit from old to new CPU. Locks for *both*
> > >     * CPUs needs to have been taken already when calling this!
> > > @@ -2615,6 +2646,7 @@ static void schedule(void)
> > >    
> > >        SCHED_STAT_CRANK(sched_run);
> > >    
> > > +    vcpu_end_hyp_task(current);
> > >        rcu_read_lock(&sched_res_rculock);
> > >    
> > >        lock = pcpu_schedule_lock_irq(cpu);
> > > diff --git a/xen/common/softirq.c b/xen/common/softirq.c
> > > index 063e93cbe3..03a29384d1 100644
> > > --- a/xen/common/softirq.c
> > > +++ b/xen/common/softirq.c
> > > @@ -71,7 +71,9 @@ void process_pending_softirqs(void)
> > >    void do_softirq(void)
> > >    {
> > >        ASSERT_NOT_IN_ATOMIC();
> > > +    vcpu_begin_hyp_task(current);
> > >        __do_softirq(0);
> > > +    vcpu_end_hyp_task(current);
> > This won't work for scheduling. current will either have changed,
> > or in x86 case __do_softirq() might just not return. You need to
> > handle that case explicitly in schedule() (you did that for the
> > old vcpu, but for the case schedule() is returning you need to
> > call vcpu_begin_hyp_task(current) there).
> Well, this is one of questions, I wanted to discuss. I certainly need
> to call vcpu_begin_hyp_task(current) after context switch. But what it
> is the right place? If my understaning is right, code on x86 platform
> will never reach this point. Or I'm wrong there?
No, this is correct.

You can add the call to context_switch() just after set_current() has
been called.


Juergen