Date: Sat, 12 Jan 2002 18:59:45 -0800 (PST) From: John Baldwin <jhb@FreeBSD.org> To: John Baldwin <jhb@FreeBSD.org> Cc: smp@freebsd.org, dillon@freebsd.org, tanimura@freebsd.org, Alfred Perlstein <bright@mu.org> Subject: Re: fd locking. Message-ID: <XFMail.020112185945.jhb@FreeBSD.org> In-Reply-To: <XFMail.020112174456.jhb@FreeBSD.org>
next in thread | previous in thread | raw e-mail | index | archive | help
On 13-Jan-02 John Baldwin wrote: > > On 13-Jan-02 Alfred Perlstein wrote: >> * Alfred Perlstein <bright@mu.org> [020112 03:11] wrote: >>> I've got world building with these patches. >>> >>> http://people.freebsd.org/~alfred/fd.diff >>> >>> or >>> >>> http://people.freebsd.org/~alfred/fd.diff.gz Why did you add the else here: diff -u -r1.9 svr4_ioctl.c --- compat/svr4/svr4_ioctl.c 2001/09/12 08:36:58 1.9 +++ compat/svr4/svr4_ioctl.c 2002/01/08 22:26:13 @@ -100,21 +100,22 @@ dir, c, num, argsiz, SCARG(uap, data))); #endif retval = td->td_retval; - fdp = td->td_proc->p_fd; cmd = SCARG(uap, com); - if ((u_int)SCARG(uap, fd) >= fdp->fd_nfiles || - (fp = fdp->fd_ofiles[SCARG(uap, fd)]) == NULL) + fp = ffind_hold(td, uap->fd); + if (fp == NULL) return EBADF; - if ((fp->f_flag & (FREAD | FWRITE)) == 0) + if ((fp->f_flag & (FREAD | FWRITE)) == 0) { + fdrop(fp, td); return EBADF; + } #if defined(DEBUG_SVR4) if (fp->f_type == DTYPE_SOCKET) { struct socket *so = (struct socket *)fp->f_data; DPRINTF(("<<< IN: so_state = 0x%x\n", so->so_state)); - } + } else #endif switch (cmd & 0xff00) { and another proc -> thread bug at the bottom of this hunk: @@ -145,17 +146,23 @@ case SVR4_XIOC: /* We do not support those */ + fdrop(fp, td); return EINVAL; default: + fdrop(fp, td); DPRINTF(("Unimplemented ioctl %lx\n", cmd)); return 0; /* XXX: really ENOSYS */ } #if defined(DEBUG_SVR4) if (fp->f_type == DTYPE_SOCKET) { - struct socket *so = (struct socket *)fp->f_data; + struct socket *so; + + so = (struct socket *)fp->f_data; DPRINTF((">>> OUT: so_state = 0x%x\n", so->so_state)); } #endif - return (*fun)(fp, td, retval, SCARG(uap, fd), cmd, SCARG(uap, data)); + error = (*fun)(fp, p, retval, SCARG(uap, fd), cmd, SCARG(uap, data)); + fdrop(fp, td); + return (error); } And another one: diff -u -r1.34 svr4_misc.c --- compat/svr4/svr4_misc.c 2001/10/10 23:06:52 1.34 +++ compat/svr4/svr4_misc.c 2002/01/08 22:30:32 @@ -400,9 +405,10 @@ eof: td->td_retval[0] = nbytes - resid; out: + VOP_UNLOCK(vp, 0, p); + fdrop(fp, td); if (cookies) free(cookies, M_TEMP); - VOP_UNLOCK(vp, 0, td); free(buf, M_TEMP); return error; } hmm, some more, have you compiled this? :) Might want to compile LINT (minus usb since that is broken) and fix the new warnings In svr4_stream.c, do_putmsg and do_getmsg should be taking a thread not a proc as their first argument and callers should be fixed as well. You might consider a file_init() and file_destroy() function or macro like so: void file_init(struct file *fp) { mtx_init(&fp->f_mtx, "struct file", MTX_DEF); fp->f_count = 1; } void file_destroy(struct file *fp) { mtx_destroy(&fp->f_mtx); } I would just make file_destroy a macro for now. Having file_init a function would save a little space since the "struct file" string wouldn't be duplicated, but you can always change it later. You could add more stuff if needed as well. :) Hmm, do you think you could change fdalloc() to take a filedesc * instead of a thread so it's clearer when you lock the old filedesc that it is being used? hmm, for this code: + mtx_init(&fp->f_mtx, "file structure", MTX_DEF); + fp->f_gcflag = 0; fp->f_count = 1; fp->f_cred = crhold(p->p_ucred); fp->f_ops = &badfileops; fp->f_seqcount = 1; + FILEDESC_UNLOCK(p->p_fd); + sx_xlock(&filelist_lock); + FILEDESC_LOCK(p->p_fd); if ((fq = p->p_fd->fd_ofiles[0])) { LIST_INSERT_AFTER(fq, fp, f_list); } else { LIST_INSERT_HEAD(&filehead, fp, f_list); } p->p_fd->fd_ofiles[i] = fp; + FILEDESC_UNLOCK(p->p_fd); + sx_xunlock(&filelist_lock); if (resultfp) *resultfp = fp; if (resultfd) You could xlock filelist_lock earlier before the first FILEDESC_LOCK with associated changes to avoid as many locking operations. You wouldn't keep the xlock held for much longer and it would probably be quicker in the long run. Bruce is going to not like you for adding nested includes of sys/lock.h and sys/mutex.h. Instead, add nested includes of sys/_lock.h and sys/_mutex.h, and then add sys/lock.h and sys/mutex.h to the files that need them. Other then that it looks great. Can you clean these bits up and post a new patch for folks to test. Aside form svr4, the current patch should be good for testing as well. Esp. need people with SMP machines to test this stuff. -- John Baldwin <jhb@FreeBSD.org> <>< http://www.FreeBSD.org/~jhb/ "Power Users Use the Power to Serve!" - http://www.FreeBSD.org/ To Unsubscribe: send mail to majordomo@FreeBSD.org with "unsubscribe freebsd-smp" in the body of the message
Want to link to this message? Use this URL: <https://mail-archive.FreeBSD.org/cgi/mid.cgi?XFMail.020112185945.jhb>