Re: [Xen-devel] [RFC 1/9] schedule: Introduce per-pcpu time accounting

[Volodymyr Babchuk <Volodymyr_Babchuk@xxxxxxxx>]

Hello All,

This is gentle reminder about this RFC. 

Sadly, Andrii Anisov has left our team. But I'm commited to continue
his work on time accounting and real time scheduling.

I do realize, that proposed patches have become moldy. I can rebase
them onto current master, if it would help. 

On Wed, 2019-11-06 at 13:24 +0200, Andrii Anisov wrote:
> Hello Julien,
> 
> On 28.10.19 16:28, Julien Grall wrote:
> > It would be good to get a review from the scheduler maintainers (Dario, 
> > George) to make sure they are happy with the suggested states here.
> 
> I would not say I'm completely happy with this set of states. I'd like to 
> have a discussion on this topic with scheduler maintainers. Also because they 
> could have a different view from x86 world.

I would love to hear any inputs on this topc from general scheduling
approach standpoint and from x86 view.  

> > > Introduce per-pcpu time accounting what includes the following states:
> > 
> > I think we need a very detailed description of each states. Otherwise it 
> > will be hard to know how to categorize it.
> 
> I agree that we need a very detailed description of each states. Ask 
> questions if something is not clear or doubtful. I guess we could have 
> something better after Q&A process.
> 
> > > TACC_HYP - the pcpu executes hypervisor code like softirq processing
> > >             (including scheduling), tasklets and context switches
> > 
> > IHMO, "like" is too weak here. What do you exactly plan to introduce?
> 
> I think this should be what hypervisor does except hypercall and IO emulation 
> (what is TACC_GSYNC).
> 
> > For instance, on Arm, you consider that leave_hypervisor_tail() is part of 
> > TACC_HYP. This function will include some handling for synchronous trap.
> 
> I guess you are saying about `p2m_flush_vm`. I doubt here, and open for 
> suggestions.
> 
> 
> > > TACC_GUEST - the pcpu executes guests code
> > 
> > Looking at the arm64 code, you are executing some hypervisor code here. I 
> > agree this is impossible to not run any hypervisor code with TACC_GUEST, 
> > but I think this should be clarified in the documentation.
> 
> Do you mean adding few words about still having some hypervisor code near the 
> actual context switch from/to guest (entry/return_from_trap)?
> 
> > > TACC_IDLE - the low-power state of the pcpu
> > 
> > Did you intend to mean "idle vCPU" is in use?
> 
> No. I did mean what is written.
> Currently, the idle vcpu does hypervisor work (e.g. tasklets) along with the 
> low-power mode. IMO we have to separate them.
> 
> > > TACC_IRQ - the pcpu performs interrupts processing, without separation to
> > >             guest or hypervisor interrupts
> > > TACC_GSYNC - the pcpu executes hypervisor code to process synchronous trap
> > >               from the guest. E.g. hypercall processing or io emulation.
> > > 
> > > Currently, the only reenterant state is TACC_IRQ. It is assumed, no 
> > > changes
> > > to state other than TACC_IRQ could happen until we return from nested
> > > interrupts. IRQ time is accounted in a distinct way comparing to other 
> > > states.
> > 
> > s/comparing/compare/
> 
> OK.
> 
> > > It is acumulated between other states transition moments, and is 
> > > substracted
> > 
> > s/acumulated/accumulated/ s/substracted/subtracted/
> 
> OK.
> 
> > > from the old state on states transion calculation.
> [1]
> > s/transion/transition/
> 
> OK.
> 
> > > Signed-off-by: Andrii Anisov <andrii_anisov@xxxxxxxx>
> > > ---
> > >   xen/common/schedule.c   | 81 
> > > +++++++++++++++++++++++++++++++++++++++++++++++++
> > >   xen/include/xen/sched.h | 27 +++++++++++++++++
> > >   2 files changed, 108 insertions(+)
> > > 
> > > diff --git a/xen/common/schedule.c b/xen/common/schedule.c
> > > index 7b71581..6dd6603 100644
> > > --- a/xen/common/schedule.c
> > > +++ b/xen/common/schedule.c
> > > @@ -1539,6 +1539,87 @@ static void schedule(void)
> > >       context_switch(prev, next);
> > >   }
> > > +DEFINE_PER_CPU(struct tacc, tacc);
> > > +
> > > +static void tacc_state_change(enum TACC_STATES new_state)
> > 
> > This should never be called with the TACC_IRQ, right?
> 
> Yes. Actually, tacc->state should never be TACC_IRQ.
> Because of TACC_IRQ reenterability it is handled through the tacc->irq_cnt 
> and tacc->irq_enter_time.
> 
> > > +{
> > > +    s_time_t now, delta;
> > > +    struct tacc* tacc = &this_cpu(tacc);
> > > +    unsigned long flags;
> > > +
> > > +    local_irq_save(flags);
> > > +
> > > +    now = NOW();
> > > +    delta = now - tacc->state_entry_time;
> > > +
> > > +    /* We do not expect states reenterability (at least through this 
> > > function)*/
> > > +    ASSERT(new_state != tacc->state);
> > > +
> > > +    tacc->state_time[tacc->state] += delta - tacc->irq_time;
> > > +    tacc->state_time[TACC_IRQ] += tacc->irq_time;
> > > +    tacc->irq_time = 0;
> > > +    tacc->state = new_state;
> > > +    tacc->state_entry_time = now;
> > > +
> > > +    local_irq_restore(flags);
> > > +}
> > > +
> > > +void tacc_hyp(int place)
> > 
> > Place is never used except for your commented printk. So what's the goal 
> > for it?
> 
> Place is just a piece of code used for debugging, as well as printk. I keept 
> it here because this series is very RFC, yet it could be removed.
> 
> > Also, is it really necessary to provide helper for each state? Couldn't we 
> > just introduce one functions doing all the state?
> 
> I'd like calling that stuff from assembler without parameters. But have no 
> strong opinion here.
>   
> > > +{
> > > +//    printk("\ttacc_hyp %u, place %d\n", smp_processor_id(), place);
> > > +    tacc_state_change(TACC_HYP);
> > > +}
> > > +
> > > +void tacc_guest(int place)
> > > +{
> > > +//    printk("\ttacc_guest %u, place %d\n", smp_processor_id(), place);
> > > +    tacc_state_change(TACC_GUEST);
> > > +}
> > > +
> > > +void tacc_idle(int place)
> > > +{
> > > +//    printk("\tidle cpu %u, place %d\n", smp_processor_id(), place);
> > > +    tacc_state_change(TACC_IDLE);
> > > +}
> > > +
> > > +void tacc_gsync(int place)
> > > +{
> > > +//    printk("\ttacc_gsync %u, place %d\n", smp_processor_id(), place);
> > > +    tacc_state_change(TACC_GSYNC);
> > > +}
> > > +
> > > +void tacc_irq_enter(int place)
> > > +{
> > > +    struct tacc* tacc = &this_cpu(tacc);
> > > +
> > > +//    printk("\ttacc_irq_enter %u, place %d, cnt %d\n", 
> > > smp_processor_id(), place, this_cpu(tacc).irq_cnt);
> > > +    ASSERT(!local_irq_is_enabled());
> > > +    ASSERT(tacc->irq_cnt >= 0);
> > > +
> > > +    if ( tacc->irq_cnt == 0 )
> > > +    {
> > > +        tacc->irq_enter_time = NOW();
> > > +    }
> > > +
> > > +    tacc->irq_cnt++;
> > > +}
> > > +
> > > +void tacc_irq_exit(int place)
> > > +{
> > > +    struct tacc* tacc = &this_cpu(tacc);
> > > +
> > > +//    printk("\ttacc_irq_exit %u, place %d, cnt %d\n", 
> > > smp_processor_id(), place, tacc->irq_cnt);
> > > +    ASSERT(!local_irq_is_enabled());
> > > +    ASSERT(tacc->irq_cnt > 0);
> > > +    if ( tacc->irq_cnt == 1 )
> > > +    {
> > > +        tacc->irq_time = NOW() - tacc->irq_enter_time;
> > 
> > If I understand correctly, you will use irq_time to update TACC_IRQ in 
> > tacc_state_change(). It may be possible to receive another interrupt before 
> > the state is changed (e.g. HYP -> GUEST). This means only the time for the 
> > last IRQ received would be accounted.
> 
> I do lock IRQs for state change. Shouldn't that protect it?
> 
> > > +        tacc->irq_enter_time = 0;
> > > +    }
> > > +
> > > +    tacc->irq_cnt--;
> > > +}
> > > +
> > >   void context_saved(struct vcpu *prev)
> > >   {
> > >       /* Clear running flag /after/ writing context to memory. */
> > > diff --git a/xen/include/xen/sched.h b/xen/include/xen/sched.h
> > > index e3601c1..04a8724 100644
> > > --- a/xen/include/xen/sched.h
> > > +++ b/xen/include/xen/sched.h
> > > @@ -1002,6 +1002,33 @@ extern void dump_runq(unsigned char key);
> > >   void arch_do_physinfo(struct xen_sysctl_physinfo *pi);
> > > +enum TACC_STATES {
> > 
> > We don't tend to use all uppercases for enum name.
> 
> OK.
> 
> > > +    TACC_HYP = 0,
> > 
> > enum begins at 0 and increment by one every time. So there is no need to 
> > hardcode a number.
> > 
> > Also, looking at the code, I think you rely on the first state to be 
> > TACC_HYP. Am I correct?
> 
> TACC_HYP is expected to be the initial state of the PCPU.
> 
> > > +    TACC_GUEST = 1,
> > > +    TACC_IDLE = 2,
> > > +    TACC_IRQ = 3,
> > > +    TACC_GSYNC = 4,
> > > +    TACC_STATES_MAX
> > > +};
> > > It would be good to document all the states in the header as well.
> 
> OK.
> 
> > > +
> > > +struct tacc
> > 
> > Please document the structure.
> 
> OK.
> 
> > > +{
> > > +    s_time_t state_time[TACC_STATES_MAX];
> > > +    s_time_t state_entry_time;
> > > +    int state;
> > 
> > This should be the enum you used above here.
> 
> Yep.
> 
> > > +
> > > +    s_time_t guest_time;
> > 
> > This is not used.
> 
> Yep, will drop it.
> 
> > > +
> > > +    s_time_t irq_enter_time;
> > > +    s_time_t irq_time;
> > > +    int irq_cnt;
> > Why do you need this to be signed?
> 
> For assertion.
>   
> > > +};
> > > +
> > > +DECLARE_PER_CPU(struct tacc, tacc);
> > > +
> > > +void tacc_hyp(int place);
> > > +void tacc_idle(int place);
> > > +
> > >   #endif /* __SCHED_H__ */
> > >   /*
> > > 
> > 
> > Cheers,
> >