Andrii,
Andrii Anisov writes:
> From: Andrii Anisov <andrii_ani...@epam.com>
>
> Introduce per-pcpu time accounting what includes the following states:
>
> TACC_HYP - the pcpu executes hypervisor code like softirq processing
> (including scheduling), tasklets and context switches
> TACC_GUEST - the pcpu executes guests code
> TACC_IDLE - the low-power state of the pcpu
Is it really low-power?
> TACC_IRQ - the pcpu performs interrupts processing, without separation to
> guest or hypervisor interrupts
I think, word "distinguishing" would be better than "separation"
> 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.
> It is acumulated between other states transition moments, and is substracted
> from the old state on states transion calculation.
>
> Signed-off-by: Andrii Anisov <andrii_ani...@epam.com>
> ---
> 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)
> +{
> + 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)
I believe, you want some enum for this "place" parameter type
> +{
> +// printk("\ttacc_hyp %u, place %d\n", smp_processor_id(), place);
Please, don't push commented-out code. BTW, I think, it is possible to
add some TACC_DEBUG facilities to enable/disable this traces during
compile-time.
Also, looks like you don't use "place" parameter at all.
Lastly, I believe that this function (and other similar functions below)
can be defined as "static inline" in a header file.
> + 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);
You can make irq_cnt unsigned and drop this assert.
> +
> + if ( tacc->irq_cnt == 0 )
> + {
> + tacc->irq_enter_time = NOW();
> + }
Coding style:
---
Braces should be omitted for blocks with a single statement. e.g.,
if ( condition )
single_statement();
---
> +
> + 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;
> + tacc->irq_enter_time = 0;
> + }
> +
> + tacc->irq_cnt--;
What if, you IRQ will arrive right after this? I believe, you will lose
some of the accumulated time.
> +}
> +
> 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 {
If I remember correct, enum names should in lower case
> + TACC_HYP = 0,
> + TACC_GUEST = 1,
> + TACC_IDLE = 2,
> + TACC_IRQ = 3,
> + TACC_GSYNC = 4,
> + TACC_STATES_MAX
> +};
> +
> +struct tacc
> +{
> + s_time_t state_time[TACC_STATES_MAX];
> + s_time_t state_entry_time;
> + int state;
enum, maybe?
> +
> + s_time_t guest_time;
> +
> + s_time_t irq_enter_time;
> + s_time_t irq_time;
> + int irq_cnt;
> +};
> +
> +DECLARE_PER_CPU(struct tacc, tacc);
> +
> +void tacc_hyp(int place);
> +void tacc_idle(int place);
What about functions from sched.c? Should they be declared there?
> +
> #endif /* __SCHED_H__ */
>
> /*
--
Volodymyr Babchuk at EPAM
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel
