Date: Sun, 13 Jan 2002 09:35:11 -0800 (PST) From: John Baldwin <jhb@FreeBSD.org> To: Alfred Perlstein <bright@mu.org> Cc: tanimura@freebsd.org, dillon@freebsd.org, smp@freebsd.org Subject: Re: fd locking. Message-ID: <XFMail.020113093511.jhb@FreeBSD.org> In-Reply-To: <20020112191645.J7984@elvis.mu.org>
next in thread | previous in thread | raw e-mail | index | archive | help
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 <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.020113093511.jhb>