xfs
[Top] [All Lists]

Re: [PATCH 5/6] workqueue: introduce NR_WORKER_POOLS and for_each_worker

To: Tejun Heo <tj@xxxxxxxxxx>
Subject: Re: [PATCH 5/6] workqueue: introduce NR_WORKER_POOLS and for_each_worker_pool()
From: Linus Torvalds <torvalds@xxxxxxxxxxxxxxxxxxxx>
Date: Fri, 13 Jul 2012 21:27:03 -0700
Cc: linux-kernel@xxxxxxxxxxxxxxx, joshhunt00@xxxxxxxxx, axboe@xxxxxxxxx, rni@xxxxxxxxxx, vgoyal@xxxxxxxxxx, vwadekar@xxxxxxxxxx, herbert@xxxxxxxxxxxxxxxxxxxx, davem@xxxxxxxxxxxxx, linux-crypto@xxxxxxxxxxxxxxx, swhiteho@xxxxxxxxxx, bpm@xxxxxxx, elder@xxxxxxxxxx, xfs@xxxxxxxxxxx, marcel@xxxxxxxxxxxx, gustavo@xxxxxxxxxxx, johan.hedberg@xxxxxxxxx, linux-bluetooth@xxxxxxxxxxxxxxx, martin.petersen@xxxxxxxxxx
Dkim-signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=gmail.com; s=20120113; h=mime-version:sender:in-reply-to:references:from:date :x-google-sender-auth:message-id:subject:to:cc:content-type; bh=YCO829wc5UjGpa5+DIu1lnILznSbRkRiJgo8r4NRfdA=; b=k+8ZrwDP0sHEKN/faCkej78cu3naLyaOjiueqfv3X2sCfPHzYOYe+a+IXmLi+ENlQB Eor2xk/ui6qq/9c3Db82KI6P09FajX/xL8K1RaT6waJHg91LkmwE0DqTn4rldmSRwx3x m7d/bnfnjT9PMK1LHa9Mel82sQ8UIwX0dlVCbda10M3NmRDJyMeCDxDlsuaEzxLpa710 tLjYvJYNQ3cD/vSg2eCeDPmendy0/WkXR8Rygx/iOIkjaRfFw37XgeNJGR3B7rEy2kWh MP2QH8ln5c8J/dFNjP5YAKkiZ4vhNKJzwCPnN1e2g7j12AzpQoLM0hcRW5zG7ZQ6Tf99 ckLg==
In-reply-to: <20120714035538.GB5638@xxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxx>
References: <1341859315-17759-1-git-send-email-tj@xxxxxxxxxx> <1341859315-17759-6-git-send-email-tj@xxxxxxxxxx> <20120714035538.GB5638@xxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxx>
Sender: linus971@xxxxxxxxx
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

<Prev in Thread] Current Thread [Next in Thread>