On Tue, Jun 19, 2018 at 03:36:57PM +0000, Visa Hankala wrote:
> On Tue, Jun 19, 2018 at 03:58:51PM +0200, Mark Kettenis wrote:
> > > Date: Tue, 19 Jun 2018 15:38:01 +0200
> > > From: Martin Pieuchot <[email protected]>
> > >
> > > On 19/06/18(Tue) 14:55, Mark Kettenis wrote:
> > > > > To avoid races with another thread that might be clearing our pointer
> > > > > in `fd_ofiles', we need more than atomic operations. For that we need
> > > > > to serialize the threads. The most simple way to do so is with a
> > > > > mutex
> > > > > on a different data structure. Either global, like in my diff, or as
> > > > > visa@ suggested in the corresponding `fdp'.
> > > >
> > > > Right. Another case of trying to take a reference without holding one
> > > > already. The trick here is to use the fact that as long as there is a
> > > > file descriptor allocated for this open file the reference count is at
> > > > least 1. So in that case taking a reference is safe. Your global
> > > > mutex does indeed do the trick. But why aren't you using the file
> > > > descriptor table rwlock for this purpose? Is that because there is a
> > > > lock ordering problem between the kernel lock and that rwlock?
> > >
> > > I have two reasons. First I don't want to introduce new sleeping points,
> > > secondly I want this mutex to disappear. IMHO extending the scope of a
> > > lock is going in the wrong direction because then we'll want to split it.
> > > That's why I'm happy that our discussion made visa@ look at improving
> > > this.
> >
> > I wouldn't call this extending the scope of the lock. But yes, it
> > might cause unnecessary sleeps if a write lock is held for the purpose
> > of opening a file. The mutex that visa@ proposes trades that in for
> > potential contion in the multiple-readers case.
>
> Below is a revised version of the diff. Like mpi@'s diff, it uses
> a `fhdlk' mutex for serializing access to the file list. However, that
> lock is not used for anything else. Each file descriptor table has a
> dedicated mutex that allows fd_getfile() acquire a file reference
> safely.
>
> fd_iterfile() uses a compare-and-swap sequence for reference
> acquisition. The sequence ensures that once `f_count' has become zero
> the value is never raised.
Here is an updated diff that has been adjusted for the current tree.
OK?
Index: kern/kern_descrip.c
===================================================================
RCS file: src/sys/kern/kern_descrip.c,v
retrieving revision 1.167
diff -u -p -r1.167 kern_descrip.c
--- kern/kern_descrip.c 20 Jun 2018 10:52:49 -0000 1.167
+++ kern/kern_descrip.c 21 Jun 2018 16:10:39 -0000
@@ -198,6 +198,7 @@ struct file *
fd_iterfile(struct file *fp, struct proc *p)
{
struct file *nfp;
+ unsigned int count;
mtx_enter(&fhdlk);
if (fp == NULL)
@@ -206,10 +207,15 @@ fd_iterfile(struct file *fp, struct proc
nfp = LIST_NEXT(fp, f_list);
/* don't refcount when f_count == 0 to avoid race in fdrop() */
- while (nfp != NULL && nfp->f_count == 0)
- nfp = LIST_NEXT(nfp, f_list);
- if (nfp != NULL)
- nfp->f_count++;
+ while (nfp != NULL) {
+ count = nfp->f_count;
+ if (count == 0) {
+ nfp = LIST_NEXT(nfp, f_list);
+ continue;
+ }
+ if (atomic_cas_uint(&nfp->f_count, count, count + 1) == count)
+ break;
+ }
mtx_leave(&fhdlk);
if (fp != NULL)
@@ -228,11 +234,11 @@ fd_getfile(struct filedesc *fdp, int fd)
if ((u_int)fd >= fdp->fd_nfiles)
return (NULL);
- mtx_enter(&fhdlk);
+ mtx_enter(&fdp->fd_fplock);
fp = fdp->fd_ofiles[fd];
if (fp != NULL)
- fp->f_count++;
- mtx_leave(&fhdlk);
+ atomic_inc_int(&fp->f_count);
+ mtx_leave(&fdp->fd_fplock);
return (fp);
}
@@ -652,7 +658,7 @@ finishdup(struct proc *p, struct file *f
fdpassertlocked(fdp);
KASSERT(fp->f_iflags & FIF_INSERTED);
- if (fp->f_count == LONG_MAX-2) {
+ if (fp->f_count >= FDUP_MAX_COUNT) {
FRELE(fp, p);
return (EDEADLK);
}
@@ -666,8 +672,10 @@ finishdup(struct proc *p, struct file *f
fd_used(fdp, new);
}
+ mtx_enter(&fdp->fd_fplock);
fdp->fd_ofiles[new] = fp;
fdp->fd_ofileflags[new] = fdp->fd_ofileflags[old] & ~UF_EXCLOSE;
+ mtx_leave(&fdp->fd_fplock);
*retval = new;
if (oldfp != NULL) {
@@ -686,24 +694,35 @@ fdinsert(struct filedesc *fdp, int fd, i
fdpassertlocked(fdp);
mtx_enter(&fhdlk);
+ fp->f_iflags |= FIF_INSERTED;
if ((fq = fdp->fd_ofiles[0]) != NULL) {
LIST_INSERT_AFTER(fq, fp, f_list);
} else {
LIST_INSERT_HEAD(&filehead, fp, f_list);
}
+ mtx_leave(&fhdlk);
+
+ mtx_enter(&fdp->fd_fplock);
KASSERT(fdp->fd_ofiles[fd] == NULL);
fdp->fd_ofiles[fd] = fp;
fdp->fd_ofileflags[fd] |= (flags & UF_EXCLOSE);
- fp->f_iflags |= FIF_INSERTED;
- mtx_leave(&fhdlk);
+ mtx_leave(&fdp->fd_fplock);
}
void
fdremove(struct filedesc *fdp, int fd)
{
fdpassertlocked(fdp);
+
+ /*
+ * Use `fd_fplock' to synchronize with fd_getfile() so that
+ * the function no longer creates a new reference to the file.
+ */
+ mtx_enter(&fdp->fd_fplock);
fdp->fd_ofiles[fd] = NULL;
fdp->fd_ofileflags[fd] = 0;
+ mtx_leave(&fdp->fd_fplock);
+
fd_unused(fdp, fd);
}
@@ -833,6 +852,8 @@ fdalloc(struct proc *p, int want, int *r
int lim, last, i;
u_int new, off;
+ fdpassertlocked(fdp);
+
/*
* Search for a free descriptor starting at the higher
* of want or fd_freefile. If that fails, consider
@@ -880,14 +901,17 @@ void
fdexpand(struct proc *p)
{
struct filedesc *fdp = p->p_fd;
- int nfiles;
+ int nfiles, oldnfiles;
size_t copylen;
- struct file **newofile;
+ struct file **newofile, **oldofile;
char *newofileflags;
u_int *newhimap, *newlomap;
fdpassertlocked(fdp);
+ oldnfiles = fdp->fd_nfiles;
+ oldofile = fdp->fd_ofiles;
+
/*
* No space in current array.
*/
@@ -921,9 +945,6 @@ fdexpand(struct proc *p)
memcpy(newofileflags, fdp->fd_ofileflags, copylen);
memset(newofileflags + copylen, 0, nfiles * sizeof(char) - copylen);
- if (fdp->fd_nfiles > NDFILE)
- free(fdp->fd_ofiles, M_FILEDESC, fdp->fd_nfiles * OFILESIZE);
-
if (NDHISLOTS(nfiles) > NDHISLOTS(fdp->fd_nfiles)) {
copylen = NDHISLOTS(fdp->fd_nfiles) * sizeof(u_int);
memcpy(newhimap, fdp->fd_himap, copylen);
@@ -944,9 +965,17 @@ fdexpand(struct proc *p)
fdp->fd_himap = newhimap;
fdp->fd_lomap = newlomap;
}
+
+ mtx_enter(&fdp->fd_fplock);
fdp->fd_ofiles = newofile;
fdp->fd_ofileflags = newofileflags;
- fdp->fd_nfiles = nfiles;
+ /* Ensure the new arrays are in use when the size is updated. */
+ membar_producer();
+ fdp->fd_nfiles = nfiles;
+ mtx_leave(&fdp->fd_fplock);
+
+ if (oldnfiles > NDFILE)
+ free(oldofile, M_FILEDESC, oldnfiles * OFILESIZE);
}
/*
@@ -982,7 +1011,7 @@ restart:
* of open files at that point, otherwise put it at the front of
* the list of open files.
*/
- numfiles++;
+ atomic_inc_int(&numfiles);
fp = pool_get(&file_pool, PR_WAITOK|PR_ZERO);
/*
* We need to block interrupts as long as `f_mtx' is being taken
@@ -1012,6 +1041,7 @@ fdinit(void)
newfdp = pool_get(&fdesc_pool, PR_WAITOK|PR_ZERO);
rw_init(&newfdp->fd_fd.fd_lock, "fdlock");
+ mtx_init(&newfdp->fd_fd.fd_fplock, IPL_MPFLOOR);
/* Create the file descriptor table. */
newfdp->fd_fd.fd_refcnt = 1;
@@ -1102,7 +1132,7 @@ fdcopy(struct process *pr)
* tied to the process that opened them to enforce
* their internal consistency, so close them here.
*/
- if (fp->f_count == LONG_MAX-2 ||
+ if (fp->f_count >= FDUP_MAX_COUNT ||
fp->f_type == DTYPE_KQUEUE)
continue;
@@ -1172,11 +1202,9 @@ closef(struct file *fp, struct proc *p)
if (fp == NULL)
return (0);
- KASSERTMSG(fp->f_count >= 2, "count (%ld) < 2", fp->f_count);
+ KASSERTMSG(fp->f_count >= 2, "count (%u) < 2", fp->f_count);
- mtx_enter(&fhdlk);
- fp->f_count--;
- mtx_leave(&fhdlk);
+ atomic_dec_int(&fp->f_count);
/*
* POSIX record locking dictates that any close releases ALL
@@ -1208,10 +1236,9 @@ fdrop(struct file *fp, struct proc *p)
{
int error;
- MUTEX_ASSERT_LOCKED(&fhdlk);
-
- KASSERTMSG(fp->f_count == 0, "count (%ld) != 0", fp->f_count);
+ KASSERTMSG(fp->f_count == 0, "count (%u) != 0", fp->f_count);
+ mtx_enter(&fhdlk);
if (fp->f_iflags & FIF_INSERTED)
LIST_REMOVE(fp, f_list);
mtx_leave(&fhdlk);
@@ -1222,7 +1249,7 @@ fdrop(struct file *fp, struct proc *p)
error = 0;
crfree(fp->f_cred);
- numfiles--;
+ atomic_dec_int(&numfiles);
pool_put(&file_pool, fp);
return (error);
@@ -1348,7 +1375,7 @@ dupfdopen(struct proc *p, int indx, int
FRELE(wfp, p);
return (EACCES);
}
- if (wfp->f_count == LONG_MAX-2) {
+ if (wfp->f_count >= FDUP_MAX_COUNT) {
FRELE(wfp, p);
return (EDEADLK);
}
Index: kern/uipc_usrreq.c
===================================================================
RCS file: src/sys/kern/uipc_usrreq.c,v
retrieving revision 1.130
diff -u -p -r1.130 uipc_usrreq.c
--- kern/uipc_usrreq.c 20 Jun 2018 10:52:49 -0000 1.130
+++ kern/uipc_usrreq.c 21 Jun 2018 16:10:39 -0000
@@ -845,7 +845,7 @@ morespace:
error = EBADF;
goto fail;
}
- if (fp->f_count == LONG_MAX-2) {
+ if (fp->f_count >= FDUP_MAX_COUNT) {
error = EDEADLK;
goto fail;
}
Index: sys/file.h
===================================================================
RCS file: src/sys/sys/file.h,v
retrieving revision 1.49
diff -u -p -r1.49 file.h
--- sys/file.h 20 Jun 2018 10:52:49 -0000 1.49
+++ sys/file.h 21 Jun 2018 16:10:39 -0000
@@ -66,11 +66,12 @@ struct fileops {
* Locks used to protect struct members in this file:
* I immutable after creation
* F global `fhdlk' mutex
+ * a atomic operations
* f per file `f_mtx'
* k kernel lock
*/
struct file {
- LIST_ENTRY(file) f_list;/* [k] list of active files */
+ LIST_ENTRY(file) f_list;/* [F] list of active files */
struct mutex f_mtx;
short f_flag; /* [k] see fcntl.h */
#define DTYPE_VNODE 1 /* file */
@@ -78,7 +79,7 @@ struct file {
#define DTYPE_PIPE 3 /* pipe */
#define DTYPE_KQUEUE 4 /* event queue */
short f_type; /* [I] descriptor type */
- long f_count; /* [F] reference count */
+ u_int f_count; /* [a] reference count */
struct ucred *f_cred; /* [I] credentials associated with descriptor */
struct fileops *f_ops; /* [I] file operation pointers */
off_t f_offset; /* [k] */
@@ -98,21 +99,13 @@ struct file {
do { \
extern void vfs_stall_barrier(void); \
vfs_stall_barrier(); \
- mtx_enter(&fhdlk); \
- (fp)->f_count++; \
- mtx_leave(&fhdlk); \
+ atomic_inc_int(&(fp)->f_count); \
} while (0)
#define FRELE(fp,p) \
-({ \
- int rv = 0; \
- mtx_enter(&fhdlk); \
- if (--(fp)->f_count == 0) \
- rv = fdrop(fp, p); \
- else \
- mtx_leave(&fhdlk); \
- rv; \
-})
+ (atomic_dec_int_nv(&fp->f_count) == 0 ? fdrop(fp, p) : 0)
+
+#define FDUP_MAX_COUNT (UINT_MAX - 2 * MAXCPUS)
int fdrop(struct file *, struct proc *);
Index: sys/filedesc.h
===================================================================
RCS file: src/sys/sys/filedesc.h,v
retrieving revision 1.39
diff -u -p -r1.39 filedesc.h
--- sys/filedesc.h 18 Jun 2018 09:15:05 -0000 1.39
+++ sys/filedesc.h 21 Jun 2018 16:10:39 -0000
@@ -32,6 +32,7 @@
* @(#)filedesc.h 8.1 (Berkeley) 6/2/93
*/
+#include <sys/mutex.h>
#include <sys/rwlock.h>
/*
* This structure is used for the management of descriptors. It may be
@@ -72,6 +73,8 @@ struct filedesc {
struct rwlock fd_lock; /* lock for the file descs; must be */
/* held when writing to fd_ofiles, */
/* fd_ofileflags, or fd_{hi,lo}map */
+ struct mutex fd_fplock; /* lock for reading fd_ofiles without
+ * fd_lock */
int fd_flags; /* flags on the file descriptor table */
};
