Date: Thu, 14 Aug 2014 08:41:47 -0400 From: John Baldwin <jhb@freebsd.org> To: freebsd-arch@freebsd.org Cc: Mateusz Guzik <mjguzik@gmail.com>, kib@freebsd.org Subject: Re: Getting rid of atomic_load_acq_int(&fdp->fd_nfiles)) from fget_unlocked Message-ID: <201408140841.47510.jhb@freebsd.org> In-Reply-To: <20140713035500.GC16884@dft-labs.eu> References: <20140713035500.GC16884@dft-labs.eu>
next in thread | previous in thread | raw e-mail | index | archive | help
On Saturday, July 12, 2014 11:55:00 pm Mateusz Guzik wrote: > Currently: > /* > * Avoid reads reordering and then a first access to the > * fdp->fd_ofiles table which could result in OOB operation. > */ > if (fd < 0 || fd >= atomic_load_acq_int(&fdp->fd_nfiles)) > return (EBADF); > > However, if we put fd_nfiles and fd_otable into one atomically replaced > structure the only need to: > 1. make sure the pointer is read once > 2. issue a data dependency barrier - this is a noop on all supported > architectures and we don't even have approprate macro, so doing nothing > seems fine > > The motivation is to boost performance to amortize for seqlock cost, in > case it hits the tree. > > This has no impact on races with capability lookup. > > In a microbenchmark of 16 threads reading from the same pipe fd > immediately returning EAGAIN the numbers are: > x vanilla-readpipe-run-sum > + noacq-readpipe-run-sum > [..] > N Min Max Median Avg Stddev > x 20 13133671 14900364 13893331 13827075 471500.82 > + 20 59479718 59527286 59496714 59499504 13752.968 > Difference at 95.0% confidence > 4.56724e+07 +/- 213483 > 330.312% +/- 1.54395% > > There are 3 steps: > 1. tidy up capsicum to accept fde: > http://people.freebsd.org/~mjg/patches/single-fdtable-read-capsicum.patch The KASSERT() on the 'fd' being valid was lost from cap_fcntl_check(). Should probably put that back. However, cap_fcntl_check() is now no longer used. Instead, you should just rename cap_fcntl_check_fd() to cap_fcntl_check() and remove the old one. > 2. add __READ_ONCE: > http://people.freebsd.org/~mjg/patches/read-once.patch > 3. put stuff into one structure: > http://people.freebsd.org/~mjg/patches/filedescenttable.patch Anotehr name besides the one suggested by Konstantin could just be 'struct filetable'. Even shorter while not having abbreviations. Also, I think you can leave the comment in 'struct filedesc' as just 'open files' in that case. -- John Baldwin
Want to link to this message? Use this URL: <https://mail-archive.FreeBSD.org/cgi/mid.cgi?201408140841.47510.jhb>