pipe/fifo code merged.

[Giovanni Trematerra]

On Mon, Jan 9, 2012 at 2:48 PM, John Baldwin <jhb at freebsd.org> wrote:
> On Saturday, January 07, 2012 9:35:47 pm Giovanni Trematerra wrote:
>> Hi,
>> the patch at
>> http://www.trematerra.net/patches/pipefifo_merge2.diff
>>
>> is a preliminary version of the FIFO optimizations project that I picked up from
>> the wiki.
>> http://wiki.freebsd.org/IdeasPage#FIFO_optimizations_.28GSoC.29
>>
>> zhaoshuai@ produced the following patch in the 2009 which attempted a first
>> merge of the interfaces:
>> http://www.trematerra.net/patches/fifo_soc2009.diff
>>
>> However I felt like the work was not yet completed and come up with my final
>> version.
>> Now fifoes derive their structures from pipes one with just special handling
>> to support VFS operations.
>> All the operations but the creation/destruction for fifoes and pipes are handled
>> by the same code.
>> The heart of the patch is the new struct pipeinfo.
>> pipeinfo is a per-file descriptor state. Basically it maintains a read end and
>> a write end for the descriptor. As pipes are bidirectional in FreeBSD, for a
>> pipe this two fields are always equal but different for a fifo.
>> To let fifo code in sys/fs/fifofs/fifo_vnops.c create/destroy the pipe, two
>> functions (pipe_ctor/pipe_dtor) were written. pipe_ctor setups things like a
>> call to kern_pipe and return a pipeinfo structure, while pipe_dtor releases
>> all the resources for a given pipeinfo. Once a pipe was setup during a fifo_open
>> call, all the subsequent operations on the fifo are handled by the same code
>> of a pipe expect for the clean up code that calls pipe_dtor.
>> Allocation of two pipeinfo structures for a pipe were showed to slow down
>> things by some micro-benchmarking. To speed up things during
>> creation/destruction of pipes, the patch allocates all the needed data structure
>> zone using the umapipe struct that packing together all the needed data
>> structures to be allocated at pipe creation. A similar umafifo structure is
>> used for fifoes.
>> Thanks to jilles that made a review of the patch in a previous form, privately.
>> Thanks a lot to attilio that answered my stupid questions and drove me in the
>> right direction.
>
> Thanks for taking this on.  In general I think this looks good, but had a few
> comments:
>
> - Why did you move setting the timestamps of pipes from the UMA ctor to an
>  init routine?  This seems wrong.  The init routine is only invoked when
>  memory is first allocated to a slab to create a set of umapipe or umafifo
>  structures.  However, that umapipe/fifo may be reused multiple times, all
>  with the same timestamp.  Setting the timestamp in the ctor routine means
>  it is set each time a pipepair or fifo is created which seems more
>  appropiate.  Similarly with the inode value.

Ops, it was by accident. Thanks for pointed me out.

> - I would maybe call pipe_ctor(), fifo_ctor() instead or otherwise adjust
>  the name to note it is only used to create a FIFO, not used to create
>  a normal pipe pair (which it's name implies).
> - s/socket/pipe/ in the FIONREAD comment in sys_pipe.c.

ok.

> - Two extra blank lines in the patch that I think should be reverted:
>
> --- a/sys/fs/fifofs/fifo_vnops.c        Sun Jan 01 19:00:13 2012 +0100
> +++ b/sys/fs/fifofs/fifo_vnops.c        Mon Jan 09 00:13:55 2012 +0100
>        int             fi_wgen;
>  };
>
> +
>  static vop_print_t     fifo_print;
>  static vop_open_t      fifo_open;
> ....
> @@ -1598,13 +1839,20 @@ pipe_kqfilter(struct file *fp, struct kn
>                        return (EPIPE);
>                }
>                cpipe = cpipe->pipe_peer;
> +
>                break;
>        default:
>                PIPE_UNLOCK(cpipe);
>                return (EINVAL);

will do.

Thank you for your review.

--
Gianni