Re: [PLEASE-TESTME] Zerocopy networking patch, 2.4.0-1

[Christoph Hellwig]

On Wed, Jan 17, 2001 at 05:13:31PM -0800, Linus Torvalds wrote:
> 
> 
> On Thu, 18 Jan 2001, Christoph Hellwig wrote:
> > 
> > /*
> >  * a simple page,offset,legth tuple like Linus wants it
> >  */
> > struct kiobuf2 {
> > 	struct page *   page;   /* The page itself               */
> > 	u_int16_t       offset; /* Offset to start of valid data */
> > 	u_int16_t       length; /* Number of valid bytes of data */
> > };
> 
> Please use "u16". Or "__u16" if you want to export it to user space.

Ok.

> 
> > struct kiovec2 {
> > 	int             nbufs;          /* Kiobufs actually referenced */
> > 	int             array_len;      /* Space in the allocated lists */
> > 	struct kiobuf * bufs;
> 
> Any reason for array_len?

It's usefull for the expand function - but with kiobufs as secondary data
structure it may no more be nessesary.

> Why not just 
> 
> 	int nbufs,
> 	struct kiobuf *bufs;
> 
> 
> Remember: simplicity is a virtue. 
> 
> Simplicity is also what makes it usable for people who do NOT want to have
> huge overhead.
> 
> > 	unsigned int    locked : 1;     /* If set, pages has been locked */
> 
> Remove this. I don't think it's valid to lock the pages. Who wants to use
> this anyway?

E.g. in the block IO pathes the pages have to be locked.
It's also used by free_kiovec to see wether to do unlock_kiovec before.

> 
> > 	/* Always embed enough struct pages for 64k of IO */
> > 	struct kiobuf * buf_array[KIO_STATIC_PAGES];	 
> 
> Kill kill kill kill. 
> 
> If somebody wants to embed a kiovec into their own data structure, THEY
> can decide to add their own buffers etc. A fundamental data structure
> should _never_ make assumptions like this.

Ok.

> 
> > 	/* Private data */
> > 	void *          private;
> > 	
> > 	/* Dynamic state for IO completion: */
> > 	atomic_t        io_count;       /* IOs still in progress */
> 
> What is io_count used for?

In the current buffer_head based IO-scheme it is used to determine wether
all bh request are finished.  It's obsolete once we pass kiobufs to the
low-level drivers.

> 
> > 	int             errno;
> > 
> > 	/* Status of completed IO */
> > 	void (*end_io)	(struct kiovec *); /* Completion callback */
> > 	wait_queue_head_t wait_queue;
> 
> I suspect all of the above ("private", "end_io" etc) should be at a higher
> layer. Not everybody will necessarily need them.
> 
> Remember: if this is to be well designed, we want to have the data
> structures to pass down to low-level drivers etc, that may not want or
> need a lot of high-level stuff. You should not pass down more than the
> driver really needs.
> 
> In the end, the only thing you _know_ a driver will need (assuming that it
> wants these kinds of buffers) is just
> 
> 	int nbufs;
> 	struct biobuf *bufs;
> 
> That's kind of the minimal set. That should be one level of abstraction in
> its own right. 

Ok. Then we need an additional more or less generic object that is used for
passing in a rw_kiovec file operation (and we really want that for many kinds
of IO). I thould mostly be used for communicating to the high-level driver.

/*
 * the name is just plain stupid, but that shouldn't matter
 */
struct vfs_kiovec {
	struct kiovec *	iov;

	/* private data, mostly for the callback */
	void * private;

	/* completion callback */
 	void (*end_io)	(struct vfs_kiovec *);
 	wait_queue_head_t wait_queue;
};

	Christoph

-- 
Whip me.  Beat me.  Make me maintain AIX.