Re: [Qemu-devel] [PATCH v2 5/8] Use writev ops if available

[Orit Wasserman]

On 03/21/2013 03:44 PM, Paolo Bonzini wrote:
> Il 21/03/2013 14:23, Orit Wasserman ha scritto:
>> Update qemu_fflush and stdio_close to use writev ops if they are available
>>
>> Signed-off-by: Orit Wasserman <address@hidden>
>> ---
>>  savevm.c | 30 ++++++++++++++++++++----------
>>  1 file changed, 20 insertions(+), 10 deletions(-)
>>
>> diff --git a/savevm.c b/savevm.c
>> index ab81dd3..fde59d3 100644
>> --- a/savevm.c
>> +++ b/savevm.c
>> @@ -292,7 +292,7 @@ static int stdio_fclose(void *opaque)
>>      QEMUFileStdio *s = opaque;
>>      int ret = 0;
>>  
>> -    if (s->file->ops->put_buffer) {
>> +    if (s->file->ops->put_buffer || s->file->ops->writev_buffer) {
>>          int fd = fileno(s->stdio_file);
>>          struct stat st;
>>  
>> @@ -515,24 +515,34 @@ static void qemu_file_set_error(QEMUFile *f, int ret)
>>      }
>>  }
>>  
>> -/** Flushes QEMUFile buffer
>> +/**
>> + * Flushes QEMUFile buffer
>>   *
>> + * If there is writev_buffer QEMUFileOps it uses it otherwise uses
>> + * put_buffer ops.
>>   */
>>  static void qemu_fflush(QEMUFile *f)
>>  {
>>      int ret = 0;
>>  
>> -    if (!f->ops->put_buffer) {
>> +    if (f->ops->writev_buffer) {
>> +        if (f->is_write && f->iovcnt > 0) {
>> +            ret = f->ops->writev_buffer(f->opaque, f->iov, f->iovcnt);
> 
> This needs to update f->pos.
> 
>> +        }
>> +    } else if (f->ops->put_buffer) {
>> +        if (f->is_write && f->buf_index > 0) {
>> +            ret = f->ops->put_buffer(f->opaque, f->buf, f->pos, 
>> f->buf_index);
>> +        }
> 
> I think this has to go through all the iovecs (i.e. never look at
> f->buf_index, only f->iovcnt).  Otherwise RAM migration does not work,
> because the page data is not copied to f->buf.  But that's pretty much
> it, the series looks good.
You are right I will fix it.
> 
> However, I'd still prefer to have qemu_put_buffer_no_copy coalesce
> adjacent iovecs.  Otherwise you might end up with only 3-400 bytes in
> each sendmsg when serializing device data.
Yes this will be helpful for the device state.

Orit
> 
> Paolo
> 
>> +    } else {
>>          return;
>>      }
>> -    if (f->is_write && f->buf_index > 0) {
>> -        ret = f->ops->put_buffer(f->opaque, f->buf, f->pos, f->buf_index);
>> -        if (ret >= 0) {
>> -            f->pos += f->buf_index;
>> -        }
>> -        f->buf_index = 0;
>> -        f->iovcnt = 0;
>> +
>> +    if (ret >= 0) {
>> +        f->pos += f->buf_index;
>>      }
>> +    f->buf_index = 0;
>> +    f->iovcnt = 0;
>> +
>>      if (ret < 0) {
>>          qemu_file_set_error(f, ret);
>>      }
>>
>