On 07/02/18(Wed) 16:11, Ted Unangst wrote:
> Martin Pieuchot wrote:
> > Diff below shuffles sys_socket() to look like sys_socketpair().
> >
> > The goal is to do socket operations first in both functions. Since
> > they don't need the KERNEL_LOCK(), we will be able to mark the syscalls
> > NOLOCK and only grab it before messing with file descriptors.
>
> this looks ok, but a few comments about future directions.
>
> > + fdplock(fdp);
> > + error = falloc(p, cloexec, &fp, &fd);
> > + fdpunlock(fdp);
>
> > + fp->f_flag = fflag;
> > + fp->f_type = DTYPE_SOCKET;
> > + fp->f_ops = &socketops;
> > if (type & SOCK_NONBLOCK)
> > so->so_state |= SS_NBIO;
> > so->so_state |= ss;
> fp->f_data = so;
> FILE_SET_MATURE(fp, p);
>
> i'm a little concerned about races here. i think we are missing some barriers
> to allow the larval flag on files to properly work in a concurrent system.
> there's nothing to prevent the compiler from hoisting the flag setting above
> the other operations. then another thread in sys_read is going to see bad
> stuff.
>
> maybe we want something like this? (maybe i'm getting ahead of myself?)
You're right but there's much more than that. So in a sense you're
getting a bit ahead of yourself, yes ;) Larval flag has been designed
with sleep in mind. Now if we want to go parallel there there's much
more to do. I'm quite sure we'll trade the KERNEL_LOCK() for another
lock first. So such barrier is premature and would be misleading :)
> Index: file.h
> ===================================================================
> RCS file: /cvs/src/sys/sys/file.h,v
> retrieving revision 1.39
> diff -u -p -r1.39 file.h
> --- file.h 2 Jan 2018 06:40:55 -0000 1.39
> +++ file.h 7 Feb 2018 21:10:57 -0000
> @@ -93,6 +93,7 @@ struct file {
> #define FRELE(fp,p) (--(fp)->f_count == 0 ? fdrop(fp, p) : 0)
>
> #define FILE_SET_MATURE(fp,p) do { \
> + membar_producer(); \
> (fp)->f_iflags &= ~FIF_LARVAL; \
> FRELE(fp, p); \
> } while (0)
>
