linux-kernel - Re: [REGRESSION] funny sched_domain build failure during resume

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: <20140515085156.GG30445@twins.programming.kicks-ass.net>
Date:	Thu, 15 May 2014 10:51:56 +0200
From:	Peter Zijlstra <peterz@...radead.org>
To:	Juri Lelli <juri.lelli@...il.com>
Cc:	Tejun Heo <tj@...nel.org>, Ingo Molnar <mingo@...hat.com>,
	linux-kernel@...r.kernel.org, Johannes Weiner <hannes@...xchg.org>,
	"Rafael J. Wysocki" <rjw@...ysocki.net>
Subject: Re: [REGRESSION] funny sched_domain build failure during resume

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.

Content of type "application/pgp-signature" skipped