Re: [PATCH] sched: fix autogroup nice tune on UP

[Pekka Enberg]

On Sun, Jan 23, 2011 at 5:03 PM, Yong Zhang <yong.zhang0@xxxxxxxxx> wrote:
> And there is another issue on UP when FAIR_GROUP_SCHED
> enabled.
>
> ---
> From: Yong Zhang <yong.zhang0@xxxxxxxxx>
> Subject: [PATCH] sched: fix autogroup nice tune on UP
>
> When on UP with FAIR_GROUP_SCHED enabled, tune shares
> only affect tg->shares, but is not reflected on
> tg->se->load, the reason is update_cfs_shares()
> do nothing on SMP.

Typo here? You mean UP, right?

> So let sched_group_set_shares() update tg->se->load
> directly for UP && FAIR_GROUP_SCHED.
>
> This issue is found when enable autogroup, but also
> exists on cgroup.cpu on UP.
>
> Signed-off-by: Yong Zhang <yong.zhang0@xxxxxxxxx>
> Cc: Ingo Molnar <mingo@xxxxxxx>
> Cc: Peter Zijlstra <peterz@xxxxxxxxxxxxx>
> Cc: Mike Galbraith <efault@xxxxxx>
> ---
>  kernel/sched.c |    8 ++++++--
>  1 files changed, 6 insertions(+), 2 deletions(-)
>
> diff --git a/kernel/sched.c b/kernel/sched.c
> index 3ec2c6c..b8ed459 100644
> --- a/kernel/sched.c
> +++ b/kernel/sched.c
> @@ -8483,8 +8483,8 @@ static DEFINE_MUTEX(shares_mutex);
>
>  int sched_group_set_shares(struct task_group *tg, unsigned long shares)
>  {
> -       int i;
> -       unsigned long flags;
> +       int uninitialized_var(i);
> +       unsigned long uninitialized_var(flags);
>
>        /*
>         * We can't change the weight of the root cgroup.
> @@ -8502,6 +8502,7 @@ int sched_group_set_shares(struct task_group *tg, unsigned long shares)
>                goto done;
>
>        tg->shares = shares;
> +#ifdef CONFIG_SMP
>        for_each_possible_cpu(i) {
>                struct rq *rq = cpu_rq(i);
>                struct sched_entity *se;
> @@ -8513,6 +8514,9 @@ int sched_group_set_shares(struct task_group *tg, unsigned long shares)
>                        update_cfs_shares(group_cfs_rq(se), 0);
>                raw_spin_unlock_irqrestore(&rq->lock, flags);
>        }
> +#else
> +       update_load_set(&tg->se[0]->load, shares);
> +#endif

I'm not an expert on the scheduler but this looks like the wrong thing
to do. Shouldn't you fix update_cfs_shares() to work on UP properly?
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/