[Qemu-devel] Re: [RFC PATCH 05/20] Introduce put_vector() and get_vector to QEMUFile and qemu_fopen_ops().

[Yoshiaki Tamura]

Anthony Liguori wrote:
> On 04/21/2010 12:57 AM, Yoshiaki Tamura wrote:
> > QEMUFile currently doesn't support writev(). For sending multiple
> > data, such as pages, using writev() should be more efficient.
> >
> > Signed-off-by: Yoshiaki Tamura<address@hidden>
>
> Is there performance data that backs this up? Since QEMUFile uses a
> linear buffer for most operations that's limited to 16k, I suspect you
> wouldn't be able to observe a difference in practice.

I currently don't have data, but I'll prepare it.
There were two things I wanted to avoid.

1. Pages to be copied to QEMUFile buf through qemu_put_buffer.
2. Calling write() everytime even when we want to send multiple pages at once.

I think 2 may be neglectable.
But 1 seems to be problematic if we want make to the latency as small as 
possible, no?

> Regards,
>
> Anthony Liguori
>
> > ---
> > buffered_file.c | 2 +-
> > hw/hw.h | 16 ++++++++++++++++
> > savevm.c | 43 +++++++++++++++++++++++++------------------
> > 3 files changed, 42 insertions(+), 19 deletions(-)
> >
> > diff --git a/buffered_file.c b/buffered_file.c
> > index 54dc6c2..187d1d4 100644
> > --- a/buffered_file.c
> > +++ b/buffered_file.c
> > @@ -256,7 +256,7 @@ QEMUFile *qemu_fopen_ops_buffered(void *opaque,
> > s->wait_for_unfreeze = wait_for_unfreeze;
> > s->close = close;
> >
> > - s->file = qemu_fopen_ops(s, buffered_put_buffer, NULL,
> > + s->file = qemu_fopen_ops(s, buffered_put_buffer, NULL, NULL, NULL,
> > buffered_close, buffered_rate_limit,
> > buffered_set_rate_limit,
> > buffered_get_rate_limit);
> > diff --git a/hw/hw.h b/hw/hw.h
> > index fc9ed29..921cf90 100644
> > --- a/hw/hw.h
> > +++ b/hw/hw.h
> > @@ -23,6 +23,13 @@
> > typedef int (QEMUFilePutBufferFunc)(void *opaque, const uint8_t *buf,
> > int64_t pos, int size);
> >
> > +/* This function writes a chunk of vector to a file at the given
> > position.
> > + * The pos argument can be ignored if the file is only being used for
> > + * streaming.
> > + */
> > +typedef int (QEMUFilePutVectorFunc)(void *opaque, struct iovec *iov,
> > + int64_t pos, int iovcnt);
> > +
> > /* Read a chunk of data from a file at the given position. The pos
> > argument
> > * can be ignored if the file is only be used for streaming. The number of
> > * bytes actually read should be returned.
> > @@ -30,6 +37,13 @@ typedef int (QEMUFilePutBufferFunc)(void *opaque,
> > const uint8_t *buf,
> > typedef int (QEMUFileGetBufferFunc)(void *opaque, uint8_t *buf,
> > int64_t pos, int size);
> >
> > +/* Read a chunk of vector from a file at the given position. The pos
> > argument
> > + * can be ignored if the file is only be used for streaming. The
> > number of
> > + * bytes actually read should be returned.
> > + */
> > +typedef int (QEMUFileGetVectorFunc)(void *opaque, struct iovec *iov,
> > + int64_t pos, int iovcnt);
> > +
> > /* Close a file and return an error code */
> > typedef int (QEMUFileCloseFunc)(void *opaque);
> >
> > @@ -46,7 +60,9 @@ typedef size_t (QEMUFileSetRateLimit)(void *opaque,
> > size_t new_rate);
> > typedef size_t (QEMUFileGetRateLimit)(void *opaque);
> >
> > QEMUFile *qemu_fopen_ops(void *opaque, QEMUFilePutBufferFunc *put_buffer,
> > + QEMUFilePutVectorFunc *put_vector,
> > QEMUFileGetBufferFunc *get_buffer,
> > + QEMUFileGetVectorFunc *get_vector,
> > QEMUFileCloseFunc *close,
> > QEMUFileRateLimit *rate_limit,
> > QEMUFileSetRateLimit *set_rate_limit,
> > diff --git a/savevm.c b/savevm.c
> > index 490ab70..944e788 100644
> > --- a/savevm.c
> > +++ b/savevm.c
> > @@ -162,7 +162,9 @@ void qemu_announce_self(void)
> >
> > struct QEMUFile {
> > QEMUFilePutBufferFunc *put_buffer;
> > + QEMUFilePutVectorFunc *put_vector;
> > QEMUFileGetBufferFunc *get_buffer;
> > + QEMUFileGetVectorFunc *get_vector;
> > QEMUFileCloseFunc *close;
> > QEMUFileRateLimit *rate_limit;
> > QEMUFileSetRateLimit *set_rate_limit;
> > @@ -263,11 +265,11 @@ QEMUFile *qemu_popen(FILE *stdio_file, const
> > char *mode)
> > s->stdio_file = stdio_file;
> >
> > if(mode[0] == 'r') {
> > - s->file = qemu_fopen_ops(s, NULL, stdio_get_buffer, stdio_pclose,
> > - NULL, NULL, NULL);
> > + s->file = qemu_fopen_ops(s, NULL, NULL, stdio_get_buffer,
> > + NULL, stdio_pclose, NULL, NULL, NULL);
> > } else {
> > - s->file = qemu_fopen_ops(s, stdio_put_buffer, NULL, stdio_pclose,
> > - NULL, NULL, NULL);
> > + s->file = qemu_fopen_ops(s, stdio_put_buffer, NULL, NULL, NULL,
> > + stdio_pclose, NULL, NULL, NULL);
> > }
> > return s->file;
> > }
> > @@ -312,11 +314,11 @@ QEMUFile *qemu_fdopen(int fd, const char *mode)
> > goto fail;
> >
> > if(mode[0] == 'r') {
> > - s->file = qemu_fopen_ops(s, NULL, stdio_get_buffer, stdio_fclose,
> > - NULL, NULL, NULL);
> > + s->file = qemu_fopen_ops(s, NULL, NULL, stdio_get_buffer, NULL,
> > + stdio_fclose, NULL, NULL, NULL);
> > } else {
> > - s->file = qemu_fopen_ops(s, stdio_put_buffer, NULL, stdio_fclose,
> > - NULL, NULL, NULL);
> > + s->file = qemu_fopen_ops(s, stdio_put_buffer, NULL, NULL, NULL,
> > + stdio_fclose, NULL, NULL, NULL);
> > }
> > return s->file;
> >
> > @@ -330,8 +332,8 @@ QEMUFile *qemu_fopen_socket(int fd)
> > QEMUFileSocket *s = qemu_mallocz(sizeof(QEMUFileSocket));
> >
> > s->fd = fd;
> > - s->file = qemu_fopen_ops(s, NULL, socket_get_buffer, socket_close,
> > - NULL, NULL, NULL);
> > + s->file = qemu_fopen_ops(s, NULL, NULL, socket_get_buffer, NULL,
> > + socket_close, NULL, NULL, NULL);
> > return s->file;
> > }
> >
> > @@ -368,11 +370,11 @@ QEMUFile *qemu_fopen(const char *filename, const
> > char *mode)
> > goto fail;
> >
> > if(mode[0] == 'w') {
> > - s->file = qemu_fopen_ops(s, file_put_buffer, NULL, stdio_fclose,
> > - NULL, NULL, NULL);
> > + s->file = qemu_fopen_ops(s, file_put_buffer, NULL, NULL, NULL,
> > + stdio_fclose, NULL, NULL, NULL);
> > } else {
> > - s->file = qemu_fopen_ops(s, NULL, file_get_buffer, stdio_fclose,
> > - NULL, NULL, NULL);
> > + s->file = qemu_fopen_ops(s, NULL, NULL, file_get_buffer, NULL,
> > + stdio_fclose, NULL, NULL, NULL);
> > }
> > return s->file;
> > fail:
> > @@ -400,13 +402,16 @@ static int bdrv_fclose(void *opaque)
> > static QEMUFile *qemu_fopen_bdrv(BlockDriverState *bs, int is_writable)
> > {
> > if (is_writable)
> > - return qemu_fopen_ops(bs, block_put_buffer, NULL, bdrv_fclose,
> > - NULL, NULL, NULL);
> > - return qemu_fopen_ops(bs, NULL, block_get_buffer, bdrv_fclose, NULL,
> > NULL, NULL);
> > + return qemu_fopen_ops(bs, block_put_buffer, NULL, NULL, NULL,
> > + bdrv_fclose, NULL, NULL, NULL);
> > + return qemu_fopen_ops(bs, NULL, NULL, block_get_buffer, NULL,
> > bdrv_fclose, NULL, NULL, NULL);
> > }
> >
> > -QEMUFile *qemu_fopen_ops(void *opaque, QEMUFilePutBufferFunc
> > *put_buffer,
> > +QEMUFile *qemu_fopen_ops(void *opaque,
> > + QEMUFilePutBufferFunc *put_buffer,
> > + QEMUFilePutVectorFunc *put_vector,
> > QEMUFileGetBufferFunc *get_buffer,
> > + QEMUFileGetVectorFunc *get_vector,
> > QEMUFileCloseFunc *close,
> > QEMUFileRateLimit *rate_limit,
> > QEMUFileSetRateLimit *set_rate_limit,
> > @@ -418,7 +423,9 @@ QEMUFile *qemu_fopen_ops(void *opaque,
> > QEMUFilePutBufferFunc *put_buffer,
> >
> > f->opaque = opaque;
> > f->put_buffer = put_buffer;
> > + f->put_vector = put_vector;
> > f->get_buffer = get_buffer;
> > + f->get_vector = get_vector;
> > f->close = close;
> > f->rate_limit = rate_limit;
> > f->set_rate_limit = set_rate_limit;