Messages in this thread | | | Date | Fri, 7 Sep 2012 12:37:54 -0700 | From | Tejun Heo <> | Subject | Re: [PATCH 05/11 V5] workqueue: Add @bind arguement back without change any thing |
| |
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
| |