Messages in this thread | | | From | Linus Torvalds <> | Date | Fri, 13 Jul 2012 21:27:03 -0700 | Subject | Re: [PATCH 5/6] workqueue: introduce NR_WORKER_POOLS and for_each_worker_pool() |
| |
Seeing code like this
+ return &(*nr_running)[0];
just makes me go "WTF?"
Why are you taking the address of something you just dereferenced (the "& [0]" part).
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.
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; }
Notice how there isn't an 'address-of' operator anywhere in sight there. Those things are arrays, they get turned into "atomic_t *" automatically. And there isn't a single dereference (not a '*', and not a "[0]" - they are the exact same thing, btw) in sight either.
What am I missing? Are there some new drugs that all the cool kids chew that I should be trying? Because I really don't think the kinds of insane "take the address of a dereference" games are a good idea. They really look to me like somebody is having a really bad drug experience.
I didn't test the code, btw. I just looked at the patch and went WTF.
Linus
| |