Hello, Linus.
On Fri, Jul 13, 2012 at 09:27:03PM -0700, Linus Torvalds wrote:
> Seeing code like this
>
> + return &(*nr_running)[0];
>
> just makes me go "WTF?"
I was going WTF too. This was the smallest fix and I wanted to make
it minimal because there's another stack of patches on top of it.
Planning to just fold nr_running into worker_pool afterwards which
will remove the whole function.
> Why are you taking the address of something you just dereferenced (the
> "& [0]" part).
nr_running is atomic_t (*nr_running)[2]. Ignoring the pointer to
array part, it's just returning the address of N'th element of the
array. ARRAY + N == &ARRAY[N].
> And you actually do that *twice*, except the inner one is more
> complicated. When you assign nr_runing, you take the address of it, so
> the "*nr_running" is actually just the same kind of odd thing (except
> in reverse - you take dereference something you just took the
> address-of).
>
> Seriously, this to me is a sign of *deeply* confused code. And the
> fact that your first version of that code was buggy *EXACTLY* due to
> this confusion should have made you take a step back.
Type-wise, I don't think it's confused. Ah okay, you're looking at
the fifth patch in isolation. Upto this point, the index is always 0.
I'm puttin it in as a placeholder for the next patch which makes use
of non-zero index. This patch is supposed to prepare everything for
multiple pools and thus non-zero index.
> As far as I can tell, what you actually want that function to do is:
>
> static atomic_t *get_pool_nr_running(struct worker_pool *pool)
> {
> int cpu = pool->gcwq->cpu;
>
> if (cpu != WORK_CPU_UNBOUND)
> return per_cpu(pool_nr_running, cpu);
>
> return unbound_pool_nr_running;
> }
More like the folloiwng in the end.
static atomic_t *get_pool_nr_running(struct worker_pool *pool)
{
int cpu = pool->gcwq->cpu;
int is_highpri = pool_is_highpri(pool);
if (cpu != WORK_CPU_UNBOUND)
return &per_cpu(pool_nr_running, cpu)[is_highpri];
return &unbound_pool_nr_running[is_highpri];
}
> I didn't test the code, btw. I just looked at the patch and went WTF.
Eh... yeah, with or without [2], this is WTF. I'll just refresh it
with the above version.
Thanks.
--
tejun
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
