From mboxrd@z Thu Jan 1 00:00:00 1970 From: Linus Torvalds Subject: Re: [PATCH v7 1/4] spinlock: A new lockref structure for lockless update of refcount Date: Thu, 29 Aug 2013 17:49:46 -0700 Message-ID: References: <1375758759-29629-1-git-send-email-Waiman.Long@hp.com> <1375758759-29629-2-git-send-email-Waiman.Long@hp.com> <1377751465.4028.20.camel@pasglop> <20130829070012.GC27322@gmail.com> <1377822408.4028.44.camel@pasglop> Mime-Version: 1.0 Content-Type: multipart/mixed; boundary=047d7b677efa5cf00704e51f99d4 Cc: Ingo Molnar , Waiman Long , Alexander Viro , Jeff Layton , Miklos Szeredi , Ingo Molnar , Thomas Gleixner , linux-fsdevel , Linux Kernel Mailing List , Peter Zijlstra , Steven Rostedt , Andi Kleen , "Chandramouleeswaran, Aswin" , "Norton, Scott J" , Michael Neuling To: Benjamin Herrenschmidt Return-path: In-Reply-To: <1377822408.4028.44.camel@pasglop> Sender: linux-kernel-owner@vger.kernel.org List-Id: linux-fsdevel.vger.kernel.org --047d7b677efa5cf00704e51f99d4 Content-Type: text/plain; charset=UTF-8 On Thu, Aug 29, 2013 at 5:26 PM, Benjamin Herrenschmidt wrote: > > I assume you mean unsigned int ? :-) Oops, yes. > What's wrong with the existing arch_spin_is_locked() ? It takes a memory location. And we very much want to test the value we loaded into a register. And yes, gcc can do the right thing. But at least on x86, arch_spin_is_locked() actually uses ACCESS_ONCE() to load the value from the memory location, and I actually think that is the right thing to do (or at least not incorrect). So the end result is that arch_spin_value_unlocked() is actually fairly fundamentally different from arch_spin_is_locked(). So I could have re-used arch_spin_is_locked() after having changed the semantics of it, but I really didn't want to possibly change totally unrelated users for this particular feature. > BTW. Do you have your test case at hand ? My test-case is a joke. It's explicitly *trying* to get as much contention as possible on a dentry, by just starting up a lot of threads that look up one single pathname (the same one for everybody). It defaults to using /tmp for this, but you can specify the filename. Note that directories, regular files and symlinks have fundamentally different dentry lookup behavior: - directories tend to have an elevated reference count (because they have children). This was my primary test-case, because while I suspect that there are crazy loads (and AIM7 may be one of them) that open the same _regular_ file all concurrently, I don't think it's a "normal" load). But opening the same directory concurrently as part of pathname lookup is certainly normal. - regular files tend to have a dentry count of zero unless they are actively open, and the patch I sent out will take the dentry spinlock for them when doing the final RCU finishing touches if that's the case. So this one *will* still use the per-dentry spinlock rather than the lockless refcount increments, but as outlined above I don't think that should be a scalability issue unless you're crazy. - symlink traveral causes us to drop out of RCU lookup mode, and thus cause various slow-paths to happen. Some of that we can improve on, but I suspect it will cause the lockless refcount paths to take a hit too. Anyway, I'm attaching my completely mindless test program. It has hacky things like "unsigned long count[MAXTHREADS][32]" which are purely to just spread out the counts so that they aren't in the same cacheline etc. Also note that the performance numbers it spits out depend a lot on tings like how long the dcache hash chains etc are, so they are not really reliable. Running the test-program right after reboot when the dentries haven't been populated can result in much higher numbers - without that having anything to do with contention or locking at all. Linus --047d7b677efa5cf00704e51f99d4 Content-Type: text/x-csrc; charset=US-ASCII; name="t.c" Content-Disposition: attachment; filename="t.c" Content-Transfer-Encoding: base64 X-Attachment-Id: f_hkyonmtj0 I2luY2x1ZGUgPHN0ZGlvLmg+CiNpbmNsdWRlIDxzdGRsaWIuaD4KI2luY2x1ZGUgPHB0aHJlYWQu aD4KI2luY2x1ZGUgPHN5cy90eXBlcy5oPgojaW5jbHVkZSA8c3lzL3N0YXQuaD4KI2luY2x1ZGUg PHVuaXN0ZC5oPgoKI2RlZmluZSBNQVhUSFJFQURTIDE2CgpzdGF0aWMgdm9sYXRpbGUgaW50IHN0 YXJ0ID0gMDsKc3RhdGljIGNoYXIgKmZpbGUgPSAiL3RtcCI7CnN0YXRpYyB1bnNpZ25lZCBsb25n IGNvdW50W01BWFRIUkVBRFNdWzMyXTsKCnZvaWQgKnN0YXJ0X3JvdXRpbmUodm9pZCAqYXJnKQp7 Cgljb25zdCBjaGFyICpmaWxlbmFtZTsKCXN0cnVjdCBzdGF0IHN0OwoJdW5zaWduZWQgbG9uZyAq Y291bnRlciA9IGFyZzsKCglwdGhyZWFkX3NldGNhbmNlbHR5cGUoUFRIUkVBRF9DQU5DRUxfQVNZ TkNIUk9OT1VTLCBOVUxMKTsKCXdoaWxlICghc3RhcnQpCgkJLyogbm90aGluZyAqLzsKCWZpbGVu YW1lID0gZmlsZTsKCWZvciAoOzspIHsKCQlzdGF0KGZpbGVuYW1lLCAmc3QpOwoJCSsrKmNvdW50 ZXI7Cgl9Cn0KCmludCBtYWluKGludCBhcmdjLCBjaGFyICoqYXJndikKewoJcHRocmVhZF90IHRo cmVhZHNbTUFYVEhSRUFEU107Cgl1bnNpZ25lZCBsb25nIG47CglpbnQgaTsKCglpZiAoYXJndlsx XSkKCQlmaWxlID0gYXJndlsxXTsKCWZvciAoaSA9IDA7IGkgPCBNQVhUSFJFQURTOyBpKyspCgkJ cHRocmVhZF9jcmVhdGUodGhyZWFkcytpLCBOVUxMLCBzdGFydF9yb3V0aW5lLCBjb3VudFtpXSk7 CglzdGFydCA9IDE7CglzbGVlcCgxMCk7Cglmb3IgKGkgPSAwOyBpIDwgTUFYVEhSRUFEUzsgaSsr KQoJCXB0aHJlYWRfY2FuY2VsKHRocmVhZHNbaV0pOwoJZm9yIChpID0gMDsgaSA8IE1BWFRIUkVB RFM7IGkrKykKCQlwdGhyZWFkX2pvaW4odGhyZWFkc1tpXSwgTlVMTCk7CgluID0gMDsKCWZvciAo aSA9IDA7IGkgPCBNQVhUSFJFQURTOyBpKyspCgkJbiArPSBjb3VudFtpXVswXTsKCXByaW50Zigi VG90YWwgbG9vcHM6ICVsdVxuIiwgbik7CglyZXR1cm4gMDsKfQo= --047d7b677efa5cf00704e51f99d4--