xfs
[Top] [All Lists]

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

To: Linus Torvalds <torvalds@xxxxxxxxxxxxxxxxxxxx>
Subject: Re: [PATCH 5/6] workqueue: introduce NR_WORKER_POOLS and for_each_worker_pool()
From: Tejun Heo <tj@xxxxxxxxxx>
Date: Fri, 13 Jul 2012 21:44:38 -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=sender:date:from:to:cc:subject:message-id:references:mime-version :content-type:content-disposition:in-reply-to:user-agent; bh=jBLcLi8t1glejEWdrJzV4/NQ66JPNdtmDl5rx0+yn+Y=; b=gz1fOc9dQ6vQWZzYlM+O1iOGh6853PfZDqgBXt7MQndtiw6EA7JwgOaIMqH0lNQ6l/ UYlGdZdL5J/lXGlp+BAjUXwB0maJOXxCMaMGPKZp8HtkrdljleW4FTXDGsJTGtag6wUt bZ8qMU3CKp9UsfyY0LR3uXYOy8bsoDoZCXDZcSoznF13gOTkCTD2slbIYi9sM11Ag1yF 9HX4Xp1TqGlA5zct4+do05ybfNATLDmNaQknAbyxptPIHvF751BM3uI/qTU40ZurLnFu 1QUGiXJiWxXvvArK4HPOIVdd5PTC9BCL4EFqltMFV54qxhJgIh4pG4IG45A/G6HfGwa0 Dy0w==
In-reply-to: <CA+55aFyeauqCqrWsx4U2TB2ENrugZXYj+4vw3Fd0kGaeWBP3RA@xxxxxxxxxxxxxx>
References: <1341859315-17759-1-git-send-email-tj@xxxxxxxxxx> <1341859315-17759-6-git-send-email-tj@xxxxxxxxxx> <20120714035538.GB5638@xxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxx> <CA+55aFyeauqCqrWsx4U2TB2ENrugZXYj+4vw3Fd0kGaeWBP3RA@xxxxxxxxxxxxxx>
Sender: Tejun Heo <htejun@xxxxxxxxx>
User-agent: Mutt/1.5.20 (2009-06-14)
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

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