linux-kernel - Re: [PATCH v3 3/6] KVM-GST: KVM Steal time accounting

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <1297554412.3419.39.camel@mothafucka.localdomain>
Date:	Sat, 12 Feb 2011 21:46:52 -0200
From:	Glauber Costa <glommer@...hat.com>
To:	Peter Zijlstra <peterz@...radead.org>
Cc:	kvm@...r.kernel.org, linux-kernel@...r.kernel.org,
	Rik van Riel <riel@...hat.com>,
	Jeremy Fitzhardinge <jeremy.fitzhardinge@...rix.com>,
	Avi Kivity <avi@...hat.com>
Subject: Re: [PATCH v3 3/6] KVM-GST: KVM Steal time accounting

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.

> 
> 


--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/