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.
Sometimes a "correct" change exposes a bug and we try not to break
-current, so we cannot afford to commit a non-tested diff.
> 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?
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);
}
