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.
pgpWiPN76MvFy.pgp
Description: PGP signature
