pipe/fifo code merged.

[John Baldwin]

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.
- 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.
- 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);


-- 
John Baldwin