From owner-freebsd-smp Sun Jan 13 9:36: 0 2002 Delivered-To: freebsd-smp@freebsd.org Received: from mail6.speakeasy.net (mail6.speakeasy.net [216.254.0.206]) by hub.freebsd.org (Postfix) with ESMTP id 9A4D037B41E for ; Sun, 13 Jan 2002 09:35:52 -0800 (PST) Received: (qmail 10191 invoked from network); 13 Jan 2002 17:35:51 -0000 Received: from unknown (HELO laptop.baldwin.cx) ([64.81.54.73]) (envelope-sender ) by mail6.speakeasy.net (qmail-ldap-1.03) with SMTP for ; 13 Jan 2002 17:35:51 -0000 Message-ID: X-Mailer: XFMail 1.4.0 on FreeBSD X-Priority: 3 (Normal) Content-Type: text/plain; charset=us-ascii Content-Transfer-Encoding: 8bit MIME-Version: 1.0 In-Reply-To: <20020112191645.J7984@elvis.mu.org> Date: Sun, 13 Jan 2002 09:35:11 -0800 (PST) From: John Baldwin To: Alfred Perlstein Subject: Re: fd locking. Cc: tanimura@freebsd.org, dillon@freebsd.org, smp@freebsd.org Sender: owner-freebsd-smp@FreeBSD.ORG Precedence: bulk List-ID: List-Archive: (Web Archive) List-Help: (List Instructions) List-Subscribe: List-Unsubscribe: X-Loop: FreeBSD.org On 13-Jan-02 Alfred Perlstein wrote: >> 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. > > Can this be delayed? They are system calls, so they are really getting a thread not a proc (well, the functions that call them are syscalls). These are KSE conflicts. If you've fixed them already that is good, but if not it's pretty quick to do. :) >> 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. :) > > Can this be delayed? It's not required, it's just cleaner, esp. if we switch to a new slab allocator since these functions will almost fit in perfectly to such a beast. >> 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. > > Yes, but those codes call malloc with M_WAITOK, if someone was to close > a filedescriptor i may get deadlock because they block on the filehead > sx lock while i'm blocked in malloc and i already have the filedesc lock. No, not that early, you get the FILEDESC_LOCK a few lines earlier, and you could lock the xlock right before that, but after the malloc. >> 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. > > Can this be delayed? Well, now you won't know what headers to remove unless you start going around with phk's script. Much easier to get this right the first time. >> 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. > > New patch up: > > http://people.freebsd.org/~alfred/fd.diff I see you've committed it already, so I'll just test the code in the tree now. You really need to get it tested by SMP prior to commit since a UP box just isn't going to see the same races, and now if you have a bug that hoses current, we all get to live with it until it's fixed. :-P > I still need to fix Matt's fd holding stuff, but I'm anxious to get this > in before I loose it all to some massive structure renaming or whitespace > run like I have before. :) Start using p4 since it handles this for you better. It's not hard or anything. :-P -- John Baldwin <>< 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