Date: Wed, 8 Feb 2012 16:49:41 +1100 (EST) From: Bruce Evans <brde@optusnet.com.au> To: Giovanni Trematerra <gianni@FreeBSD.org> Cc: flo@FreeBSD.org, Kip Macy <kmacy@FreeBSD.org>, John Baldwin <jhb@FreeBSD.org>, Peter Holm <pho@FreeBSD.org>, Attilio Rao <attilio@FreeBSD.org>, Konstantin Belousov <kib@FreeBSD.org>, bde@FreeBSD.org, freebsd-arch@FreeBSD.org, jilles@FreeBSD.org Subject: Re: [LAST ROUND] fifo/pipe merge code Message-ID: <20120208151251.J1853@besplex.bde.org> In-Reply-To: <CACfq090%2BEDVjj_rRNyafiod_BBzGUzmQ9CjApZLcXWR-Ax=0uQ@mail.gmail.com> References: <CACfq090%2BEDVjj_rRNyafiod_BBzGUzmQ9CjApZLcXWR-Ax=0uQ@mail.gmail.com>
next in thread | previous in thread | raw e-mail | index | archive | help
On Tue, 7 Feb 2012, Giovanni Trematerra wrote: > Hi All, > Here a commit candidate patch of the experimental one posted on Jan 8 > http://www.trematerra.net/patches/pipefifo_merge.4.3.patch > > The patch aims to > - not introduce any performance regressions in pipe code > - speed up performance for fifos > - not introduce any regressions > - fifos and pipes have to have the same behavior as before the patch. > - share as much code as possible between pipes and fifos. > > The patch is greatly improved from its first experimental form. > Now it's more clean and compact and integrates all the good suggestions > from previous reviewers. Here some of the major differences: > - no more different UMA zones to allocate things. > - no more pipeinfo structure. > - fix a bug at fifo_close. > - fix a different behavior for fifos during truncate (thanks to jilles). > > The patch was reviewed in a previous form by jhb@, bde@, jilles@, > attilio@(only style bug) > This version of the patch was reviewed privately by jilles@, > attilio@(only style bug) It looks quite good now. I see only a few minor style bugs: % diff --git a/sys/fs/fifofs/fifo_vnops.c b/sys/fs/fifofs/fifo_vnops.c % index 94a2713..0d35877 100644 % --- a/sys/fs/fifofs/fifo_vnops.c % +++ b/sys/fs/fifofs/fifo_vnops.c % ... % +int % +fifo_iseof(struct file *fp) % ... % + KASSERT(fp->f_vnode != NULL, ("fifo_iseof: no vnode info.")); % + KASSERT(fp->f_vnode->v_fifoinfo != NULL, ("fifo_iseof: no fifoinfo.")); Error messages should not be terminated by a ".". All other KASSERT()s in this file and in sys_pipe.c follow this rule. But 1 old one in sys_pipe.c is terminated by a "!", and some in sys_pipe.c don't follow the rule that error messages should not be capitalized. I wonder if you can assert that some suitable lock is held here. % + fip = fp->f_vnode->v_fifoinfo; % + return (fp->f_seqcount == fip->fi_wgen); % } % ... % diff --git a/sys/kern/sys_pipe.c b/sys/kern/sys_pipe.c % index 9edcb74..5892e9f 100644 % --- a/sys/kern/sys_pipe.c % +++ b/sys/kern/sys_pipe.c % @@ -1,4 +1,5 @@ % /*- % + * Copyright (c) 2012 Giovanni Trematerra % * Copyright (c) 1996 John S. Dyson % * All rights reserved. % * Copyrights should be in historical order, as in fifo_vnops.c. % @@ -135,6 +138,10 @@ __FBSDID("$FreeBSD$"); % */ % /* #define PIPE_NODIRECT */ % % +#define PIPE_PEER(pipe) \ % + (((pipe)->pipe_state & PIPE_NAMED) ? \ % + (pipe) : ((pipe)->pipe_peer)) % + This has strange indentation for the macro body, and weird indentation for the backslashes to not line them up at all. With more normal indentation, there is no need to split the macro body and thus not enough backslashes to need lining up: %%% #define PIPE_PEER(pipe) \ (((pipe)->pipe_state & PIPE_NAMED) ? (pipe) : ((pipe)->pipe_peer)) %%% % ... % @@ -175,6 +184,11 @@ static struct filterops pipe_wfiltops = { % .f_detach = filt_pipedetach, % .f_event = filt_pipewrite % }; % +static struct filterops pipe_nfiltops = { % + .f_isfd = 1, % + .f_detach = filt_pipedetach_notsup, % + .f_event = filt_pipenotsup % +}; This should probably be sorted before the others (order nrw, not rwn). % % /* % * Default pipe buffer size(s), this can be kind-of large now because pipe % @@ -220,6 +234,7 @@ static void pipe_clone_write_buffer(struct pipe *wpipe); % static int pipespace(struct pipe *cpipe, int size); % static int pipespace_new(struct pipe *cpipe, int size); % % +static int pipe_paircreate(struct thread *td, struct pipepair **p_pp); % static int pipe_zone_ctor(void *mem, int size, void *arg, int flags); % static int pipe_zone_init(void *mem, int size, int flags); % static void pipe_zone_fini(void *mem, int size); This seems to be unsorted. I see no reason to sort it with the pipe_zone constriuctors and desstructors, especially if you don't name it pipe_zone_*. The declarations of lots of other constructors are unsorted into the main list. Old style bugs: the pipe_zone functions shouldn't be in a separate section and that section shouldn't have a different indentation style (although that style is closer to KNF), especially since these functions are more systematically named than the others (the zone_ in their prefix keeps them together when they are sorted alphabetically). % @@ -1284,6 +1348,17 @@ pipe_ioctl(fp, cmd, data, active_cred, td) % break; % % case FIONREAD: % + Extra blank line. % + /* % + * FIONREAD will return 0 for non-readable descriptors, and % + * the results of FIONREAD on the read pipe for readable % + * descriptors. % + */ % + if (!(fp->f_flag & FREAD)) { % + *(int *)data = 0; % + PIPE_UNLOCK(mpipe); % + return (0); % + } % if (mpipe->pipe_state & PIPE_DIRECTW) % *(int *)data = mpipe->pipe_map.cnt; % else Urk, now I see many major style bugs: % @@ -1333,47 +1408,55 @@ pipe_poll(fp, events, active_cred, td) % int error; % #endif % % - wpipe = rpipe->pipe_peer; % + wpipe = PIPE_PEER(rpipe); % PIPE_LOCK(rpipe); % #ifdef MAC % error = mac_pipe_check_poll(active_cred, rpipe->pipe_pair); % if (error) % goto locked_error; % #endif % - if (events & (POLLIN | POLLRDNORM)) % - if ((rpipe->pipe_state & PIPE_DIRECTW) || % - (rpipe->pipe_buffer.cnt > 0)) % - revents |= events & (POLLIN | POLLRDNORM); % - % - if (events & (POLLOUT | POLLWRNORM)) % - if (wpipe->pipe_present != PIPE_ACTIVE || % - (wpipe->pipe_state & PIPE_EOF) || % - (((wpipe->pipe_state & PIPE_DIRECTW) == 0) && % - ((wpipe->pipe_buffer.size - wpipe->pipe_buffer.cnt) >= PIPE_BUF || % - wpipe->pipe_buffer.size == 0))) % - revents |= events & (POLLOUT | POLLWRNORM); % - % - if ((events & POLLINIGNEOF) == 0) { % - if (rpipe->pipe_state & PIPE_EOF) { % - revents |= (events & (POLLIN | POLLRDNORM)); % - if (wpipe->pipe_present != PIPE_ACTIVE || % - (wpipe->pipe_state & PIPE_EOF)) % - revents |= POLLHUP; % + if (fp->f_flag & FREAD) { % + if (events & (POLLIN | POLLRDNORM)) % + if ((rpipe->pipe_state & PIPE_DIRECTW) || % + (rpipe->pipe_buffer.cnt > 0)) % + revents |= events & (POLLIN | POLLRDNORM); % + This part of the patch is still unreadable since it indents everything to add FREAD and FWRITE checks. I don't like those anyway (they should probably produce POLLERR POLLNVAL instead of ignoring the error -- see previous mail), but they are needed for now for compatibility in the FIFO case (hopefully they don't break compatibility for the plain pipe case). To avoid churning this code for them now and later when the behaviour is fixed, they should be added in a non-invasive way like the old fifo code did. That involved using && instead of a nested if. In the old code, everything was under the combined && if. Now there are 3 nested ifs. This logic change seems to be non-null and give bugs (1). % + if (rpipe->pipe_state & PIPE_NAMED) % + if (fifo_iseof(fp)) Here the (unrelated) nested if is just a style bug. It gives verboseness and extra indentation. % + events |= POLLINIGNEOF; % + (1) In the old code, POLLINIGNEOF was only set here if we are polling for some input condition (and this is valid). Now it is set if the file is just open for input. Either way could be right, but this is a change. If we are not polling for input, we don't care about it, but POSIX requires POLLHUP to be set in some cases, and this affects whether we return or block, and POLLINIGNEOF interacts with this subtly. There may also be a problem not going through here to set POLLINIGNEOF in the !FREAD case. % + if ((events & POLLINIGNEOF) == 0) { % + if (rpipe->pipe_state & PIPE_EOF) { % + revents |= (events & (POLLIN | POLLRDNORM)); % + if (wpipe->pipe_present != PIPE_ACTIVE || % + (wpipe->pipe_state & PIPE_EOF)) % + revents |= POLLHUP; % + } % } This is the old sys_pipe.c logic, indented under the FREAD condition. It is also move up to be inside the FREAD block. To avoid the re-indentation, repeat the FREAD test in and && condition. After avoiding all re-indentations by repeating FREAD and FWRITE tests, you don't need to re-order either. % } This is not consistently verbose in adding extra blank lines. % + if (fp->f_flag & FWRITE) % + if (events & (POLLOUT | POLLWRNORM)) % + if (wpipe->pipe_present != PIPE_ACTIVE || % + (wpipe->pipe_state & PIPE_EOF) || % + (((wpipe->pipe_state & PIPE_DIRECTW) == 0) && % + ((wpipe->pipe_buffer.size - wpipe->pipe_buffer.cnt) >= % + PIPE_BUF || wpipe->pipe_buffer.size == 0))) % + revents |= events & (POLLOUT | POLLWRNORM); % I don't like excessive braces, but this statement needs unnecessary ones. It should also be written using &&, not using a nested if. Since there is nothing like POLLINIGNEOF to complicate things, only 1 if statement is needed for FWRITE (until we get to slrecording). % if (revents == 0) { % - if (events & (POLLIN | POLLRDNORM)) { % - selrecord(td, &rpipe->pipe_sel); % - if (SEL_WAITING(&rpipe->pipe_sel)) % - rpipe->pipe_state |= PIPE_SEL; % - } % + if (fp->f_flag & FREAD) % + if (events & (POLLIN | POLLRDNORM)) { Use && here. It already has sufficient braces, at least after this change. % + selrecord(td, &rpipe->pipe_sel); % + if (SEL_WAITING(&rpipe->pipe_sel)) % + rpipe->pipe_state |= PIPE_SEL; % + } % % - if (events & (POLLOUT | POLLWRNORM)) { % - selrecord(td, &wpipe->pipe_sel); % - if (SEL_WAITING(&wpipe->pipe_sel)) % - wpipe->pipe_state |= PIPE_SEL; % - } % + if (fp->f_flag & FWRITE) % + if (events & (POLLOUT | POLLWRNORM)) { Use && here, as above. % + selrecord(td, &wpipe->pipe_sel); % + if (SEL_WAITING(&wpipe->pipe_sel)) % + wpipe->pipe_state |= PIPE_SEL; % + } % } % #ifdef MAC % locked_error: % ... % diff --git a/sys/sys/pipe.h b/sys/sys/pipe.h % index 20b1092..479223f 100644 % --- a/sys/sys/pipe.h % +++ b/sys/sys/pipe.h % ... % @@ -138,5 +139,9 @@ struct pipepair { % #define PIPE_UNLOCK(pipe) mtx_unlock(PIPE_MTX(pipe)) % #define PIPE_LOCK_ASSERT(pipe, type) mtx_assert(PIPE_MTX(pipe), (type)) % % +extern struct fileops pipeops; This should be declared in the existing extern section for data at the top of the file. That section has just one declaration. Old bug: that section has a bogus comment. It says /* See sys_pipe.c for info on what these limits mean. */" (actually a block comment to be verbose about this unimportant and misdescribed point), but there are no limits here; there is just 1 variable declaration. There are many more variables and limits, but the others are all in sys_pipe.c. This one (maxpipekva) unfortunately has to be declared here so that subr_param.c can initialize it, but this is an implementation detail unrelated to the meaning of this variable. maxpipekva is also instantiated in a wrong place (in subr_param.c instead of in sys_pipe.c). % + % +int pipe_fifo_ctor(struct pipe **ppipe, struct thread *td); % +void pipe_dtor(struct pipe *dpipe); These are sorted backwards. They aren't really even in logical order (ctor before dtor, which is also alphabetical), because the ctor is for fifos only but the dtor is generic. % % #endif /* !_SYS_PIPE_H_ */ Bruce
Want to link to this message? Use this URL: <https://mail-archive.FreeBSD.org/cgi/mid.cgi?20120208151251.J1853>