Re: [PATCH v4 8/9] KVM-GST: adjust scheduler cpu power

[Peter Zijlstra <peterz@xxxxxxxxxxxxx>]

On Fri, 2011-07-01 at 17:22 -0400, Glauber Costa wrote:
> @@ -1971,8 +1974,14 @@ static inline u64 steal_ticks(u64 steal)
>  
>  static void update_rq_clock_task(struct rq *rq, s64 delta)
>  {
> -       s64 irq_delta;
> -
> +/*
> + * In theory, the compile should just see 0 here, and optimize out the call
> + * to sched_rt_avg_update. But I don't trust it...
> + */
> +#if defined(CONFIG_IRQ_TIME_ACCOUNTING) || defined(CONFIG_PARAVIRT_TIME_ACCOUNTING)
> +       s64 steal = 0, irq_delta = 0;
> +#endif

So I wanted to ask a GCC person (Hi Jeff) about this.

> +#ifdef CONFIG_IRQ_TIME_ACCOUNTING
>         irq_delta = irq_time_read(cpu_of(rq)) - rq->prev_irq_time;
>  
>         /*
> @@ -1995,12 +2004,35 @@ static void update_rq_clock_task(struct rq *rq, s64 delta)
>  
>         rq->prev_irq_time += irq_delta;
>         delta -= irq_delta;
> +#endif
> +#ifdef CONFIG_PARAVIRT_TIME_ACCOUNTING
> +       if (static_branch((&paravirt_steal_rq_enabled))) {
> +               u64 st;
> +
> +               steal = paravirt_steal_clock(cpu_of(rq));
> +               steal -= rq->prev_steal_time_rq;
> +
> +               if (unlikely(steal > delta))
> +                       steal = delta;
> +
> +               st = steal_ticks(steal);
> +               steal = st * TICK_NSEC;
> +
> +               rq->prev_steal_time_rq += steal;
> +
> +               delta -= steal;
> +       }
> +#endif
> +
>         rq->clock_task += delta;
>  
> -       if (irq_delta && sched_feat(NONIRQ_POWER))
> -               sched_rt_avg_update(rq, irq_delta);
> +#if defined(CONFIG_IRQ_TIME_ACCOUNTING) || defined(CONFIG_PARAVIRT_TIME_ACCOUNTING)
> +       if ((irq_delta + steal) && sched_feat(NONTASK_POWER))
> +               sched_rt_avg_update(rq, irq_delta + steal);
> +#endif
>  } 

In case of !CONFIG_IRQ_TIME_ACCOUNTING && !
CONFIG_PARAVIRT_TIME_ACCOUNTING, irq_delta and steal will both always be
0.

The function will basically look like:

static void update_rq_clock_task(struct rq *rq, s64 delta)
{
	s64 irq_delta = 0, steal = 0;

	rq->clock_task += delta;

	if ((irq_delta + steal) && sched_feat(NONTASK_POWER))
		sched_rt_avg_update(rq, irq_delta + steal);
}

And we want it to emit the equivalent of:

static void update_rq_clock_task(struct rq *rq, s64 delta)
{
	rq->clock_task += delta;
}

Now Glauber is properly paranoid and doesn't trust his compiler (this is
very hot code in the kernel so any extra code emitted here is sad) and
chose the heavy handed CPP solution.

Now without checking a all relevant gcc versions on all relevant
architectures (see you in a few weeks etc..) can we actually rely on gcc
doing such relatively simple things correct, or should we stick with CPP
just to make sure?
--
To unsubscribe from this list: send the line "unsubscribe kvm" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html