| Message ID | 1363881940-27505-8-git-send-email-owasserm@redhat.com |
|---|---|
| State | New |
| Headers | show |
Orit Wasserman <owasserm@redhat.com> wrote: > This allow us to add a buffer to the iovec to send without copying it > into the static buffer. > > Signed-off-by: Orit Wasserman <owasserm@redhat.com> > --- > include/migration/qemu-file.h | 5 +++++ > savevm.c | 37 ++++++++++++++++++++++++++++--------- > 2 files changed, 33 insertions(+), 9 deletions(-) > > diff --git a/include/migration/qemu-file.h b/include/migration/qemu-file.h > index 8d3da9b..5168be2 100644 > --- a/include/migration/qemu-file.h > +++ b/include/migration/qemu-file.h > @@ -75,6 +75,11 @@ int qemu_fclose(QEMUFile *f); > int64_t qemu_ftell(QEMUFile *f); > void qemu_put_buffer(QEMUFile *f, const uint8_t *buf, int size); > void qemu_put_byte(QEMUFile *f, int v); > +/* > + * put_buffer without copying the buffer. > + * The buffer should be available till it is sent. > + */ > +void qemu_put_buffer_no_copy(QEMUFile *f, const uint8_t *buf, int size); > > static inline void qemu_put_ubyte(QEMUFile *f, unsigned int v) > { > diff --git a/savevm.c b/savevm.c > index 83aa9e7..fbfb9e3 100644 > --- a/savevm.c > +++ b/savevm.c > @@ -621,6 +621,30 @@ int qemu_fclose(QEMUFile *f) > return ret; > } > > + > +void qemu_put_buffer_no_copy(QEMUFile *f, const uint8_t *buf, int size) > +{ > + if (f->last_error) { > + return; > + } > + > + if (f->is_write == 0 && f->buf_index > 0) { > + fprintf(stderr, > + "Attempted to write to buffer while read buffer is not empty\n"); > + abort(); > + } I don't understand this test at all (yes, I know that the test already existed). We shouldn't never arrived qemu_put_buffer() with a QEMUFile with f->is_write == 0. Change it for one assert()? > + f->iov[f->iovcnt].iov_base = f->buf + f->buf_index; > + f->iov[f->iovcnt++].iov_len = size; This is clearly wrong, or I have completely missunderstood it (I will give a 50% to each posiblitiy). Here we should be using "buf" and "size" passed as paramenters, f->buf and f->buf_index shouldn't be used, no? > + > + f->is_write = 1; is_write is completely redundant, and should just be a: f->ops->put_buffer test (or now with writev). We only set it up when there is anything to be written? But again, this is independent of this series.
On 03/21/2013 07:34 PM, Juan Quintela wrote: > Orit Wasserman <owasserm@redhat.com> wrote: >> This allow us to add a buffer to the iovec to send without copying it >> into the static buffer. >> >> Signed-off-by: Orit Wasserman <owasserm@redhat.com> >> --- >> include/migration/qemu-file.h | 5 +++++ >> savevm.c | 37 ++++++++++++++++++++++++++++--------- >> 2 files changed, 33 insertions(+), 9 deletions(-) >> >> diff --git a/include/migration/qemu-file.h b/include/migration/qemu-file.h >> index 8d3da9b..5168be2 100644 >> --- a/include/migration/qemu-file.h >> +++ b/include/migration/qemu-file.h >> @@ -75,6 +75,11 @@ int qemu_fclose(QEMUFile *f); >> int64_t qemu_ftell(QEMUFile *f); >> void qemu_put_buffer(QEMUFile *f, const uint8_t *buf, int size); >> void qemu_put_byte(QEMUFile *f, int v); >> +/* >> + * put_buffer without copying the buffer. >> + * The buffer should be available till it is sent. >> + */ >> +void qemu_put_buffer_no_copy(QEMUFile *f, const uint8_t *buf, int size); >> >> static inline void qemu_put_ubyte(QEMUFile *f, unsigned int v) >> { >> diff --git a/savevm.c b/savevm.c >> index 83aa9e7..fbfb9e3 100644 >> --- a/savevm.c >> +++ b/savevm.c >> @@ -621,6 +621,30 @@ int qemu_fclose(QEMUFile *f) >> return ret; >> } >> >> + >> +void qemu_put_buffer_no_copy(QEMUFile *f, const uint8_t *buf, int size) >> +{ >> + if (f->last_error) { >> + return; >> + } >> + >> + if (f->is_write == 0 && f->buf_index > 0) { >> + fprintf(stderr, >> + "Attempted to write to buffer while read buffer is not empty\n"); >> + abort(); >> + } > > I don't understand this test at all (yes, I know that the test already > existed). > > We shouldn't never arrived qemu_put_buffer() with a QEMUFile with > f->is_write == 0. > > Change it for one assert()? > >> + f->iov[f->iovcnt].iov_base = f->buf + f->buf_index; >> + f->iov[f->iovcnt++].iov_len = size; > > This is clearly wrong, or I have completely missunderstood it (I will > give a 50% to each posiblitiy). > > Here we should be using "buf" and "size" passed as paramenters, f->buf > and f->buf_index shouldn't be used, no? you are right, it somehow moved to the next patch :( I will send a fixed v4 after you finish review the series .. > >> + >> + f->is_write = 1; > > is_write is completely redundant, and should just be a: > > f->ops->put_buffer test (or now with writev). We only set it up when > there is anything to be written? > > But again, this is independent of this series. > I agree, maybe we need a cleanup series and remove it. It will certainly simplify this code Orit
diff --git a/include/migration/qemu-file.h b/include/migration/qemu-file.h index 8d3da9b..5168be2 100644 --- a/include/migration/qemu-file.h +++ b/include/migration/qemu-file.h @@ -75,6 +75,11 @@ int qemu_fclose(QEMUFile *f); int64_t qemu_ftell(QEMUFile *f); void qemu_put_buffer(QEMUFile *f, const uint8_t *buf, int size); void qemu_put_byte(QEMUFile *f, int v); +/* + * put_buffer without copying the buffer. + * The buffer should be available till it is sent. + */ +void qemu_put_buffer_no_copy(QEMUFile *f, const uint8_t *buf, int size); static inline void qemu_put_ubyte(QEMUFile *f, unsigned int v) { diff --git a/savevm.c b/savevm.c index 83aa9e7..fbfb9e3 100644 --- a/savevm.c +++ b/savevm.c @@ -621,6 +621,30 @@ int qemu_fclose(QEMUFile *f) return ret; } + +void qemu_put_buffer_no_copy(QEMUFile *f, const uint8_t *buf, int size) +{ + if (f->last_error) { + return; + } + + if (f->is_write == 0 && f->buf_index > 0) { + fprintf(stderr, + "Attempted to write to buffer while read buffer is not empty\n"); + abort(); + } + + f->iov[f->iovcnt].iov_base = f->buf + f->buf_index; + f->iov[f->iovcnt++].iov_len = size; + + f->is_write = 1; + f->bytes_xfer += size; + + if (f->buf_index >= IO_BUF_SIZE || f->iovcnt >= MAX_IOV_SIZE) { + qemu_fflush(f); + } +} + void qemu_put_buffer(QEMUFile *f, const uint8_t *buf, int size) { int l; @@ -640,19 +664,14 @@ void qemu_put_buffer(QEMUFile *f, const uint8_t *buf, int size) if (l > size) l = size; memcpy(f->buf + f->buf_index, buf, l); - f->iov[f->iovcnt].iov_base = f->buf + f->buf_index; - f->iov[f->iovcnt++].iov_len = l; f->is_write = 1; f->buf_index += l; - f->bytes_xfer += l; + qemu_put_buffer_no_copy(f, f->buf + (f->buf_index - l), l); + if (qemu_file_get_error(f)) { + break; + } buf += l; size -= l; - if (f->buf_index >= IO_BUF_SIZE || f->iovcnt >= MAX_IOV_SIZE) { - qemu_fflush(f); - if (qemu_file_get_error(f)) { - break; - } - } } }
This allow us to add a buffer to the iovec to send without copying it into the static buffer. Signed-off-by: Orit Wasserman <owasserm@redhat.com> --- include/migration/qemu-file.h | 5 +++++ savevm.c | 37 ++++++++++++++++++++++++++++--------- 2 files changed, 33 insertions(+), 9 deletions(-)