On Wed, Oct 11, 2017 at 03:15:42PM +0200, Martin Pieuchot wrote: > On 10/10/17(Tue) 20:19, Mateusz Guzik wrote: > > On Tue, Oct 10, 2017 at 10:15:48AM +0200, Martin Pieuchot wrote: > > > Hello Mateusz, > > > > > > On 09/10/17(Mon) 21:43, Mateusz Guzik wrote: > > > > I was looking at rw lock code out of curiosity and noticed you always do > > > > membar_enter which on MP-enabled amd64 kernel translates to mfence. > > > > This makes the entire business a little bit slower. > > > > > > > > Interestingly you already have relevant macros for amd64: > > > > #define membar_enter_after_atomic() __membar("") > > > > #define membar_exit_before_atomic() __membar("") > > > > > > If you look at the history, you'll see that theses macro didn't exist > > > when membar_* where added to rw locks. > > > > > > > I know. Addition of such a macro sounds like an accelent opportunity to > > examine existing users. I figured rw locks were ommitted by accident > > as the original posting explicitly mentions mutexes. > > > > https://marc.info/?l=openbsd-tech&m=149555411827749&w=2 > > > > > > And there is even a default variant for archs which don't provide one. > > > > I guess the switch should be easy. > > > > > > Then please do it :) At lot of stuff is easy on OpenBSD but we are not > > > enough. > > > > > > Do it, test it, explain it, get oks and commit it 8) > > > > > > > As noted earlier the patch is trivial. I have no easy means to even > > compile-test it though as I'm not using OpenBSD. Only had a look out of > > curiosity. > > Why don't you start using it? Testing is what makes the difference. > Many of us have ideas of how to improve the kernel but we're lacking > the time to test & debug them all. >
I'm playing with concurrency as a hobby and smpifying single-threaded code is above the pain threshold I'm willing to accept in my spare time. > Sometimes a "correct" change exposes a bug and we try not to break > -current, so we cannot afford to commit a non-tested diff. > I definitely don't advocate for just committing stuff. I do state though that there should be no binary change for archs other than i386 and amd64 (== nothing to test there). Given the nature of the change for these two it really seemed it just fell through the cracks earlier and I just wanted to point it out as perhaps someone will find cycles to pick it up and test on relevant platforms. > > Nonetheless, here is the diff which can be split in 2. One part deals > > with barriers, another one cleans up rw_status. > > > > [...] > > > > Read from the lock only once in rw_status. The lock word is marked as > > volatile which forces re-read on each access. There is no correctness > > issue here, but the assembly is worse than it needs to be and in case > > of contention this adds avoidable cache traffic. > > Here's the diff to cleans rw_status. The difference on amd64 with > clang 4.0 is the following: > > 48 8b 0f mov (%rdi),%rcx > f6 c1 04 test $0x4,%cl > 75 0c jne 738 <rrw_status+0x18> > 31 c0 xor %eax,%eax > - 48 83 3f 00 cmpq $0x0,(%rdi) > + 48 85 c9 test %rcx,%rcx > > ok? There should be another change past rrw_status+0x18: ffffffff812557e8: 65 48 8b 04 25 08 00 mov %gs:0x8,%rax ffffffff812557ef: 00 00 ffffffff812557f1: 48 8b 80 90 02 00 00 mov 0x290(%rax),%rax ffffffff812557f8: 48 33 07 xor (%rdi),%rax > > Index: kern/kern_rwlock.c > =================================================================== > RCS file: /cvs/src/sys/kern/kern_rwlock.c,v > retrieving revision 1.30 > diff -u -p -r1.30 kern_rwlock.c > --- kern/kern_rwlock.c 12 Aug 2017 23:27:44 -0000 1.30 > +++ kern/kern_rwlock.c 11 Oct 2017 12:59:19 -0000 > @@ -312,13 +312,15 @@ _rw_exit(struct rwlock *rwl LOCK_FL_VARS > int > rw_status(struct rwlock *rwl) > { > - if (rwl->rwl_owner & RWLOCK_WRLOCK) { > - if (RW_PROC(curproc) == RW_PROC(rwl->rwl_owner)) > + unsigned long owner = rwl->rwl_owner; > + > + if (owner & RWLOCK_WRLOCK) { > + if (RW_PROC(curproc) == RW_PROC(owner)) > return RW_WRITE; > else > return RW_WRITE_OTHER; > } > - if (rwl->rwl_owner) > + if (owner) > return RW_READ; > return (0); > } -- Mateusz Guzik