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

[Volodymyr Babchuk <Volodymyr_Babchuk@xxxxxxxx>]

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?