Re: [PATCH 05/11 V5] workqueue: Add @bind arguement back withoutchange any thing

[Tejun Heo]

Hello, Lai.

On Fri, Sep 07, 2012 at 10:11:56AM +0800, Lai Jiangshan wrote:
> When I read "gcwq->flags & GCWQ_DISASSOCIATED" in create_worker, I
> thought: WTF, gcwq->flags can be change by other, is it
> correct?. When I saw the comments claim it is correct, I have to use
> about 30 mins to check whether it is correct in several places of
> code in workqueue.c(include check does flags has internal state in
> all gcwq->lock).  I'm not good on it, but I think there are some
> reviewers will be confused like me.

The head-scratchy part there is not where it's tested - under
manager_mutex or gcwq->lock.  It's that DISASSOCIATED has an
additional locking restriction while still living in gcwq->flags.
Simply moving DISASSOCIATED test inside gcwq->lock doesn't change
anything including readability.  You still have to verify the flag
changes according to the said locking requirement.

Separating DISASSOCIATED into a separate variable could help it but I
really don't see much point in doing that.  I don't know why it took
you 30 minutes to figure out when it's clearly stated in the constant
definition and there are only two places which modify the flag - one
setting and the other clearing.  Were you worrying about the flag
flipping while other bits are modified?  But even then, there's only
one other flag - GCWQ_FREEZING which again is set and cleared once in
the whole code.

So, either you were hopelessly confused for some reason or I'm missing
something.  If the latter, please enlighten me; otherwise, I actually
like putting the test outside gcwq->lock.  That makes it explicit that
the flag shouldn't be changing outside manager_mutex, which is the
necessary locking requirement.

Thanks.

-- 
tejun
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/