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 -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to [email protected] More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
