> Date: Wed, 5 Jun 2019 10:16:25 +0200
> From: Sebastien Marie <[email protected]>
>
> Hi,
>
> I would like to have feedback and testing on this diff. The initial work
> was done by ian@.
Don't forget to run "make syscalls" in sys/kern when building your own
kernel with this diff!
> It unlocks read(2) and write(2) families, and push the KERNEL_LOCK
> deeper in the code path. With it, read(2) and write(2) on socket will be
> KERNEL_LOCK-free.
>
> read(2) and write(2) are file type agnostics by using `struct fileops'
> (see sys/file.h) which define effective implementation per file type.
>
> Currently, we have the following fileops defined:
> - dmabuf dev/pci/drm/drm_linux.c
> - kqueue kern/kern_event.c
> - pipe kern/sys_pipe.c
> - socket kern/sys_socket.c
> - vnode kern/vfs_vnops.c
>
>
> read(2) family uses dofilereadv(), which calls:
> - fileops::fo_read()
> - FRELE() -> fdrop() -> fileops::fo_close()
>
> write(2) family uses dofilewritev(), which calls:
> - fileops::fo_write()
> - FRELE() -> fdrop() -> fileops::fo_close()
>
>
> The diff pushs the KERNEL_LOCK inside each function, where it is
> effectively needed. But as some file types doesn't need the lock (socket
> for example), it makes read(2) and write(2) lock-free for them, while
> still grabbing the lock for others (vnode for example).
The socket calls will still grab the kernel lock for selwakeup and
sending signals of course. But some of them will indeed be lock free.
It should be fairly easy to also unlock pipes, but that can come
later.
> fileops::fo_read
> - dmabuf_read fine lock-free (just return ENXIO)
> - kqueue_read fine lock-free (just return ENXIO)
> - pipe_read KERNEL_LOCK/UNLOCK added (tsleep, wakeup, selwakeup)
> - soo_read calls soreceive() which is already called without KERNEL_LOCK
> by recvit() (via recvmsg(2))
> - vn_read KERNEL_LOCK/UNLOCK added
>
> fileops::fo_write
> - dmabuf_write fine lock-free (just return ENXIO)
> - kqueue_write fine lock-free (just return ENXIO)
> - pipe_write KERNEL_LOCK/UNLOCK added (tsleep, wakeup, selwakeup)
> - soo_write calls sosend() which is already called without KERNEL_LOCK by
> sendit() (via sendmsg(2))
> - vn_write KERNEL_LOCK/UNLOCK added
>
> fileops::fo_close
> - dmabuf_close already take care of it
> - kqueue_close already take care of it
> - pipe_close already take care of it
> - soo_close call soclose() which is already called without KERNEL_LOCK by
> socketpair(2)
> - vn_closefile already take care of it
>
>
> In dofilewritev(), I take care of calling ptsignal() with the
> KERNEL_LOCK() too, as it requires the lock (it is asserted).
>
> Others functions should be fine to be called without the KERNEL_LOCK, as
> they are already used in such context.
Looks good to me. Certainly needs some testing especially on setups
that do a lot of socket communication. But if testing doesn't reveal
any problems this is ok kettenis@
> Index: kern/sys_generic.c
> ===================================================================
> RCS file: /cvs/src/sys/kern/sys_generic.c,v
> retrieving revision 1.123
> diff -u -p -r1.123 sys_generic.c
> --- kern/sys_generic.c 21 Jan 2019 23:41:26 -0000 1.123
> +++ kern/sys_generic.c 4 Jun 2019 06:19:23 -0000
> @@ -366,8 +366,11 @@ dofilewritev(struct proc *p, int fd, str
> if (uio->uio_resid != cnt && (error == ERESTART ||
> error == EINTR || error == EWOULDBLOCK))
> error = 0;
> - if (error == EPIPE)
> + if (error == EPIPE) {
> + KERNEL_LOCK();
> ptsignal(p, SIGPIPE, STHREAD);
> + KERNEL_UNLOCK();
> + }
> }
> cnt -= uio->uio_resid;
>
> Index: kern/sys_pipe.c
> ===================================================================
> RCS file: /cvs/src/sys/kern/sys_pipe.c,v
> retrieving revision 1.87
> diff -u -p -r1.87 sys_pipe.c
> --- kern/sys_pipe.c 13 Nov 2018 13:02:20 -0000 1.87
> +++ kern/sys_pipe.c 5 Jun 2019 08:06:19 -0000
> @@ -314,9 +314,11 @@ pipe_read(struct file *fp, struct uio *u
> int error;
> size_t size, nread = 0;
>
> + KERNEL_LOCK();
> +
> error = pipelock(rpipe);
> if (error)
> - return (error);
> + goto done;
>
> ++rpipe->pipe_busy;
>
> @@ -420,6 +422,8 @@ unlocked_error:
> if ((rpipe->pipe_buffer.size - rpipe->pipe_buffer.cnt) >= PIPE_BUF)
> pipeselwakeup(rpipe);
>
> +done:
> + KERNEL_UNLOCK();
> return (error);
> }
>
> @@ -430,6 +434,8 @@ pipe_write(struct file *fp, struct uio *
> size_t orig_resid;
> struct pipe *wpipe, *rpipe;
>
> + KERNEL_LOCK();
> +
> rpipe = fp->f_data;
> wpipe = rpipe->pipe_peer;
>
> @@ -437,7 +443,8 @@ pipe_write(struct file *fp, struct uio *
> * detect loss of pipe read side, issue SIGPIPE if lost.
> */
> if ((wpipe == NULL) || (wpipe->pipe_state & PIPE_EOF)) {
> - return (EPIPE);
> + error = EPIPE;
> + goto done;
> }
> ++wpipe->pipe_busy;
>
> @@ -471,7 +478,7 @@ pipe_write(struct file *fp, struct uio *
> wpipe->pipe_state &= ~(PIPE_WANT | PIPE_WANTR);
> wakeup(wpipe);
> }
> - return (error);
> + goto done;
> }
>
> orig_resid = uio->uio_resid;
> @@ -642,6 +649,8 @@ retrywrite:
> if (wpipe->pipe_buffer.cnt)
> pipeselwakeup(wpipe);
>
> +done:
> + KERNEL_UNLOCK();
> return (error);
> }
>
> Index: kern/syscalls.master
> ===================================================================
> RCS file: /cvs/src/sys/kern/syscalls.master,v
> retrieving revision 1.191
> diff -u -p -r1.191 syscalls.master
> --- kern/syscalls.master 13 May 2019 22:55:27 -0000 1.191
> +++ kern/syscalls.master 5 Jun 2019 08:06:19 -0000
> @@ -51,8 +51,8 @@
> 0 INDIR { int sys_syscall(int number, ...); }
> 1 STD { void sys_exit(int rval); }
> 2 STD { int sys_fork(void); }
> -3 STD { ssize_t sys_read(int fd, void *buf, size_t nbyte); }
> -4 STD { ssize_t sys_write(int fd, const void *buf, \
> +3 STD NOLOCK { ssize_t sys_read(int fd, void *buf, size_t nbyte); }
> +4 STD NOLOCK { ssize_t sys_write(int fd, const void *buf, \
> size_t nbyte); }
> 5 STD { int sys_open(const char *path, \
> int flags, ... mode_t mode); }
> @@ -247,9 +247,9 @@
> 118 STD { int sys_getsockopt(int s, int level, int name, \
> void *val, socklen_t *avalsize); }
> 119 STD { int sys_thrkill(pid_t tid, int signum, void *tcb); }
> -120 STD { ssize_t sys_readv(int fd, \
> +120 STD NOLOCK { ssize_t sys_readv(int fd, \
> const struct iovec *iovp, int iovcnt); }
> -121 STD { ssize_t sys_writev(int fd, \
> +121 STD NOLOCK { ssize_t sys_writev(int fd, \
> const struct iovec *iovp, int iovcnt); }
> 122 STD { int sys_kill(int pid, int signum); }
> 123 STD { int sys_fchown(int fd, uid_t uid, gid_t gid); }
> @@ -316,9 +316,9 @@
> 170 OBSOL msgsys10
> 171 OBSOL shmsys10
> 172 UNIMPL
> -173 STD { ssize_t sys_pread(int fd, void *buf, \
> +173 STD NOLOCK { ssize_t sys_pread(int fd, void *buf, \
> size_t nbyte, int pad, off_t offset); }
> -174 STD { ssize_t sys_pwrite(int fd, const void *buf, \
> +174 STD NOLOCK { ssize_t sys_pwrite(int fd, const void *buf, \
> size_t nbyte, int pad, off_t offset); }
> 175 UNIMPL ntp_gettime
> 176 UNIMPL ntp_adjtime
> @@ -452,10 +452,10 @@
> 264 STD { int sys_fhopen(const fhandle_t *fhp, int flags); }
> 265 UNIMPL
> 266 UNIMPL
> -267 STD { ssize_t sys_preadv(int fd, \
> +267 STD NOLOCK { ssize_t sys_preadv(int fd, \
> const struct iovec *iovp, int iovcnt, \
> int pad, off_t offset); }
> -268 STD { ssize_t sys_pwritev(int fd, \
> +268 STD NOLOCK { ssize_t sys_pwritev(int fd, \
> const struct iovec *iovp, int iovcnt, \
> int pad, off_t offset); }
> 269 STD { int sys_kqueue(void); }
> Index: kern/vfs_vnops.c
> ===================================================================
> RCS file: /cvs/src/sys/kern/vfs_vnops.c,v
> retrieving revision 1.97
> diff -u -p -r1.97 vfs_vnops.c
> --- kern/vfs_vnops.c 20 Aug 2018 16:00:22 -0000 1.97
> +++ kern/vfs_vnops.c 5 Jun 2019 08:06:19 -0000
> @@ -343,6 +343,8 @@ vn_read(struct file *fp, struct uio *uio
> off_t offset;
> int error;
>
> + KERNEL_LOCK();
> +
> /*
> * Check below can race. We can block on the vnode lock
> * and resume with a different `fp->f_offset' value.
> @@ -353,11 +355,15 @@ vn_read(struct file *fp, struct uio *uio
> offset = uio->uio_offset;
>
> /* no wrap around of offsets except on character devices */
> - if (vp->v_type != VCHR && count > LLONG_MAX - offset)
> - return (EINVAL);
> + if (vp->v_type != VCHR && count > LLONG_MAX - offset) {
> + error = EINVAL;
> + goto done;
> + }
>
> - if (vp->v_type == VDIR)
> - return (EISDIR);
> + if (vp->v_type == VDIR) {
> + error = EISDIR;
> + goto done;
> + }
>
> vn_lock(vp, LK_EXCLUSIVE | LK_RETRY);
> if ((fflags & FO_POSITION) == 0)
> @@ -367,6 +373,8 @@ vn_read(struct file *fp, struct uio *uio
> if ((fflags & FO_POSITION) == 0)
> fp->f_offset += count - uio->uio_resid;
> VOP_UNLOCK(vp);
> +done:
> + KERNEL_UNLOCK();
> return (error);
> }
>
> @@ -381,6 +389,8 @@ vn_write(struct file *fp, struct uio *ui
> int error, ioflag = IO_UNIT;
> size_t count;
>
> + KERNEL_LOCK();
> +
> /* note: pwrite/pwritev are unaffected by O_APPEND */
> if (vp->v_type == VREG && (fp->f_flag & O_APPEND) &&
> (fflags & FO_POSITION) == 0)
> @@ -402,6 +412,8 @@ vn_write(struct file *fp, struct uio *ui
> fp->f_offset += count - uio->uio_resid;
> }
> VOP_UNLOCK(vp);
> +
> + KERNEL_UNLOCK();
> return (error);
> }
>
> Index: sys/file.h
> ===================================================================
> RCS file: /cvs/src/sys/sys/file.h,v
> retrieving revision 1.53
> diff -u -p -r1.53 file.h
> --- sys/file.h 20 Aug 2018 16:00:22 -0000 1.53
> +++ sys/file.h 5 Jun 2019 08:06:19 -0000
> @@ -46,6 +46,13 @@ struct stat;
> struct file;
> struct ucred;
>
> +/**
> + * File operations.
> + * The following entries could be called without KERNEL_LOCK hold:
> + * - fo_read
> + * - fo_write
> + * - fo_close
> + */
> struct fileops {
> int (*fo_read)(struct file *, struct uio *, int);
> int (*fo_write)(struct file *, struct uio *, int);
>
>
