[Qemu-devel] [PATCH RFC] qemu-file: output data directly if possible

[Qemu-devel] [PATCH RFC] qemu-file: output data directly if possible

[Michael S. Tsirkin]

qemu file currently always buffers up data before writing it out.
At least for memory this is probably not a good idea:
writing out to file would be cheaper. Let's do
that if we can, which should be the common case. If we can't, buffer.

Signed-off-by: Michael S. Tsirkin <mst@redhat.com>

---

Completely untested, this is just thinking aloud.
Shouldn't the below save us a data copy in the
common case, helping speed up migration?

Please comment.

diff --git a/qemu-file.c b/qemu-file.c
index 761f2a9..6d30151 100644
--- a/qemu-file.c
+++ b/qemu-file.c
@@ -142,6 +142,16 @@ void qemu_put_buffer(QEMUFile *f, const uint8_t *buf, int size)
         abort();
     }
 
+    if (!f->has_error && size > 0 && !f->buf_index) {
+        int len = f->put_buffer(f->opaque, buf, 0, size);
+        if (len >= 0) {
+            size -= len;
+            buf += len;
+        } else {
+            f->has_error = 1;
+        }
+    }
+
     while (!f->has_error && size > 0) {
         l = IO_BUF_SIZE - f->buf_index;
         if (l > size)

Re: [Qemu-devel] [PATCH RFC] qemu-file: output data directly if possible

[Paolo Bonzini]

On 10/10/2011 01:56 AM, Michael S. Tsirkin wrote:
> qemu file currently always buffers up data before writing it out.
> At least for memory this is probably not a good idea:
> writing out to file would be cheaper. Let's do
> that if we can, which should be the common case. If we can't, buffer.
>
> Signed-off-by: Michael S. Tsirkin<mst@redhat.com>
>
> ---
>
> Completely untested, this is just thinking aloud.
> Shouldn't the below save us a data copy in the
> common case, helping speed up migration?

The problem here is qemu_put_byte and friends, where the indirection of 
a function call would probably slow things down.  In the common case, 
qemu_put_byte is called a lot and f->buf_index would not be zero.  The 
way to go would probably be to merge QEMUFile and QEMUBufferedFile's two 
buffering layers, which also removes a copy.

Paolo

Re: [Qemu-devel] [PATCH RFC] qemu-file: output data directly if possible

[Paolo Bonzini]

On 10/10/2011 03:16 PM, Michael S. Tsirkin wrote:
> Yes, it does look sane. QEMUFile doesn't seem to ever be used without
> QEMUBufferedFile - is that true?

I think savevm does use it that way.  With migration thread it will be 
much easier, because the migration thread can just block.  I'd hold it 
off until Juan's patches are in and then I can rebase Umesh's patches 
for migration thread.

Paolo

Re: [Qemu-devel] [PATCH RFC] qemu-file: output data directly if possible

[Michael S. Tsirkin]

On Mon, Oct 10, 2011 at 09:42:51AM +0200, Paolo Bonzini wrote:
> On 10/10/2011 01:56 AM, Michael S. Tsirkin wrote:
> >qemu file currently always buffers up data before writing it out.
> >At least for memory this is probably not a good idea:
> >writing out to file would be cheaper. Let's do
> >that if we can, which should be the common case. If we can't, buffer.
> >
> >Signed-off-by: Michael S. Tsirkin<mst@redhat.com>
> >
> >---
> >
> >Completely untested, this is just thinking aloud.
> >Shouldn't the below save us a data copy in the
> >common case, helping speed up migration?
> 
> The problem here is qemu_put_byte and friends, where the indirection
> of a function call would probably slow things down.  In the common
> case, qemu_put_byte is called a lot and f->buf_index would not be
> zero.

True, maybe the right thing to do is use a size cutoff,
avoid a copy if buffer is large enough.
I note the buffer in qemu file is 32K - is that
based on some specific measurements or just
a random large number? Any objections to making
it smaller, like 4K?

>  The way to go would probably be to merge QEMUFile and
> QEMUBufferedFile's two buffering layers, which also removes a copy.
> 
> Paolo

Yes, it does look sane. QEMUFile doesn't seem to ever be used without
QEMUBufferedFile - is that true?

-- 
MST

Re: [Qemu-devel] [PATCH RFC] qemu-file: output data directly if possible

[Juan Quintela]

"Michael S. Tsirkin" <mst@redhat.com> wrote:
> On Mon, Oct 10, 2011 at 09:42:51AM +0200, Paolo Bonzini wrote:
>> On 10/10/2011 01:56 AM, Michael S. Tsirkin wrote:
>> >qemu file currently always buffers up data before writing it out.
>> >At least for memory this is probably not a good idea:
>> >writing out to file would be cheaper. Let's do
>> >that if we can, which should be the common case. If we can't, buffer.
>> >
>> >Signed-off-by: Michael S. Tsirkin<mst@redhat.com>
>> >
>> >---
>> >
>> >Completely untested, this is just thinking aloud.
>> >Shouldn't the below save us a data copy in the
>> >common case, helping speed up migration?
>> 
>> The problem here is qemu_put_byte and friends, where the indirection
>> of a function call would probably slow things down.  In the common
>> case, qemu_put_byte is called a lot and f->buf_index would not be
>> zero.
>
> True, maybe the right thing to do is use a size cutoff,
> avoid a copy if buffer is large enough.
> I note the buffer in qemu file is 32K - is that
> based on some specific measurements or just
> a random large number? Any objections to making
> it smaller, like 4K?

We can't until we move to a thread or similar.  Basically we have to be
able to end a "section write", there is no way to stop in the midle of a
section, we can only split on whole sections.

I think that for this, it should be easier to just make ram_save_live to
bypass QEMUFile layering and just write directly to the fd (header is
really small (between 4bytes and something less that 256 if we have bad
luck).


>>  The way to go would probably be to merge QEMUFile and
>> QEMUBufferedFile's two buffering layers, which also removes a copy.
>> 
>> Paolo
>
> Yes, it does look sane. QEMUFile doesn't seem to ever be used without
> QEMUBufferedFile - is that true?

As Paolo says, savevm uses QEMUFile directly.  I think that the way to
go is to make savevm to also use QEMUBufferedFile, and then do same
trick to avoid the buffering.  It is on my ToDo list, but there are 4-5
things before that.

Later, Juan.