| Message ID | 1363872230-15081-6-git-send-email-owasserm@redhat.com |
|---|---|
| State | New |
| Headers | show |
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 <owasserm@redhat.com> > --- > 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. 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. 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); > } >
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 <owasserm@redhat.com> >> --- >> 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); >> } >> >
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); + } + } 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); + } + } 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); }
Update qemu_fflush and stdio_close to use writev ops if they are available Signed-off-by: Orit Wasserman <owasserm@redhat.com> --- savevm.c | 30 ++++++++++++++++++++---------- 1 file changed, 20 insertions(+), 10 deletions(-)