From: Vitaliy Makkoveev <mvs@openbsd.org>
Subject: Re: Push KERNEL_LOCK() into open(2) & openat(2)
To: tech@openbsd.org
Date: Wed, 22 Jan 2025 14:33:43 +0300

On Wed, Jan 22, 2025 at 12:01:49PM +0100, Martin Pieuchot wrote:
> Here's the next step from my VFS unlock diff sent some weeks ago.
> 
> This one pushes the KERNEL_LOCK() around vn_open() in doopenat().  This
> should give a small boost and make contention clearer in FlameGraphs.
> 
> ok?
> 

ok mvs

> Index: kern/syscalls.master
> ===================================================================
> RCS file: /cvs/src/sys/kern/syscalls.master,v
> diff -u -p -r1.265 syscalls.master
> --- kern/syscalls.master	2 Aug 2024 14:34:45 -0000	1.265
> +++ kern/syscalls.master	28 Nov 2024 18:55:36 -0000
> @@ -52,7 +52,7 @@
>  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, \
> +5	STD NOLOCK	{ int sys_open(const char *path, \
>  			    int flags, ... mode_t mode); }
>  6	STD NOLOCK	{ int sys_close(int fd); }
>  7	STD NOLOCK	{ int sys_getentropy(void *buf, size_t nbyte); }
> @@ -553,7 +553,7 @@
>  			    mode_t mode); }
>  320	STD		{ int sys_mknodat(int fd, const char *path, \
>  			    mode_t mode, dev_t dev); }
> -321	STD		{ int sys_openat(int fd, const char *path, int flags, \
> +321	STD NOLOCK	{ int sys_openat(int fd, const char *path, int flags, \
>  			    ... mode_t mode); }
>  322	STD		{ ssize_t sys_readlinkat(int fd, const char *path, \
>  			    char *buf, size_t count); }
> Index: kern/vfs_syscalls.c
> ===================================================================
> RCS file: /cvs/src/sys/kern/vfs_syscalls.c,v
> diff -u -p -r1.370 vfs_syscalls.c
> --- kern/vfs_syscalls.c	5 Nov 2024 06:03:19 -0000	1.370
> +++ kern/vfs_syscalls.c	28 Nov 2024 18:19:24 -0000
> @@ -1120,23 +1126,20 @@ doopenat(struct proc *p, int fd, const c
>  		localtrunc = 1;
>  		flags &= ~O_TRUNC;	/* Must do truncate ourselves */
>  	}
> +	KERNEL_LOCK();
>  	if ((error = vn_open(&nd, flags, cmode)) != 0) {
>  		fdplock(fdp);
>  		if (error == ENODEV &&
>  		    p->p_dupfd >= 0 &&			/* XXX from fdopen */
>  		    (error =
>  			dupfdopen(p, indx, flags)) == 0) {
> -			fdpunlock(fdp);
> -			closef(fp, p);
>  			*retval = indx;
> -			return (error);
> +			goto error;
>  		}
>  		if (error == ERESTART)
>  			error = EINTR;
>  		fdremove(fdp, indx);
> -		fdpunlock(fdp);
> -		closef(fp, p);
> -		return (error);
> +		goto error;
>  	}
>  	p->p_dupfd = 0;
>  	vp = nd.ni_vp;
> @@ -1161,9 +1164,7 @@ doopenat(struct proc *p, int fd, const c
>  			fdplock(fdp);
>  			/* closef will vn_close the file for us. */
>  			fdremove(fdp, indx);
> -			fdpunlock(fdp);
> -			closef(fp, p);
> -			return (error);
> +			goto error;
>  		}
>  		vn_lock(vp, LK_EXCLUSIVE | LK_RETRY);
>  		atomic_setbits_int(&fp->f_iflags, FIF_HASLOCK);
> @@ -1185,18 +1186,22 @@ doopenat(struct proc *p, int fd, const c
>  			fdplock(fdp);
>  			/* closef will close the file for us. */
>  			fdremove(fdp, indx);
> -			fdpunlock(fdp);
> -			closef(fp, p);
> -			return (error);
> +			goto error;
>  		}
>  	}
>  	VOP_UNLOCK(vp);
> +	KERNEL_UNLOCK();
>  	*retval = indx;
>  	fdplock(fdp);
>  	fdinsert(fdp, indx, cloexec, fp);
>  	fdpunlock(fdp);
>  	FRELE(fp, p);
>  	return (error);
> +error:
> +	KERNEL_UNLOCK();
> +	fdpunlock(fdp);
> +	closef(fp, p);
> +	return (error);
>  }
>  
>  /*
> 
>