LKML Archive on lore.kernel.org
help / color / mirror / Atom feed
From: Glauber Costa <glommer@redhat.com>
To: Peter Zijlstra <peterz@infradead.org>
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: Sat, 12 Feb 2011 21:46:52 -0200	[thread overview]
Message-ID: <1297554412.3419.39.camel@mothafucka.localdomain> (raw)
In-Reply-To: <1297451143.5226.87.camel@laptop>

On Fri, 2011-02-11 at 20:05 +0100, Peter Zijlstra wrote:
> 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).

Agree on the function pointer, but (at first) disagree on the
conditional. The mere accounting of steal time is in principle
independent on its use to adjust CPU power. So we potentially may want
the first, but avoid the later. 

I can wrap the accounting on a CONFIG switch, but I don't honestly see a
reason for it. 

> 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.

Fair.

> >  #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.

Okay, I will update it.

> 
> 



  reply	other threads:[~2011-02-12 23:47 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
2011-02-12 23:46     ` Glauber Costa [this message]
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=1297554412.3419.39.camel@mothafucka.localdomain \
    --to=glommer@redhat.com \
    --cc=avi@redhat.com \
    --cc=jeremy.fitzhardinge@citrix.com \
    --cc=kvm@vger.kernel.org \
    --cc=linux-kernel@vger.kernel.org \
    --cc=peterz@infradead.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).