LKML Archive on lore.kernel.org
help / color / mirror / Atom feed
From: Peter Zijlstra <peterz@infradead.org>
To: Glauber Costa <glommer@redhat.com>
Cc: kvm@vger.kernel.org, linux-kernel@vger.kernel.org,
	Rik van Riel <riel@redhat.com>,
	Jeremy Fitzhardinge <jeremy.fitzhardinge@citrix.com>,
	Avi Kivity <avi@redhat.com>
Subject: Re: [PATCH v3 3/6] KVM-GST: KVM Steal time accounting
Date: Fri, 11 Feb 2011 20:05:43 +0100	[thread overview]
Message-ID: <1297451143.5226.87.camel@laptop> (raw)
In-Reply-To: <1297448364-14051-4-git-send-email-glommer@redhat.com>

On Fri, 2011-02-11 at 13:19 -0500, Glauber Costa wrote:

> diff --git a/include/linux/sched.h b/include/linux/sched.h
> index d747f94..5dbf509 100644
> --- a/include/linux/sched.h
> +++ b/include/linux/sched.h
> @@ -302,6 +302,7 @@ long io_schedule_timeout(long timeout);
>  extern void cpu_init (void);
>  extern void trap_init(void);
>  extern void update_process_times(int user);
> +extern u64 (*hypervisor_steal_time)(int cpu);
>  extern void scheduler_tick(void);

That's quite terrible..

>  extern void sched_show_task(struct task_struct *p);
> diff --git a/kernel/sched.c b/kernel/sched.c
> index 18d38e4..60b0cf8 100644
> --- a/kernel/sched.c
> +++ b/kernel/sched.c
> @@ -524,6 +524,8 @@ struct rq {
>  	u64 prev_irq_time;
>  #endif
>  
> +	u64 prev_steal_ticks;
> +
>  	/* calc_load related fields */
>  	unsigned long calc_load_update;
>  	long calc_load_active;
> @@ -1780,6 +1782,16 @@ static void deactivate_task(struct rq *rq, struct task_struct *p, int flags)
>  	dec_nr_running(rq);
>  }
>  
> +u64 (*hypervisor_steal_time)(int cpu) = NULL;

I don't think exposing functions pointers like that is very nice at all.

> +static u64 steal_time_clock(int cpu)
> +{
> +	if (!hypervisor_steal_time)
> +		return 0;
> +
> +	return hypervisor_steal_time(cpu);
> +}

This really wants to be under some PARAVIRT config thing, preferably the
same as the other bits (PARAVIRT_TIME_ACCOUNTING).

Also, it would be nice to avoid the function call and conditional on
native hardware, this is on all scheduler hot paths, best it to make
sure you don't even get so far as to call this function on native
hardware.

>  #ifdef CONFIG_IRQ_TIME_ACCOUNTING
>  
>  /*
> @@ -3509,6 +3521,33 @@ unsigned long long thread_group_sched_runtime(struct task_struct *p)
>  }
>  
>  /*
> + * We have to at flush steal time information every time something else
> + * is accounted. Since the accounting functions are all visible to the rest
> + * of the kernel, it gets tricky to do them in one place. This helper function
> + * helps us.
> + *
> + * When the system is idle, the concept of steal time does not apply. We just
> + * tell the underlying hypervisor that we grabbed the data, but skip steal time
> + * accounting
> + */
> +static int touch_steal_time(int is_idle)
> +{
> +	u64 steal, st;
> +
> +	steal = steal_time_clock(smp_processor_id());
> +
> +	st = steal / TICK_NSEC - this_rq()->prev_steal_ticks;

(this won't compile on 32bits)

> +	this_rq()->prev_steal_ticks += st;

This doesn't seem right..

  struct rq *rq = this_rq();
  u64 steal, delta;
  int ticks = 0;

  steal = steal_time_clock(smp_processor_id());
  delta = rq->prev_steal_ticks;
  while (delta >= TICK_NSEC) {
    ticks++;
    rq->prev_steal_time += TICK_NSEC;
  }

  if (!ticks)
    return 0;

  account_steal_time(ticks);
  return 1;

would be much more accurate.

> +	if (!st || is_idle)
> +		return 0;
> +
> +	account_steal_time(st);
> +	return 1;
> +}

This also wants to be much much cheaper for native hardware even if you
were silly enough to enable the paravirt config ;-) ie. bail out of this
function at the start instead of going through the motions when we know
the clock doesn't count.




  reply	other threads:[~2011-02-11 19:04 UTC|newest]

Thread overview: 20+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2011-02-11 18:19 [PATCH v3 0/6] Steal time for KVM Glauber Costa
2011-02-11 18:19 ` [PATCH v3 1/6] KVM-HDR: KVM Steal time implementation Glauber Costa
2011-02-15 14:25   ` Avi Kivity
2011-02-11 18:19 ` [PATCH v3 2/6] KVM-HV: " Glauber Costa
2011-02-15 14:34   ` Avi Kivity
2011-02-11 18:19 ` [PATCH v3 3/6] KVM-GST: KVM Steal time accounting Glauber Costa
2011-02-11 19:05   ` Peter Zijlstra [this message]
2011-02-12 23:46     ` Glauber Costa
2011-02-15 14:35   ` Avi Kivity
2011-02-15 14:45     ` Peter Zijlstra
2011-02-15 15:17       ` Avi Kivity
2011-02-15 15:24         ` Rik van Riel
2011-02-15 15:26           ` Avi Kivity
2011-02-15 15:27         ` Peter Zijlstra
2011-02-11 18:19 ` [PATCH v3 4/6] KVM-GST: KVM Steal time registration Glauber Costa
2011-02-15 14:41   ` Avi Kivity
2011-02-15 15:48     ` Peter Zijlstra
2011-02-11 18:19 ` [PATCH v3 5/6] KVM-GST: adjust scheduler cpu power Glauber Costa
2011-02-11 19:05   ` Peter Zijlstra
2011-02-11 18:19 ` [PATCH v3 6/6] Describe KVM_MSR_STEAL_TIME Glauber Costa

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=1297451143.5226.87.camel@laptop \
    --to=peterz@infradead.org \
    --cc=avi@redhat.com \
    --cc=glommer@redhat.com \
    --cc=jeremy.fitzhardinge@citrix.com \
    --cc=kvm@vger.kernel.org \
    --cc=linux-kernel@vger.kernel.org \
    --cc=riel@redhat.com \
    --subject='Re: [PATCH v3 3/6] KVM-GST: KVM Steal time accounting' \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).