On Tue, 28 Feb 2012 13:38:46 +0400 Pavel Shilovsky <piastry@xxxxxxxxxxx> wrote: > 2012/2/28 Steve French <smfrench@xxxxxxxxx>: > > I got a chance to look through this patch set in more detail on the way > > to the SMB2.2 test event. I liked many of the patches, but this one > > seems to complicate, rather than simplify by adding a spinlock > > to what used to be an atomic_inc. There is also a possibility that this > > spinlock will become hot. I don't mind moving the atomic_inc inside the > > new dec_in_flight macro though to make it easier in the future. > > > > The idea was to simplify the usage of inFlight value. Now it is > protected by both spin_lock and atomic mechanism in one case and with > only atomic in another - I think it's rather complicated. The patch > gets rid of one of the protection mechanism (atomic) and use only > spin_lock - this spin_lock is used further for protecting oplock and > echo specific variables as well. > The problem here is that the locking around this variable is unclear: If you need a spinlock to access/change it, then why use an atomic? If an atomic is sufficient then why use the spinlock at all? I don't much care what scheme you end up using for this, but if you really need a belt-and-suspenders approach (atomic + spinlock), then the reasoning for it needs to be clearly documented. We mere mortals must understand how it's supposed to work so we don't break it later. -- Jeff Layton <jlayton@xxxxxxxxxx> -- To unsubscribe from this list: send the line "unsubscribe linux-cifs" in the body of a message to majordomo@xxxxxxxxxxxxxxx More majordomo info at http://vger.kernel.org/majordomo-info.html
