Date: Tue, 10 Jan 2012 20:41:15 +1100 (EST) From: Bruce Evans <brde@optusnet.com.au> To: Giovanni Trematerra <gianni@freebsd.org> Cc: flo@freebsd.org, Attilio Rao <attilio@freebsd.org>, Konstantin Belousov <kib@freebsd.org>, freebsd-arch@freebsd.org, jilles@freebsd.org Subject: Re: pipe/fifo code merged. Message-ID: <20120110153807.H943@besplex.bde.org> In-Reply-To: <CACfq09225iMYLe6p8jSiVhsDw_rqTyEHsvPdtZXLrQYT0-skzg@mail.gmail.com> References: <CACfq093o9iVZKxCj58OR2hpCLDYTUTdxg_re_bEMYn2SrNrLCQ@mail.gmail.com> <20120110005155.S2378@besplex.bde.org> <CACfq09225iMYLe6p8jSiVhsDw_rqTyEHsvPdtZXLrQYT0-skzg@mail.gmail.com>
next in thread | previous in thread | raw e-mail | index | archive | help
This message is in MIME format. The first part should be readable text,
while the remaining parts are likely unreadable without MIME-aware tools.
--0-1060448873-1326188475=:943
Content-Type: TEXT/PLAIN; charset=X-UNKNOWN; format=flowed
Content-Transfer-Encoding: QUOTED-PRINTABLE
On Mon, 9 Jan 2012, Giovanni Trematerra wrote:
> On Mon, Jan 9, 2012 at 3:34 PM, Bruce Evans <brde@optusnet.com.au> wrote:
>>
>> I would go the other way, and pessimize pipes to be like fifos. =A0Then
>> optimize the socket layer under both. =A0Fifos are not important, but
>> they are implemented on top of the socket layer which is important.
>> Pipes are important. ...
>> ...
>> Linux-2.6.10 implements fifos as a small wrapper around pipes, while
>> FreeBSD implements them as a large wrapper around sockets. =A0I hope the
>> former is what you do -- share most pipe code, without making it more
>> complicated, and with making the fifo wrapper much simpler. =A0The Linux
>> code is much simpler and smaller, since for pipes it it doesn't
>> implement direct mode, and for sockets it doesn't have to interact with
>> the complicated socket layer.
>
> If you read the patch, as I think you didn't, you'd see that there's no w=
rapper
> at all. fifo's code is just fifo_open, fifo_close and another couple of h=
elper
> functions to deal with VFS, all the remaining code is shared with pipes a=
nd
> no complicated code was added.
I think you don't want me to read the patch, since I would see too much
detail starting with style bugs. Anyway..
% diff -r fee0771aad22 sys/fs/fifofs/fifo.h
% --- a/sys/fs/fifofs/fifo.h=09Sun Jan 01 19:00:13 2012 +0100
% +++ b/sys/fs/fifofs/fifo.h=09Mon Jan 09 00:13:55 2012 +0100
% @@ -35,4 +35,5 @@
% */
% int=09fifo_vnoperate(struct vop_generic_args *);
% int=09fifo_printinfo(struct vnode *);
% +int=09fifo_iseof(struct file *);
This further unsorts an unsorted list by adding to the end of it.
% diff -r fee0771aad22 sys/fs/fifofs/fifo_vnops.c
% --- a/sys/fs/fifofs/fifo_vnops.c=09Sun Jan 01 19:00:13 2012 +0100
% +++ b/sys/fs/fifofs/fifo_vnops.c=09Mon Jan 09 00:13:55 2012 +0100
%=20
% @@ -54,74 +54,28 @@
% ...
% struct fifoinfo {
% -=09struct socket=09*fi_readsock;
% -=09struct socket=09*fi_writesock;
% -=09long=09=09fi_readers;
% -=09long=09=09fi_writers;
% +=09struct pipeinfo *fi_pipeinfo;
% +=09long=09fi_readers;
% +=09long=09fi_writers;
Indentation lost.
% =09int=09=09fi_wgen;
% };
%=20
% +
Extra blank line.
% static vop_print_t=09fifo_print;
% ...
% @@ -186,47 +139,34 @@ fifo_open(ap)
% ...
% -=09=09SOCKBUF_LOCK(&rso->so_rcv);
% -=09=09rso->so_rcv.sb_state |=3D SBS_CANTRCVMORE;
% -=09=09SOCKBUF_UNLOCK(&rso->so_rcv);
% -=09=09KASSERT(vp->v_fifoinfo =3D=3D NULL,
% -=09=09 ("fifo_open: v_fifoinfo race"));
% +
% + =09=09KASSERT(vp->v_fifoinfo =3D=3D NULL, ("fifo_open: v_fifoinfo race"=
));
% +
Extra blank line.
% =09=09vp->v_fifoinfo =3D fip;
% =09}
% +=09pip =3D fip->fi_pipeinfo;
% +
Extra blank line.
% + =09KASSERT(pip !=3D NULL, ("fifo_open: pipeinfo is NULL"));
% +
Extra blank line.
No comment on 100 or so further errors of this type.
Extra blank lines near KASSERT()s are a common style bug, but this bug
was missing in the old code here.
% +=09rpipe =3D pip->pi_rpipe;
% +=09wpipe =3D pip->pi_wpipe;
%=20
% =09/*
% =09 * Use the fifo_mtx lock here, in addition to the vnode lock,
% ...
% -static int
% -fifo_poll_f(struct file *fp, int events, struct ucred *cred, struct thre=
ad *td)
% -{
% -=09struct fifoinfo *fip;
% -=09struct file filetmp;
% -=09int levents, revents =3D 0;
% -
% -=09fip =3D fp->f_data;
% -=09levents =3D events &
% -=09 (POLLIN | POLLINIGNEOF | POLLPRI | POLLRDNORM | POLLRDBAND);
% -=09if ((fp->f_flag & FREAD) && levents) {
% -=09=09filetmp.f_data =3D fip->fi_readsock;
% -=09=09filetmp.f_cred =3D cred;
% -=09=09mtx_lock(&fifo_mtx);
% -=09=09if (fp->f_seqcount =3D=3D fip->fi_wgen)
% -=09=09=09levents |=3D POLLINIGNEOF;
% -=09=09mtx_unlock(&fifo_mtx);
% -=09=09revents |=3D soo_poll(&filetmp, levents, cred, td);
% -=09}
% -=09levents =3D events & (POLLOUT | POLLWRNORM | POLLWRBAND);
% -=09if ((fp->f_flag & FWRITE) && levents) {
% -=09=09filetmp.f_data =3D fip->fi_writesock;
% -=09=09filetmp.f_cred =3D cred;
% -=09=09revents |=3D soo_poll(&filetmp, levents, cred, td);
% -=09}
% -=09return (revents);
% -}
In this file, I have most experience fixing this function (and open
and close so that select and poll work). The above looks simple, but
has a complex interaction with layers above and below it. Most of the
details are in the socket layer. You had to reimplement these in the
pipe layer. The most delicate point involving fs_wgen seems to be
reimplemented correctly in fifo_iseof(). Before I fixed this for
fifos, poll and select on pipes (especially for EOF) was less broken
than for fifos, partly because pipes are simpler -- they can't be
reopened. My tests in /usr/src/tools/regression/poll/ are hopefully
enough to detect any regressions. Some of the tests are intentionally
left broken and/or expected to fail, to be bug for bug compatible with
old kernel bugs.
% ...
% diff -r fee0771aad22 sys/kern/sys_pipe.c
% --- a/sys/kern/sys_pipe.c=09Sun Jan 01 19:00:13 2012 +0100
% +++ b/sys/kern/sys_pipe.c=09Mon Jan 09 00:13:55 2012 +0100
% ...
% @@ -164,6 +184,8 @@ static struct fileops pipeops =3D {
% static void=09filt_pipedetach(struct knote *kn);
% static int=09filt_piperead(struct knote *kn, long hint);
% static int=09filt_pipewrite(struct knote *kn, long hint);
% +static void filt_pipedetach_notsup(struct knote *kn);
% +static int filt_pipenotsup(struct knote *kn, long hint);
This unsorts a sorted list by adding to the end of it.
The old prototypes are in KNF style (tab before the function name)
but the new ones aren't.
% @@ -205,7 +232,7 @@ SYSCTL_INT(_kern_ipc, OID_AUTO, piperesi
% =09 &piperesizeallowed, 0, "Pipe resizing allowed");
%=20
% static void pipeinit(void *dummy __unused);
% -static void pipeclose(struct pipe *cpipe);
% +static void pipeclose(struct pipe *cpipe, int isfifo);
% static void pipe_free_kmem(struct pipe *cpipe);
% static int pipe_create(struct pipe *pipe, int backing);
% static __inline int pipelock(struct pipe *cpipe, int catch);
% @@ -223,8 +250,12 @@ static int pipespace_new(struct pipe *cp
% static int=09pipe_zone_ctor(void *mem, int size, void *arg, int flags);
% static int=09pipe_zone_init(void *mem, int size, int flags);
% static void=09pipe_zone_fini(void *mem, int size);
% +static int=09fifo_zone_ctor(void *mem, int size, void *arg, int flags);
% +static int=09fifo_zone_init(void *mem, int size, int flags);
% +static void=09fifo_zone_fini(void *mem, int size);
Further unsortings of an unsorted list (f < p). The ctor/init/fini were
sorted in "logical" (activation), but that becomes unmanageable for long
lists. init/close/free_kmem/create are now in random order.
The indentation style matches the nearby code but not KNF or previous
code.
The style for nameing parameters matches the nearby code and KNF but not
previous code.
% +static int pipe_umacreate(struct thread *td, struct umapipe **p_up, int =
isfifo);
As above, plus line too long.
% @@ -247,12 +282,14 @@ pipeinit(void *dummy __unused)
% static int
% pipe_zone_ctor(void *mem, int size, void *arg, int flags)
% {
% +=09struct umapipe *up;
% =09struct pipepair *pp;
% =09struct pipe *rpipe, *wpipe;
Unsorting.
% @@ -295,42 +328,133 @@ pipe_zone_ctor(void *mem, int size, void
% static int
% pipe_zone_init(void *mem, int size, int flags)
% {
% -=09struct pipepair *pp;
% +=09struct umapipe *up;
% +=09struct pipeinfo *pip;
% +=09struct timespec ctime;
%=20
% -=09KASSERT(size =3D=3D sizeof(*pp), ("pipe_zone_init: wrong size"));
% +=09KASSERT(size =3D=3D sizeof(*up), ("pipe_zone_init: wrong size"));
%=20
% -=09pp =3D (struct pipepair *)mem;
% +=09up =3D (struct umapipe *)mem;
% +=09vfs_timestamp(&ctime);
% +=09pip =3D &up->pip[0];
% +=09pip->pi_atime =3D pip->pi_mtime =3D pip->pi_ctime =3D ctime;
% +=09pip->pi_ino =3D -1;
%=20
% -=09mtx_init(&pp->pp_mtx, "pipe mutex", NULL, MTX_DEF | MTX_RECURSE);
% +=09pip =3D &up->pip[1];
% +=09pip->pi_atime =3D pip->pi_mtime =3D pip->pi_ctime =3D ctime;
% +=09pip->pi_ino =3D -1;
% +
% +=09mtx_init(&up->pp.pp_mtx, "pipe mutex", NULL, MTX_DEF | MTX_RECURSE);
% =09return (0);
Timestamps seem to be broken. jhb pointed out a problem in them, without
much detail (but I forget the exact detail). Here it can't be right to
have timestamps on both sides, since timestamps are a property of the
file at its lowest level, not the file descriptor or even the file at the
open file (fcntl) level. For fifos, timestamps are even more a property
of the file.
% ...
% +static int
% +fifo_zone_init(void *mem, int size, int flags)
% +{
% +=09struct umafifo *up;
% +=09struct pipeinfo *pip;
% +
% +=09KASSERT(size =3D=3D sizeof(*up), ("fifo_zone_init: wrong size"));
% +
% +=09up =3D (struct umafifo *)mem;
% +=09pip =3D &up->pip[0];
% +=09vfs_timestamp(&pip->pi_ctime);
% +=09pip->pi_atime =3D pip->pi_mtime =3D pip->pi_ctime;
For fifos, is wrong to have even 1 timestamp at this level (except
unused ones won't hurt). Fifos and their timstamps persist as disk
files, and the timestamps for these disk files are managed by the
underlying file system. Any timestamps at this level can only give
possibilities for inconsistencies. For example, this level uses
vfs_timestamp(), but the file system level might use a different
timestamp method, either because it is buggy or because it cannot
represent timestamps with the granularity that vfs_timestamp() gives.
(It is a bug that vfs_timestamp() is global.)
% @@ -1219,7 +1403,7 @@ pipe_write(fp, uio, active_cred, flags,=20
% =09}
%=20
% =09if (error =3D=3D 0)
% -=09=09vfs_timestamp(&wpipe->pipe_mtime);
% +=09=09vfs_timestamp(&pip->pi_mtime);
%=20
% =09/*
% =09 * We have something to offer,
It's doing timstamps in the same way for fifos as for pipes. There must
by a problem for stat() too. The old fifo code uses fo_stat() to get
back to the underlying file system which knows all the attributes (not
just timestamps). I can't see anything like that here. The new fifo
code seems to just use pipe_stat() which gives many fake attributes
which are likely to differ from ones in the file system.
% @@ -1492,12 +1726,12 @@ pipe_free_kmem(cpipe)
% * shutdown the pipe
% */
% static void
% -pipeclose(cpipe)
% +pipeclose(cpipe, isfifo)
% =09struct pipe *cpipe;
% +=09int isfifo;
% {
I don't see any reason to have a different zone for fifos. This
complicates some interfaces ...
% @@ -1570,21 +1798,34 @@ pipeclose(cpipe)
% #ifdef MAC
% =09=09mac_pipe_destroy(pp);
% #endif
% -=09=09uma_zfree(pipe_zone, cpipe->pipe_pair);
% +=09=09uma_zfree(isfifo ? fifo_zone : pipe_zone, cpipe->pipe_pair);
The new isfifo parameter is only used here.
I think the separate fifo zone just lets you see the memory usage for
fifos separately. In the old version, this was mixed up with socket
memory usage and thus even harder to separate. But this is cosmetic.
There seem to be no changes for actual control of the memory usage.
This gives some minor breakages:
- there is a limit on pipe kva. This now applies to fifos too (?).
I sometimes find the old limit too small. It might need to be
increased.
- there seems to be no limit on pipe actual memory, except implicit
ones from limiting pipe kva
- there are too many sysctls for controlling and reporting pipe kva
and other things. These give more and/or different detail than the
pipe zone statistics, but they are not duplicated for fifos. Good,
since there are already too many.
- there is a resource limit on socket real memory that used to apply
to fifos too. It might need to be decreased, to correspond to the
move to the pipe kva limit.
% diff -r fee0771aad22 sys/sys/pipe.h
% --- a/sys/sys/pipe.h=09Sun Jan 01 19:00:13 2012 +0100
% +++ b/sys/sys/pipe.h=09Mon Jan 09 00:13:55 2012 +0100
% @@ -28,6 +28,8 @@
% #error "no user-servicable parts inside"
% #endif
%=20
% +#include <sys/selinfo.h>
% +
This namespace pollution was intentionally left out.
% /*
% * Pipe buffer size, keep moderate in value, pipes take kva space.
% */
% @@ -103,16 +105,12 @@ struct pipe {
% =09struct=09pipebuf pipe_buffer;=09/* data storage */
% =09struct=09pipemapping pipe_map;=09/* pipe mapping for direct I/O */
% =09struct=09selinfo pipe_sel;=09/* for compat with select */
<sys/selinfo.h> was a prerequisite for this file.
% -=09struct=09timespec pipe_atime;=09/* time of last access */
% -=09struct=09timespec pipe_mtime;=09/* time of last modify */
% -=09struct=09timespec pipe_ctime;=09/* time of status change */
% =09struct=09sigio *pipe_sigio;=09/* information for async I/O */
% =09struct=09pipe *pipe_peer;=09/* link with other direction */
% =09struct=09pipepair *pipe_pair;=09/* container structure pointer */
% =09u_int=09pipe_state;=09=09/* pipe status info */
% =09int=09pipe_busy;=09=09/* busy flag, mostly to handle rundown sanely *=
/
% =09int=09pipe_present;=09=09/* still present? */
% -=09ino_t=09pipe_ino;=09=09/* fake inode for stat(2) */
Both this and the timestamps should never have been here, since they
are per-"disk"-file but they were per-pipe-endpoint (see below).
% };
%=20
% /*
% @@ -138,5 +136,24 @@ struct pipepair {
% #define PIPE_UNLOCK(pipe)=09mtx_unlock(PIPE_MTX(pipe))
% #define PIPE_LOCK_ASSERT(pipe, type) mtx_assert(PIPE_MTX(pipe), (type))
%=20
% +#define PIPE_CNT(pipe)=09((pipe->pipe_state & PIPE_DIRECTW) ? \
% +=09=09pipe->pipe_map.cnt : pipe->pipe_buffer.cnt)
% +
% +/*
% + * Per-file descriptor structure.
% + */
I was very confused by this comment. It is sort of backwards. This
structure is for the lowest level, which is the pipepair level for
pipes and the disk level for for fifos.
(But we have to expand the disk level, first from dinodes/directory
entries to inodes/directory blocks, then from inodes to vnodes, then
append pipepairs). The open file level is a level or two above that,
and the file descriptor level is further above.
The levels are stacked even more confusingly for pipes. Above the
pipepair level, there is the pipe level, but this is modes sideways
than fully above. Open files are more at the level of pipes than
pipepairs. "pipe" in normal usage can mean either "pipe" or
"pipepair" in this implementation.)
The timestamps and inode were at a wrong level. You made a step towards
fixing this by moving them down, but the comment says that they are now
at the highest level.
% +struct pipeinfo {
% + =09struct=09pipe=09*pi_rpipe;=09/* pipe we read from */
% + =09struct=09pipe=09*pi_wpipe;=09/* pipe we write to */
% +=09struct=09timespec pi_atime;=09/* time of last access */
% +=09struct=09timespec pi_mtime;=09/* time of last modify */
% +=09struct=09timespec pi_ctime;=09/* time of status change */
% +=09ino_t=09pi_ino;=09=09/* fake pipe inode for stat(2) */
Indentation error.
% +};
I was confused by the layering for this struct too. This struct seems
to be needed only to swap rpipe with wpipe for the 2 ends of a pipe.
This is confusing, but I can't see any better way at the moment.
Putting the other fields in it just gives confusion which leads to
bugs and minor resource wastage. All the other fields must be per-file
at the lowest level, so any duplication of them gives either bugs
(if they are different) or just wastes resources time to write them
and check that they are the same, and and space to hold copies).
These fields belong in the pipepair struct. This moves them down
(more sideways) another level.
POSIX is fuzzy about whether the attributes are unique for the 2 ends
of a pipe. It requires all st_ timestamps and some other attributes
to be "meaningful" unless otherwise specified and doesn't specify
anything otherwise for pipes, at least in an old draft. But for pipe()
it says:
27884 Upon successful completion, pipe( ) shall mark for update =
the st_atime, st_ctime, and st_mtime
27885 fields of the pipe.
Assuming that this part is not fuzzy, "the ... st_atime... fields of
the pipe" in it must refer to unique fields.
Similarly for pi_ino.
These fields shouldn't be used at all for fifos, as mentioned above.
File systems still maintain separate non-copies which pipe_stat()
hides. This doesn't matter much for timestamps and st_ino. It
matters for modes and permissions, etc.
After moving timestamps and pi_ino into the pipepair struct, the new
info struct is reduced to 2 pointers into the pipepair struct.
Unfortunately, the pointer to the pipeinfo struct cannot be simply
replaced by a pointer to the pipepair struct (and dereferencing the
latter instead of the former) because of complications for the
separate ends of a pipe:
@ @@ -372,17 +554,17 @@ kern_pipe(struct thread *td, int fildes[
@ =09 * to avoid races against processes which manage to dup() the read
@ =09 * side while we are blocked trying to allocate the write side.
@ =09 */
@ -=09finit(rf, FREAD | FWRITE, DTYPE_PIPE, rpipe, &pipeops);
@ +=09finit(rf, FREAD | FWRITE, DTYPE_PIPE, &up->pip[0], &pipeops);
@ =09error =3D falloc(td, &wf, &fd, 0);
@ =09if (error) {
@ =09=09fdclose(fdp, rf, fildes[0], td);
@ =09=09fdrop(rf, td);
@ =09=09/* rpipe has been closed by fdrop(). */
@ -=09=09pipeclose(wpipe);
@ +=09=09pipeclose(wpipe, 0);
@ =09=09return (error);
@ =09}
@ =09/* An extra reference on `wf' has been held for us by falloc(). */
@ -=09finit(wf, FREAD | FWRITE, DTYPE_PIPE, wpipe, &pipeops);
@ +=09finit(wf, FREAD | FWRITE, DTYPE_PIPE, &up->pip[1], &pipeops);
One end used to get rpipe and the other end wpipe. This is used mainly
by read/write to go in the correct direction. Now, the 2 ends get
pointers to different pipeinfo structs, with the only differences in
the pipeinfo structs being:
- swap rpipe and wpipe. This is used mainly by read/write as before
- different timestamps (to implement bugs and resource wastage)
- pi_ino should be the same in both (just waste space).
For fifos, rpipe =3D=3D wpipe so the 2 pipeinfo structs should be the same
and the complications are not needed.
I always get confused about the plumbing for the 4 directions (2
directions for 2 pipe fd's with bidirectional pipes), but it seems
that we have lots of complexity to support bidirectional pipes
which "no one" uses.
There are some more complications resource wastages related to having
2 pipe ends when only 1 is needed:
- struct pipe is in struct pipepair twice. This can't quite be fixed
by moving it to struct pipeinfo. If both pipe structs for malloc()ed,
then the 2 pointers in struct pipeinfo would be enough for accessing
them, but I think the space wastage is too small to fix like this.
- some code was symmetrical relative to rpipe and wpipe and it didn't
care in which order they were in. Now rpipe can be equal to wpipe,
some of this code is no longer symmetrical because it does things
like free(rpipe), while the rest of it is still symmetrical because
it does things like initalizing rpipe->foo to the same value as
wpipe->foo. You had to make some changes in this area. Example
of a change in this area:
% @@@ -349,18 +473,76 @@ kern_pipe(struct thread *td, int fildes[
% @ =09/* Only the forward direction pipe is backed by default */
% @ =09if ((error =3D pipe_create(rpipe, 1)) !=3D 0 ||
% @ =09 (error =3D pipe_create(wpipe, 0)) !=3D 0) {
% @-=09=09pipeclose(rpipe);
% @-=09=09pipeclose(wpipe);
% @+=09=09pipeclose(rpipe, isfifo);
% @+=09=09pipeclose(wpipe, isfifo);
Not really symmetrical. There must be 2 closes if there are 2 open
fd's (1 in each direction). But what if there is only 1 open fd for
a fifo with rpipe =3D wpipe? pipe_dtor() is more obviously correct,
since it only does 1 pipeclose() for fifos. The new isfifo flag is
only used in pipeclose() to select the zone.
% @ =09=09return (error);
% @ =09}
% @=20
% @ =09rpipe->pipe_state |=3D PIPE_DIRECTOK;
% @ =09wpipe->pipe_state |=3D PIPE_DIRECTOK;
% @=20
% @+=09if (isfifo) {
% @+=09=09up->pip[0].pi_rpipe =3D rpipe;
% @+=09=09up->pip[0].pi_wpipe =3D wpipe;
% @+=09} else {
% @+=09=09up->pip[0].pi_rpipe =3D rpipe;
% @+=09=09up->pip[0].pi_wpipe =3D rpipe;
% @+=09=09up->pip[1].pi_rpipe =3D wpipe;
% @+=09=09up->pip[1].pi_wpipe =3D wpipe;
% @+=09}
This seems slightly broken. Fifos should only use 1 pipeinfo struct,
but other fifo code initializes both up->pip[0] and up->pip[1].
Having only 1 pipe end initialized would destroy any remaining symmetry
but would make it clear which 1 is actually used.
% +
% +extern struct fileops pipeops;
% +
% +int pipe_ctor(struct pipeinfo **ppip, struct thread *td);
% +void pipe_dtor(struct pipeinfo *pip);
Indentation errors.
%=20
% #endif /* !_SYS_PIPE_H_ */
Bruce
--0-1060448873-1326188475=:943--
Want to link to this message? Use this URL: <https://mail-archive.FreeBSD.org/cgi/mid.cgi?20120110153807.H943>