Messages in this thread | | | Date | Fri, 07 Sep 2012 10:11:56 +0800 | From | Lai Jiangshan <> | Subject | Re: [PATCH 05/11 V5] workqueue: Add @bind arguement back without change any thing |
| |
On 09/07/2012 12:51 AM, Tejun Heo wrote: > Hello, Lai. > > On Thu, Sep 06, 2012 at 09:04:06AM +0800, Lai Jiangshan wrote: >>> This doesn't change anything. You're just moving the test to the >>> caller with comments there explaining how it won't change even if >>> gcwq->lock is released. It seems more confusing to me. The flag is >>> still protected by manager_mutex. How is this an improvement? >>> >> >> Some other bit of gcwq->flags is accessed(modified) without manager_mutex. >> making gcwq->flags be accessed only form gcwq->lock C.S. will help the reviewer. >> >> I don't like adding special things/code when not-absolutely-required. > > I really fail to see this. The flag has to stay stable while > manage_mutex is held no matter where you test it.
Only one bit is stable, the whole flags can be changed outside.
I prefer the whole byte or short or int or long is protected under the same synchronization. I don't prefer different bit uses different synchronization.
> It doesn't make any > it any more readable whether you test it inside gcwq->lock with the > comment saying "this won't change while manager_mutex is held" or just > test it while manager_mutex is held. It is a synchronization oddity > no matter what and as long as it's well documented, I don't really see > the point in the change. >
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.
Thanks, Lai will be
| |