Re: [REGRESSION] funny sched_domain build failure during resume

[Peter Zijlstra]

On Thu, May 15, 2014 at 10:40:55AM +0200, Juri Lelli wrote:
> Hi,
> 
> > @@ -37,10 +38,7 @@ static inline int dl_time_before(u64 a, u64 b)
> >  
> >  static void cpudl_exchange(struct cpudl *cp, int a, int b)
> >  {
> > -	int cpu_a = cp->elements[a].cpu, cpu_b = cp->elements[b].cpu;
> > -
> >  	swap(cp->elements[a], cp->elements[b]);
> > -	swap(cp->cpu_to_idx[cpu_a], cp->cpu_to_idx[cpu_b]);
> >  }
> >  
> 
> I think there is a problem here. Your patch "embeds" cpu_to_idx array
> in elements array, but here the swap operates differently on the two.

<snip>

> Sorry for this long reply, but I had to convince also myself...

Glad you explained it before I tried to untangle that code myself.

> So, I think that having just one dynamic array is nicer and better, but
> we still need to swap things separately. Something like (on top of
> yours):
> 
> diff --git a/kernel/sched/cpudeadline.c b/kernel/sched/cpudeadline.c
> index 60ad0af..10dc7ab 100644
> --- a/kernel/sched/cpudeadline.c
> +++ b/kernel/sched/cpudeadline.c
> @@ -36,9 +36,31 @@ static inline int dl_time_before(u64 a, u64 b)
>         return (s64)(a - b) < 0;
>  }
>  
> +static inline void swap_element(struct cpudl *cp, int a, int b)
> +{
> +       int cpu_tmp = cp->elements[a].cpu;
> +       u64 dl_tmp = cp->elements[a].dl;
> +
> +       cp->elements[a].cpu = cp->elements[b].cpu;
> +       cp->elements[a].dl = cp->elements[b].dl;
> +       cp->elements[b].cpu = cpu_tmp;
> +       cp->elements[b].dl = dl_tmp;

You could've just written:

	swap(cp->elements[a].cpu, cp->elements[b].cpu);
	swap(cp->elements[a].dl , cp->elements[b].dl );

The swap macro does the tmp var for you.

> +}
> +
> +static inline void swap_idx(struct cpudl *cp, int a, int b)
> +{
> +       int idx_tmp = cp->elements[a].idx;
> +
> +       cp->elements[a].idx = cp->elements[b].idx;
> +       cp->elements[b].idx = idx_tmp;

	swap(cp->elements[a].idx, cp->elements[b].idx);

> +}
> +
>  static void cpudl_exchange(struct cpudl *cp, int a, int b)
>  {
> -       swap(cp->elements[a], cp->elements[b]);
> +       int cpu_a = cp->elements[a].cpu, cpu_b = cp->elements[b].cpu;
> +
> +       swap_element(cp, a, b);
> +       swap_idx(cp, cpu_a, cpu_b);

Or just skip the lot and put the 3 swap() stmts here.

>  }
>  
>  static void cpudl_heapify(struct cpudl *cp, int idx)
> ---
> 
> But, I don't know if this is too ugly and we better go with two arrays
> (or a better solution, as this is the fastest thing I could come up
> with :)).

Thanks for looking at it, and sorry for breaking it.

Attachment: pgpjCV6tfTNbS.pgp
Description: PGP signature