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

[Yoshiaki Tamura <tamura.yoshiaki@lab.ntt.co.jp>]

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<tamura.yoshiaki@lab.ntt.co.jp>
>
> 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;
>
>
>
>