| Message ID | 20190731144225.3784-3-richardw.yang@linux.intel.com |
|---|---|
| State | New |
| Headers | show |
| Series | migration/qemu-file: cleanup and refine qemu-file | expand |
(Copying Dan in) * Wei Yang (richardw.yang@linux.intel.com) wrote: > In add_to_iovec(), qemu_fflush() will be called if iovec is full. If > this happens, buf_index is reset. Currently, this is not checked and > buf_index would always been adjust with buf size. > > This is not harmful, but will waste some space in file buffer. That's a nice find. > This patch make add_to_iovec() return 1 when it has flushed the file. > Then the caller could check the return value to see whether it is > necessary to adjust the buf_index any more. > > Signed-off-by: Wei Yang <richardw.yang@linux.intel.com> Reviewed-by: Dr. David Alan Gilbert <dgilbert@redhat.com> (I wonder if there's a way to wrap that little add_to_iovec, check, add to index, flush in a little wrapper). Dave > --- > migration/qemu-file.c | 42 ++++++++++++++++++++++++++++-------------- > 1 file changed, 28 insertions(+), 14 deletions(-) > > diff --git a/migration/qemu-file.c b/migration/qemu-file.c > index 35c22605dd..05d9f42ddb 100644 > --- a/migration/qemu-file.c > +++ b/migration/qemu-file.c > @@ -343,8 +343,16 @@ int qemu_fclose(QEMUFile *f) > return ret; > } > > -static void add_to_iovec(QEMUFile *f, const uint8_t *buf, size_t size, > - bool may_free) > +/* > + * Add buf to iovec. Do flush if iovec is full. > + * > + * Return values: > + * 1 iovec is full and flushed > + * 0 iovec is not flushed > + * > + */ > +static int add_to_iovec(QEMUFile *f, const uint8_t *buf, size_t size, > + bool may_free) > { > /* check for adjacent buffer and coalesce them */ > if (f->iovcnt > 0 && buf == f->iov[f->iovcnt - 1].iov_base + > @@ -362,7 +370,10 @@ static void add_to_iovec(QEMUFile *f, const uint8_t *buf, size_t size, > > if (f->iovcnt >= MAX_IOV_SIZE) { > qemu_fflush(f); > + return 1; > } > + > + return 0; > } > > void qemu_put_buffer_async(QEMUFile *f, const uint8_t *buf, size_t size, > @@ -391,10 +402,11 @@ void qemu_put_buffer(QEMUFile *f, const uint8_t *buf, size_t size) > } > memcpy(f->buf + f->buf_index, buf, l); > f->bytes_xfer += l; > - add_to_iovec(f, f->buf + f->buf_index, l, false); > - f->buf_index += l; > - if (f->buf_index == IO_BUF_SIZE) { > - qemu_fflush(f); > + if (!add_to_iovec(f, f->buf + f->buf_index, l, false)) { > + f->buf_index += l; > + if (f->buf_index == IO_BUF_SIZE) { > + qemu_fflush(f); > + } > } > if (qemu_file_get_error(f)) { > break; > @@ -412,10 +424,11 @@ void qemu_put_byte(QEMUFile *f, int v) > > f->buf[f->buf_index] = v; > f->bytes_xfer++; > - add_to_iovec(f, f->buf + f->buf_index, 1, false); > - f->buf_index++; > - if (f->buf_index == IO_BUF_SIZE) { > - qemu_fflush(f); > + if (!add_to_iovec(f, f->buf + f->buf_index, 1, false)) { > + f->buf_index++; > + if (f->buf_index == IO_BUF_SIZE) { > + qemu_fflush(f); > + } > } > } > > @@ -717,10 +730,11 @@ ssize_t qemu_put_compression_data(QEMUFile *f, z_stream *stream, > } > > qemu_put_be32(f, blen); > - add_to_iovec(f, f->buf + f->buf_index, blen, false); > - f->buf_index += blen; > - if (f->buf_index == IO_BUF_SIZE) { > - qemu_fflush(f); > + if (!add_to_iovec(f, f->buf + f->buf_index, blen, false)) { > + f->buf_index += blen; > + if (f->buf_index == IO_BUF_SIZE) { > + qemu_fflush(f); > + } > } > return blen + sizeof(int32_t); > } > -- > 2.17.1 > -- Dr. David Alan Gilbert / dgilbert@redhat.com / Manchester, UK
On Fri, Aug 23, 2019 at 12:06:09PM +0100, Dr. David Alan Gilbert wrote: > (Copying Dan in) > > * Wei Yang (richardw.yang@linux.intel.com) wrote: > > In add_to_iovec(), qemu_fflush() will be called if iovec is full. If > > this happens, buf_index is reset. Currently, this is not checked and > > buf_index would always been adjust with buf size. > > > > This is not harmful, but will waste some space in file buffer. > > That's a nice find. > > > This patch make add_to_iovec() return 1 when it has flushed the file. > > Then the caller could check the return value to see whether it is > > necessary to adjust the buf_index any more. > > > > Signed-off-by: Wei Yang <richardw.yang@linux.intel.com> > > Reviewed-by: Dr. David Alan Gilbert <dgilbert@redhat.com> > > (I wonder if there's a way to wrap that little add_to_iovec, check, add > to index, flush in a little wrapper). Given the name "add_to_iovec" I think it is pretty surprising that it calls "qemu_flush" at all. It is also pretty wierd that we're checking two different conditions in two different places. Right now the code is essentially doing this: if (f->iovcnt >= MAX_IOV_SIZE) { qemu_fflush(f); } if (f->buf_index == IO_BUF_SIZE) { qemu_fflush(f); } Except that in the qemu_put_buffer_async() case, we're only doing the first of these two checks. This feels very odd indeed - I would have thought either it should do both, or do neither. Assuming doing both flushs is ok for qemu_put_buffer_async then I'd suggest renaming 'add_to_iovec' to 'queue_buffer' and have that method do both of these qemu_fflush() calls. > > --- > > migration/qemu-file.c | 42 ++++++++++++++++++++++++++++-------------- > > 1 file changed, 28 insertions(+), 14 deletions(-) > > > > diff --git a/migration/qemu-file.c b/migration/qemu-file.c > > index 35c22605dd..05d9f42ddb 100644 > > --- a/migration/qemu-file.c > > +++ b/migration/qemu-file.c > > @@ -343,8 +343,16 @@ int qemu_fclose(QEMUFile *f) > > return ret; > > } > > > > -static void add_to_iovec(QEMUFile *f, const uint8_t *buf, size_t size, > > - bool may_free) > > +/* > > + * Add buf to iovec. Do flush if iovec is full. > > + * > > + * Return values: > > + * 1 iovec is full and flushed > > + * 0 iovec is not flushed > > + * > > + */ > > +static int add_to_iovec(QEMUFile *f, const uint8_t *buf, size_t size, > > + bool may_free) > > { > > /* check for adjacent buffer and coalesce them */ > > if (f->iovcnt > 0 && buf == f->iov[f->iovcnt - 1].iov_base + > > @@ -362,7 +370,10 @@ static void add_to_iovec(QEMUFile *f, const uint8_t *buf, size_t size, > > > > if (f->iovcnt >= MAX_IOV_SIZE) { > > qemu_fflush(f); > > + return 1; > > } > > + > > + return 0; > > } > > > > void qemu_put_buffer_async(QEMUFile *f, const uint8_t *buf, size_t size, > > @@ -391,10 +402,11 @@ void qemu_put_buffer(QEMUFile *f, const uint8_t *buf, size_t size) > > } > > memcpy(f->buf + f->buf_index, buf, l); > > f->bytes_xfer += l; > > - add_to_iovec(f, f->buf + f->buf_index, l, false); > > - f->buf_index += l; > > - if (f->buf_index == IO_BUF_SIZE) { > > - qemu_fflush(f); > > + if (!add_to_iovec(f, f->buf + f->buf_index, l, false)) { > > + f->buf_index += l; > > + if (f->buf_index == IO_BUF_SIZE) { > > + qemu_fflush(f); > > + } > > } > > if (qemu_file_get_error(f)) { > > break; > > @@ -412,10 +424,11 @@ void qemu_put_byte(QEMUFile *f, int v) > > > > f->buf[f->buf_index] = v; > > f->bytes_xfer++; > > - add_to_iovec(f, f->buf + f->buf_index, 1, false); > > - f->buf_index++; > > - if (f->buf_index == IO_BUF_SIZE) { > > - qemu_fflush(f); > > + if (!add_to_iovec(f, f->buf + f->buf_index, 1, false)) { > > + f->buf_index++; > > + if (f->buf_index == IO_BUF_SIZE) { > > + qemu_fflush(f); > > + } > > } > > } > > > > @@ -717,10 +730,11 @@ ssize_t qemu_put_compression_data(QEMUFile *f, z_stream *stream, > > } > > > > qemu_put_be32(f, blen); > > - add_to_iovec(f, f->buf + f->buf_index, blen, false); > > - f->buf_index += blen; > > - if (f->buf_index == IO_BUF_SIZE) { > > - qemu_fflush(f); > > + if (!add_to_iovec(f, f->buf + f->buf_index, blen, false)) { > > + f->buf_index += blen; > > + if (f->buf_index == IO_BUF_SIZE) { > > + qemu_fflush(f); > > + } > > } > > return blen + sizeof(int32_t); > > } > > -- > > 2.17.1 > > > -- > Dr. David Alan Gilbert / dgilbert@redhat.com / Manchester, UK > Regards, Daniel
* Daniel P. Berrangé (berrange@redhat.com) wrote: > On Fri, Aug 23, 2019 at 12:06:09PM +0100, Dr. David Alan Gilbert wrote: > > (Copying Dan in) > > > > * Wei Yang (richardw.yang@linux.intel.com) wrote: > > > In add_to_iovec(), qemu_fflush() will be called if iovec is full. If > > > this happens, buf_index is reset. Currently, this is not checked and > > > buf_index would always been adjust with buf size. > > > > > > This is not harmful, but will waste some space in file buffer. > > > > That's a nice find. > > > > > This patch make add_to_iovec() return 1 when it has flushed the file. > > > Then the caller could check the return value to see whether it is > > > necessary to adjust the buf_index any more. > > > > > > Signed-off-by: Wei Yang <richardw.yang@linux.intel.com> > > > > Reviewed-by: Dr. David Alan Gilbert <dgilbert@redhat.com> > > > > (I wonder if there's a way to wrap that little add_to_iovec, check, add > > to index, flush in a little wrapper). > > Given the name "add_to_iovec" I think it is pretty surprising > that it calls "qemu_flush" at all. > > It is also pretty wierd that we're checking two different > conditions in two different places. > > Right now the code is essentially doing this: > > if (f->iovcnt >= MAX_IOV_SIZE) { > qemu_fflush(f); > } > if (f->buf_index == IO_BUF_SIZE) { > qemu_fflush(f); > } > > Except that in the qemu_put_buffer_async() case, we're > only doing the first of these two checks. This feels > very odd indeed - I would have thought either it should > do both, or do neither. No, there's two separate types of buffers. There's f->buf which is a single allocated buffer in the QEMUFile with an offset buf_index, and there are arbitrary RAM pages added typically via qemu_put_buffer_async. The check for >= IO_BUF_SIZE is only done when adding to the f->buf, where as the check on f->iovcnt is done when you add an element to the iovec and that can happen potentially in either case. Dave > Assuming doing both flushs is ok for qemu_put_buffer_async > then I'd suggest renaming 'add_to_iovec' to 'queue_buffer' > and have that method do both of these qemu_fflush() calls. > > > > --- > > > migration/qemu-file.c | 42 ++++++++++++++++++++++++++++-------------- > > > 1 file changed, 28 insertions(+), 14 deletions(-) > > > > > > diff --git a/migration/qemu-file.c b/migration/qemu-file.c > > > index 35c22605dd..05d9f42ddb 100644 > > > --- a/migration/qemu-file.c > > > +++ b/migration/qemu-file.c > > > @@ -343,8 +343,16 @@ int qemu_fclose(QEMUFile *f) > > > return ret; > > > } > > > > > > -static void add_to_iovec(QEMUFile *f, const uint8_t *buf, size_t size, > > > - bool may_free) > > > +/* > > > + * Add buf to iovec. Do flush if iovec is full. > > > + * > > > + * Return values: > > > + * 1 iovec is full and flushed > > > + * 0 iovec is not flushed > > > + * > > > + */ > > > +static int add_to_iovec(QEMUFile *f, const uint8_t *buf, size_t size, > > > + bool may_free) > > > { > > > /* check for adjacent buffer and coalesce them */ > > > if (f->iovcnt > 0 && buf == f->iov[f->iovcnt - 1].iov_base + > > > @@ -362,7 +370,10 @@ static void add_to_iovec(QEMUFile *f, const uint8_t *buf, size_t size, > > > > > > if (f->iovcnt >= MAX_IOV_SIZE) { > > > qemu_fflush(f); > > > + return 1; > > > } > > > + > > > + return 0; > > > } > > > > > > void qemu_put_buffer_async(QEMUFile *f, const uint8_t *buf, size_t size, > > > @@ -391,10 +402,11 @@ void qemu_put_buffer(QEMUFile *f, const uint8_t *buf, size_t size) > > > } > > > memcpy(f->buf + f->buf_index, buf, l); > > > f->bytes_xfer += l; > > > - add_to_iovec(f, f->buf + f->buf_index, l, false); > > > - f->buf_index += l; > > > - if (f->buf_index == IO_BUF_SIZE) { > > > - qemu_fflush(f); > > > + if (!add_to_iovec(f, f->buf + f->buf_index, l, false)) { > > > + f->buf_index += l; > > > + if (f->buf_index == IO_BUF_SIZE) { > > > + qemu_fflush(f); > > > + } > > > } > > > if (qemu_file_get_error(f)) { > > > break; > > > @@ -412,10 +424,11 @@ void qemu_put_byte(QEMUFile *f, int v) > > > > > > f->buf[f->buf_index] = v; > > > f->bytes_xfer++; > > > - add_to_iovec(f, f->buf + f->buf_index, 1, false); > > > - f->buf_index++; > > > - if (f->buf_index == IO_BUF_SIZE) { > > > - qemu_fflush(f); > > > + if (!add_to_iovec(f, f->buf + f->buf_index, 1, false)) { > > > + f->buf_index++; > > > + if (f->buf_index == IO_BUF_SIZE) { > > > + qemu_fflush(f); > > > + } > > > } > > > } > > > > > > @@ -717,10 +730,11 @@ ssize_t qemu_put_compression_data(QEMUFile *f, z_stream *stream, > > > } > > > > > > qemu_put_be32(f, blen); > > > - add_to_iovec(f, f->buf + f->buf_index, blen, false); > > > - f->buf_index += blen; > > > - if (f->buf_index == IO_BUF_SIZE) { > > > - qemu_fflush(f); > > > + if (!add_to_iovec(f, f->buf + f->buf_index, blen, false)) { > > > + f->buf_index += blen; > > > + if (f->buf_index == IO_BUF_SIZE) { > > > + qemu_fflush(f); > > > + } > > > } > > > return blen + sizeof(int32_t); > > > } > > > -- > > > 2.17.1 > > > > > -- > > Dr. David Alan Gilbert / dgilbert@redhat.com / Manchester, UK > > > > Regards, > Daniel > -- > |: https://berrange.com -o- https://www.flickr.com/photos/dberrange :| > |: https://libvirt.org -o- https://fstop138.berrange.com :| > |: https://entangle-photo.org -o- https://www.instagram.com/dberrange :| -- Dr. David Alan Gilbert / dgilbert@redhat.com / Manchester, UK
On Fri, Aug 23, 2019 at 12:06:09PM +0100, Dr. David Alan Gilbert wrote: >(Copying Dan in) > >* Wei Yang (richardw.yang@linux.intel.com) wrote: >> In add_to_iovec(), qemu_fflush() will be called if iovec is full. If >> this happens, buf_index is reset. Currently, this is not checked and >> buf_index would always been adjust with buf size. >> >> This is not harmful, but will waste some space in file buffer. > >That's a nice find. > >> This patch make add_to_iovec() return 1 when it has flushed the file. >> Then the caller could check the return value to see whether it is >> necessary to adjust the buf_index any more. >> >> Signed-off-by: Wei Yang <richardw.yang@linux.intel.com> > >Reviewed-by: Dr. David Alan Gilbert <dgilbert@redhat.com> > >(I wonder if there's a way to wrap that little add_to_iovec, check, add >to index, flush in a little wrapper). > >Dave > >> --- >> migration/qemu-file.c | 42 ++++++++++++++++++++++++++++-------------- >> 1 file changed, 28 insertions(+), 14 deletions(-) >> >> diff --git a/migration/qemu-file.c b/migration/qemu-file.c >> index 35c22605dd..05d9f42ddb 100644 >> --- a/migration/qemu-file.c >> +++ b/migration/qemu-file.c >> @@ -343,8 +343,16 @@ int qemu_fclose(QEMUFile *f) >> return ret; >> } >> >> -static void add_to_iovec(QEMUFile *f, const uint8_t *buf, size_t size, >> - bool may_free) >> +/* >> + * Add buf to iovec. Do flush if iovec is full. >> + * >> + * Return values: >> + * 1 iovec is full and flushed >> + * 0 iovec is not flushed >> + * >> + */ >> +static int add_to_iovec(QEMUFile *f, const uint8_t *buf, size_t size, >> + bool may_free) >> { >> /* check for adjacent buffer and coalesce them */ >> if (f->iovcnt > 0 && buf == f->iov[f->iovcnt - 1].iov_base + >> @@ -362,7 +370,10 @@ static void add_to_iovec(QEMUFile *f, const uint8_t *buf, size_t size, >> >> if (f->iovcnt >= MAX_IOV_SIZE) { >> qemu_fflush(f); >> + return 1; >> } >> + >> + return 0; >> } >> >> void qemu_put_buffer_async(QEMUFile *f, const uint8_t *buf, size_t size, >> @@ -391,10 +402,11 @@ void qemu_put_buffer(QEMUFile *f, const uint8_t *buf, size_t size) >> } >> memcpy(f->buf + f->buf_index, buf, l); >> f->bytes_xfer += l; >> - add_to_iovec(f, f->buf + f->buf_index, l, false); >> - f->buf_index += l; >> - if (f->buf_index == IO_BUF_SIZE) { >> - qemu_fflush(f); >> + if (!add_to_iovec(f, f->buf + f->buf_index, l, false)) { >> + f->buf_index += l; >> + if (f->buf_index == IO_BUF_SIZE) { >> + qemu_fflush(f); >> + } You mean put these four lines into a wrapper? Name it as add_buf_to_iovec? >> } >> if (qemu_file_get_error(f)) { >> break; >> @@ -412,10 +424,11 @@ void qemu_put_byte(QEMUFile *f, int v) >> >> f->buf[f->buf_index] = v; >> f->bytes_xfer++; >> - add_to_iovec(f, f->buf + f->buf_index, 1, false); >> - f->buf_index++; >> - if (f->buf_index == IO_BUF_SIZE) { >> - qemu_fflush(f); >> + if (!add_to_iovec(f, f->buf + f->buf_index, 1, false)) { >> + f->buf_index++; >> + if (f->buf_index == IO_BUF_SIZE) { >> + qemu_fflush(f); >> + } >> } >> } >> >> @@ -717,10 +730,11 @@ ssize_t qemu_put_compression_data(QEMUFile *f, z_stream *stream, >> } >> >> qemu_put_be32(f, blen); >> - add_to_iovec(f, f->buf + f->buf_index, blen, false); >> - f->buf_index += blen; >> - if (f->buf_index == IO_BUF_SIZE) { >> - qemu_fflush(f); >> + if (!add_to_iovec(f, f->buf + f->buf_index, blen, false)) { >> + f->buf_index += blen; >> + if (f->buf_index == IO_BUF_SIZE) { >> + qemu_fflush(f); >> + } >> } >> return blen + sizeof(int32_t); >> } >> -- >> 2.17.1 >> >-- >Dr. David Alan Gilbert / dgilbert@redhat.com / Manchester, UK
On Fri, Aug 23, 2019 at 02:05:02PM +0100, Dr. David Alan Gilbert wrote: >* Daniel P. Berrang? (berrange@redhat.com) wrote: >> On Fri, Aug 23, 2019 at 12:06:09PM +0100, Dr. David Alan Gilbert wrote: >> > (Copying Dan in) >> > >> > * Wei Yang (richardw.yang@linux.intel.com) wrote: >> > > In add_to_iovec(), qemu_fflush() will be called if iovec is full. If >> > > this happens, buf_index is reset. Currently, this is not checked and >> > > buf_index would always been adjust with buf size. >> > > >> > > This is not harmful, but will waste some space in file buffer. >> > >> > That's a nice find. >> > >> > > This patch make add_to_iovec() return 1 when it has flushed the file. >> > > Then the caller could check the return value to see whether it is >> > > necessary to adjust the buf_index any more. >> > > >> > > Signed-off-by: Wei Yang <richardw.yang@linux.intel.com> >> > >> > Reviewed-by: Dr. David Alan Gilbert <dgilbert@redhat.com> >> > >> > (I wonder if there's a way to wrap that little add_to_iovec, check, add >> > to index, flush in a little wrapper). >> >> Given the name "add_to_iovec" I think it is pretty surprising >> that it calls "qemu_flush" at all. >> >> It is also pretty wierd that we're checking two different >> conditions in two different places. >> >> Right now the code is essentially doing this: >> >> if (f->iovcnt >= MAX_IOV_SIZE) { >> qemu_fflush(f); >> } >> if (f->buf_index == IO_BUF_SIZE) { >> qemu_fflush(f); >> } >> >> Except that in the qemu_put_buffer_async() case, we're >> only doing the first of these two checks. This feels >> very odd indeed - I would have thought either it should >> do both, or do neither. > >No, there's two separate types of buffers. > >There's f->buf which is a single allocated buffer in the QEMUFile >with an offset buf_index, and there are arbitrary RAM pages >added typically via qemu_put_buffer_async. > qemu_put_buffer_async is the only one which put a range not in f->buf into the iovec. And one thing confused me is even its name is async, add_to_iovec still would call qemu_fflush when iovec is full. So it is not a always async function. From the function name, it is a little difficult to differentiate qemu_put_buffer and qemu_put_buffer_async. >The check for >= IO_BUF_SIZE is only done when adding to the f->buf, >where as the check on f->iovcnt is done when you add an element to >the iovec and that can happen potentially in either case. > >Dave > >> Assuming doing both flushs is ok for qemu_put_buffer_async >> then I'd suggest renaming 'add_to_iovec' to 'queue_buffer' >> and have that method do both of these qemu_fflush() calls. >> >> > > --- >> > > migration/qemu-file.c | 42 ++++++++++++++++++++++++++++-------------- >> > > 1 file changed, 28 insertions(+), 14 deletions(-) >> > > >> > > diff --git a/migration/qemu-file.c b/migration/qemu-file.c >> > > index 35c22605dd..05d9f42ddb 100644 >> > > --- a/migration/qemu-file.c >> > > +++ b/migration/qemu-file.c >> > > @@ -343,8 +343,16 @@ int qemu_fclose(QEMUFile *f) >> > > return ret; >> > > } >> > > >> > > -static void add_to_iovec(QEMUFile *f, const uint8_t *buf, size_t size, >> > > - bool may_free) >> > > +/* >> > > + * Add buf to iovec. Do flush if iovec is full. >> > > + * >> > > + * Return values: >> > > + * 1 iovec is full and flushed >> > > + * 0 iovec is not flushed >> > > + * >> > > + */ >> > > +static int add_to_iovec(QEMUFile *f, const uint8_t *buf, size_t size, >> > > + bool may_free) >> > > { >> > > /* check for adjacent buffer and coalesce them */ >> > > if (f->iovcnt > 0 && buf == f->iov[f->iovcnt - 1].iov_base + >> > > @@ -362,7 +370,10 @@ static void add_to_iovec(QEMUFile *f, const uint8_t *buf, size_t size, >> > > >> > > if (f->iovcnt >= MAX_IOV_SIZE) { >> > > qemu_fflush(f); >> > > + return 1; >> > > } >> > > + >> > > + return 0; >> > > } >> > > >> > > void qemu_put_buffer_async(QEMUFile *f, const uint8_t *buf, size_t size, >> > > @@ -391,10 +402,11 @@ void qemu_put_buffer(QEMUFile *f, const uint8_t *buf, size_t size) >> > > } >> > > memcpy(f->buf + f->buf_index, buf, l); >> > > f->bytes_xfer += l; >> > > - add_to_iovec(f, f->buf + f->buf_index, l, false); >> > > - f->buf_index += l; >> > > - if (f->buf_index == IO_BUF_SIZE) { >> > > - qemu_fflush(f); >> > > + if (!add_to_iovec(f, f->buf + f->buf_index, l, false)) { >> > > + f->buf_index += l; >> > > + if (f->buf_index == IO_BUF_SIZE) { >> > > + qemu_fflush(f); >> > > + } >> > > } >> > > if (qemu_file_get_error(f)) { >> > > break; >> > > @@ -412,10 +424,11 @@ void qemu_put_byte(QEMUFile *f, int v) >> > > >> > > f->buf[f->buf_index] = v; >> > > f->bytes_xfer++; >> > > - add_to_iovec(f, f->buf + f->buf_index, 1, false); >> > > - f->buf_index++; >> > > - if (f->buf_index == IO_BUF_SIZE) { >> > > - qemu_fflush(f); >> > > + if (!add_to_iovec(f, f->buf + f->buf_index, 1, false)) { >> > > + f->buf_index++; >> > > + if (f->buf_index == IO_BUF_SIZE) { >> > > + qemu_fflush(f); >> > > + } >> > > } >> > > } >> > > >> > > @@ -717,10 +730,11 @@ ssize_t qemu_put_compression_data(QEMUFile *f, z_stream *stream, >> > > } >> > > >> > > qemu_put_be32(f, blen); >> > > - add_to_iovec(f, f->buf + f->buf_index, blen, false); >> > > - f->buf_index += blen; >> > > - if (f->buf_index == IO_BUF_SIZE) { >> > > - qemu_fflush(f); >> > > + if (!add_to_iovec(f, f->buf + f->buf_index, blen, false)) { >> > > + f->buf_index += blen; >> > > + if (f->buf_index == IO_BUF_SIZE) { >> > > + qemu_fflush(f); >> > > + } >> > > } >> > > return blen + sizeof(int32_t); >> > > } >> > > -- >> > > 2.17.1 >> > > >> > -- >> > Dr. David Alan Gilbert / dgilbert@redhat.com / Manchester, UK >> > >> >> Regards, >> Daniel >> -- >> |: https://berrange.com -o- https://www.flickr.com/photos/dberrange :| >> |: https://libvirt.org -o- https://fstop138.berrange.com :| >> |: https://entangle-photo.org -o- https://www.instagram.com/dberrange :| >-- >Dr. David Alan Gilbert / dgilbert@redhat.com / Manchester, UK
* Wei Yang (richard.weiyang@gmail.com) wrote: > On Fri, Aug 23, 2019 at 02:05:02PM +0100, Dr. David Alan Gilbert wrote: > >* Daniel P. Berrang? (berrange@redhat.com) wrote: > >> On Fri, Aug 23, 2019 at 12:06:09PM +0100, Dr. David Alan Gilbert wrote: > >> > (Copying Dan in) > >> > > >> > * Wei Yang (richardw.yang@linux.intel.com) wrote: > >> > > In add_to_iovec(), qemu_fflush() will be called if iovec is full. If > >> > > this happens, buf_index is reset. Currently, this is not checked and > >> > > buf_index would always been adjust with buf size. > >> > > > >> > > This is not harmful, but will waste some space in file buffer. > >> > > >> > That's a nice find. > >> > > >> > > This patch make add_to_iovec() return 1 when it has flushed the file. > >> > > Then the caller could check the return value to see whether it is > >> > > necessary to adjust the buf_index any more. > >> > > > >> > > Signed-off-by: Wei Yang <richardw.yang@linux.intel.com> > >> > > >> > Reviewed-by: Dr. David Alan Gilbert <dgilbert@redhat.com> > >> > > >> > (I wonder if there's a way to wrap that little add_to_iovec, check, add > >> > to index, flush in a little wrapper). > >> > >> Given the name "add_to_iovec" I think it is pretty surprising > >> that it calls "qemu_flush" at all. > >> > >> It is also pretty wierd that we're checking two different > >> conditions in two different places. > >> > >> Right now the code is essentially doing this: > >> > >> if (f->iovcnt >= MAX_IOV_SIZE) { > >> qemu_fflush(f); > >> } > >> if (f->buf_index == IO_BUF_SIZE) { > >> qemu_fflush(f); > >> } > >> > >> Except that in the qemu_put_buffer_async() case, we're > >> only doing the first of these two checks. This feels > >> very odd indeed - I would have thought either it should > >> do both, or do neither. > > > >No, there's two separate types of buffers. > > > >There's f->buf which is a single allocated buffer in the QEMUFile > >with an offset buf_index, and there are arbitrary RAM pages > >added typically via qemu_put_buffer_async. > > > > qemu_put_buffer_async is the only one which put a range not in f->buf into the > iovec. > > And one thing confused me is even its name is async, add_to_iovec still would > call qemu_fflush when iovec is full. So it is not a always async function. > From the function name, it is a little difficult to differentiate > qemu_put_buffer and qemu_put_buffer_async. It's a bit confusing, but I think it is the right name; the 'async' refers to the fact that the data is read from the buffer we pass sometime later, not synchronously before the return of the qemu_put_buffer_async call. Dave > >The check for >= IO_BUF_SIZE is only done when adding to the f->buf, > >where as the check on f->iovcnt is done when you add an element to > >the iovec and that can happen potentially in either case. > > > >Dave > > > >> Assuming doing both flushs is ok for qemu_put_buffer_async > >> then I'd suggest renaming 'add_to_iovec' to 'queue_buffer' > >> and have that method do both of these qemu_fflush() calls. > >> > >> > > --- > >> > > migration/qemu-file.c | 42 ++++++++++++++++++++++++++++-------------- > >> > > 1 file changed, 28 insertions(+), 14 deletions(-) > >> > > > >> > > diff --git a/migration/qemu-file.c b/migration/qemu-file.c > >> > > index 35c22605dd..05d9f42ddb 100644 > >> > > --- a/migration/qemu-file.c > >> > > +++ b/migration/qemu-file.c > >> > > @@ -343,8 +343,16 @@ int qemu_fclose(QEMUFile *f) > >> > > return ret; > >> > > } > >> > > > >> > > -static void add_to_iovec(QEMUFile *f, const uint8_t *buf, size_t size, > >> > > - bool may_free) > >> > > +/* > >> > > + * Add buf to iovec. Do flush if iovec is full. > >> > > + * > >> > > + * Return values: > >> > > + * 1 iovec is full and flushed > >> > > + * 0 iovec is not flushed > >> > > + * > >> > > + */ > >> > > +static int add_to_iovec(QEMUFile *f, const uint8_t *buf, size_t size, > >> > > + bool may_free) > >> > > { > >> > > /* check for adjacent buffer and coalesce them */ > >> > > if (f->iovcnt > 0 && buf == f->iov[f->iovcnt - 1].iov_base + > >> > > @@ -362,7 +370,10 @@ static void add_to_iovec(QEMUFile *f, const uint8_t *buf, size_t size, > >> > > > >> > > if (f->iovcnt >= MAX_IOV_SIZE) { > >> > > qemu_fflush(f); > >> > > + return 1; > >> > > } > >> > > + > >> > > + return 0; > >> > > } > >> > > > >> > > void qemu_put_buffer_async(QEMUFile *f, const uint8_t *buf, size_t size, > >> > > @@ -391,10 +402,11 @@ void qemu_put_buffer(QEMUFile *f, const uint8_t *buf, size_t size) > >> > > } > >> > > memcpy(f->buf + f->buf_index, buf, l); > >> > > f->bytes_xfer += l; > >> > > - add_to_iovec(f, f->buf + f->buf_index, l, false); > >> > > - f->buf_index += l; > >> > > - if (f->buf_index == IO_BUF_SIZE) { > >> > > - qemu_fflush(f); > >> > > + if (!add_to_iovec(f, f->buf + f->buf_index, l, false)) { > >> > > + f->buf_index += l; > >> > > + if (f->buf_index == IO_BUF_SIZE) { > >> > > + qemu_fflush(f); > >> > > + } > >> > > } > >> > > if (qemu_file_get_error(f)) { > >> > > break; > >> > > @@ -412,10 +424,11 @@ void qemu_put_byte(QEMUFile *f, int v) > >> > > > >> > > f->buf[f->buf_index] = v; > >> > > f->bytes_xfer++; > >> > > - add_to_iovec(f, f->buf + f->buf_index, 1, false); > >> > > - f->buf_index++; > >> > > - if (f->buf_index == IO_BUF_SIZE) { > >> > > - qemu_fflush(f); > >> > > + if (!add_to_iovec(f, f->buf + f->buf_index, 1, false)) { > >> > > + f->buf_index++; > >> > > + if (f->buf_index == IO_BUF_SIZE) { > >> > > + qemu_fflush(f); > >> > > + } > >> > > } > >> > > } > >> > > > >> > > @@ -717,10 +730,11 @@ ssize_t qemu_put_compression_data(QEMUFile *f, z_stream *stream, > >> > > } > >> > > > >> > > qemu_put_be32(f, blen); > >> > > - add_to_iovec(f, f->buf + f->buf_index, blen, false); > >> > > - f->buf_index += blen; > >> > > - if (f->buf_index == IO_BUF_SIZE) { > >> > > - qemu_fflush(f); > >> > > + if (!add_to_iovec(f, f->buf + f->buf_index, blen, false)) { > >> > > + f->buf_index += blen; > >> > > + if (f->buf_index == IO_BUF_SIZE) { > >> > > + qemu_fflush(f); > >> > > + } > >> > > } > >> > > return blen + sizeof(int32_t); > >> > > } > >> > > -- > >> > > 2.17.1 > >> > > > >> > -- > >> > Dr. David Alan Gilbert / dgilbert@redhat.com / Manchester, UK > >> > > >> > >> Regards, > >> Daniel > >> -- > >> |: https://berrange.com -o- https://www.flickr.com/photos/dberrange :| > >> |: https://libvirt.org -o- https://fstop138.berrange.com :| > >> |: https://entangle-photo.org -o- https://www.instagram.com/dberrange :| > >-- > >Dr. David Alan Gilbert / dgilbert@redhat.com / Manchester, UK > > -- > Wei Yang > Help you, Help me -- Dr. David Alan Gilbert / dgilbert@redhat.com / Manchester, UK
* Wei Yang (richard.weiyang@gmail.com) wrote: > On Fri, Aug 23, 2019 at 12:06:09PM +0100, Dr. David Alan Gilbert wrote: > >(Copying Dan in) > > > >* Wei Yang (richardw.yang@linux.intel.com) wrote: > >> In add_to_iovec(), qemu_fflush() will be called if iovec is full. If > >> this happens, buf_index is reset. Currently, this is not checked and > >> buf_index would always been adjust with buf size. > >> > >> This is not harmful, but will waste some space in file buffer. > > > >That's a nice find. > > > >> This patch make add_to_iovec() return 1 when it has flushed the file. > >> Then the caller could check the return value to see whether it is > >> necessary to adjust the buf_index any more. > >> > >> Signed-off-by: Wei Yang <richardw.yang@linux.intel.com> > > > >Reviewed-by: Dr. David Alan Gilbert <dgilbert@redhat.com> > > > >(I wonder if there's a way to wrap that little add_to_iovec, check, add > >to index, flush in a little wrapper). > > > >Dave > > > >> --- > >> migration/qemu-file.c | 42 ++++++++++++++++++++++++++++-------------- > >> 1 file changed, 28 insertions(+), 14 deletions(-) > >> > >> diff --git a/migration/qemu-file.c b/migration/qemu-file.c > >> index 35c22605dd..05d9f42ddb 100644 > >> --- a/migration/qemu-file.c > >> +++ b/migration/qemu-file.c > >> @@ -343,8 +343,16 @@ int qemu_fclose(QEMUFile *f) > >> return ret; > >> } > >> > >> -static void add_to_iovec(QEMUFile *f, const uint8_t *buf, size_t size, > >> - bool may_free) > >> +/* > >> + * Add buf to iovec. Do flush if iovec is full. > >> + * > >> + * Return values: > >> + * 1 iovec is full and flushed > >> + * 0 iovec is not flushed > >> + * > >> + */ > >> +static int add_to_iovec(QEMUFile *f, const uint8_t *buf, size_t size, > >> + bool may_free) > >> { > >> /* check for adjacent buffer and coalesce them */ > >> if (f->iovcnt > 0 && buf == f->iov[f->iovcnt - 1].iov_base + > >> @@ -362,7 +370,10 @@ static void add_to_iovec(QEMUFile *f, const uint8_t *buf, size_t size, > >> > >> if (f->iovcnt >= MAX_IOV_SIZE) { > >> qemu_fflush(f); > >> + return 1; > >> } > >> + > >> + return 0; > >> } > >> > >> void qemu_put_buffer_async(QEMUFile *f, const uint8_t *buf, size_t size, > >> @@ -391,10 +402,11 @@ void qemu_put_buffer(QEMUFile *f, const uint8_t *buf, size_t size) > >> } > >> memcpy(f->buf + f->buf_index, buf, l); > >> f->bytes_xfer += l; > >> - add_to_iovec(f, f->buf + f->buf_index, l, false); > >> - f->buf_index += l; > >> - if (f->buf_index == IO_BUF_SIZE) { > >> - qemu_fflush(f); > >> + if (!add_to_iovec(f, f->buf + f->buf_index, l, false)) { > >> + f->buf_index += l; > >> + if (f->buf_index == IO_BUF_SIZE) { > >> + qemu_fflush(f); > >> + } > > You mean put these four lines into a wrapper? > > Name it as add_buf_to_iovec? Yes. Dave > >> } > >> if (qemu_file_get_error(f)) { > >> break; > >> @@ -412,10 +424,11 @@ void qemu_put_byte(QEMUFile *f, int v) > >> > >> f->buf[f->buf_index] = v; > >> f->bytes_xfer++; > >> - add_to_iovec(f, f->buf + f->buf_index, 1, false); > >> - f->buf_index++; > >> - if (f->buf_index == IO_BUF_SIZE) { > >> - qemu_fflush(f); > >> + if (!add_to_iovec(f, f->buf + f->buf_index, 1, false)) { > >> + f->buf_index++; > >> + if (f->buf_index == IO_BUF_SIZE) { > >> + qemu_fflush(f); > >> + } > >> } > >> } > >> > >> @@ -717,10 +730,11 @@ ssize_t qemu_put_compression_data(QEMUFile *f, z_stream *stream, > >> } > >> > >> qemu_put_be32(f, blen); > >> - add_to_iovec(f, f->buf + f->buf_index, blen, false); > >> - f->buf_index += blen; > >> - if (f->buf_index == IO_BUF_SIZE) { > >> - qemu_fflush(f); > >> + if (!add_to_iovec(f, f->buf + f->buf_index, blen, false)) { > >> + f->buf_index += blen; > >> + if (f->buf_index == IO_BUF_SIZE) { > >> + qemu_fflush(f); > >> + } > >> } > >> return blen + sizeof(int32_t); > >> } > >> -- > >> 2.17.1 > >> > >-- > >Dr. David Alan Gilbert / dgilbert@redhat.com / Manchester, UK > > -- > Wei Yang > Help you, Help me -- Dr. David Alan Gilbert / dgilbert@redhat.com / Manchester, UK
On Tue, Sep 03, 2019 at 07:43:24PM +0100, Dr. David Alan Gilbert wrote: >* Wei Yang (richard.weiyang@gmail.com) wrote: >> On Fri, Aug 23, 2019 at 12:06:09PM +0100, Dr. David Alan Gilbert wrote: >> >(Copying Dan in) >> > >> >* Wei Yang (richardw.yang@linux.intel.com) wrote: >> >> In add_to_iovec(), qemu_fflush() will be called if iovec is full. If >> >> this happens, buf_index is reset. Currently, this is not checked and >> >> buf_index would always been adjust with buf size. >> >> >> >> This is not harmful, but will waste some space in file buffer. >> > >> >That's a nice find. >> > >> >> This patch make add_to_iovec() return 1 when it has flushed the file. >> >> Then the caller could check the return value to see whether it is >> >> necessary to adjust the buf_index any more. >> >> >> >> Signed-off-by: Wei Yang <richardw.yang@linux.intel.com> >> > >> >Reviewed-by: Dr. David Alan Gilbert <dgilbert@redhat.com> >> > >> >(I wonder if there's a way to wrap that little add_to_iovec, check, add >> >to index, flush in a little wrapper). >> > >> >Dave >> > >> >> --- >> >> migration/qemu-file.c | 42 ++++++++++++++++++++++++++++-------------- >> >> 1 file changed, 28 insertions(+), 14 deletions(-) >> >> >> >> diff --git a/migration/qemu-file.c b/migration/qemu-file.c >> >> index 35c22605dd..05d9f42ddb 100644 >> >> --- a/migration/qemu-file.c >> >> +++ b/migration/qemu-file.c >> >> @@ -343,8 +343,16 @@ int qemu_fclose(QEMUFile *f) >> >> return ret; >> >> } >> >> >> >> -static void add_to_iovec(QEMUFile *f, const uint8_t *buf, size_t size, >> >> - bool may_free) >> >> +/* >> >> + * Add buf to iovec. Do flush if iovec is full. >> >> + * >> >> + * Return values: >> >> + * 1 iovec is full and flushed >> >> + * 0 iovec is not flushed >> >> + * >> >> + */ >> >> +static int add_to_iovec(QEMUFile *f, const uint8_t *buf, size_t size, >> >> + bool may_free) >> >> { >> >> /* check for adjacent buffer and coalesce them */ >> >> if (f->iovcnt > 0 && buf == f->iov[f->iovcnt - 1].iov_base + >> >> @@ -362,7 +370,10 @@ static void add_to_iovec(QEMUFile *f, const uint8_t *buf, size_t size, >> >> >> >> if (f->iovcnt >= MAX_IOV_SIZE) { >> >> qemu_fflush(f); >> >> + return 1; >> >> } >> >> + >> >> + return 0; >> >> } >> >> >> >> void qemu_put_buffer_async(QEMUFile *f, const uint8_t *buf, size_t size, >> >> @@ -391,10 +402,11 @@ void qemu_put_buffer(QEMUFile *f, const uint8_t *buf, size_t size) >> >> } >> >> memcpy(f->buf + f->buf_index, buf, l); >> >> f->bytes_xfer += l; >> >> - add_to_iovec(f, f->buf + f->buf_index, l, false); >> >> - f->buf_index += l; >> >> - if (f->buf_index == IO_BUF_SIZE) { >> >> - qemu_fflush(f); >> >> + if (!add_to_iovec(f, f->buf + f->buf_index, l, false)) { >> >> + f->buf_index += l; >> >> + if (f->buf_index == IO_BUF_SIZE) { >> >> + qemu_fflush(f); >> >> + } >> >> You mean put these four lines into a wrapper? >> >> Name it as add_buf_to_iovec? > >Yes. Sure, Let me prepare v2 with this.
diff --git a/migration/qemu-file.c b/migration/qemu-file.c index 35c22605dd..05d9f42ddb 100644 --- a/migration/qemu-file.c +++ b/migration/qemu-file.c @@ -343,8 +343,16 @@ int qemu_fclose(QEMUFile *f) return ret; } -static void add_to_iovec(QEMUFile *f, const uint8_t *buf, size_t size, - bool may_free) +/* + * Add buf to iovec. Do flush if iovec is full. + * + * Return values: + * 1 iovec is full and flushed + * 0 iovec is not flushed + * + */ +static int add_to_iovec(QEMUFile *f, const uint8_t *buf, size_t size, + bool may_free) { /* check for adjacent buffer and coalesce them */ if (f->iovcnt > 0 && buf == f->iov[f->iovcnt - 1].iov_base + @@ -362,7 +370,10 @@ static void add_to_iovec(QEMUFile *f, const uint8_t *buf, size_t size, if (f->iovcnt >= MAX_IOV_SIZE) { qemu_fflush(f); + return 1; } + + return 0; } void qemu_put_buffer_async(QEMUFile *f, const uint8_t *buf, size_t size, @@ -391,10 +402,11 @@ void qemu_put_buffer(QEMUFile *f, const uint8_t *buf, size_t size) } memcpy(f->buf + f->buf_index, buf, l); f->bytes_xfer += l; - add_to_iovec(f, f->buf + f->buf_index, l, false); - f->buf_index += l; - if (f->buf_index == IO_BUF_SIZE) { - qemu_fflush(f); + if (!add_to_iovec(f, f->buf + f->buf_index, l, false)) { + f->buf_index += l; + if (f->buf_index == IO_BUF_SIZE) { + qemu_fflush(f); + } } if (qemu_file_get_error(f)) { break; @@ -412,10 +424,11 @@ void qemu_put_byte(QEMUFile *f, int v) f->buf[f->buf_index] = v; f->bytes_xfer++; - add_to_iovec(f, f->buf + f->buf_index, 1, false); - f->buf_index++; - if (f->buf_index == IO_BUF_SIZE) { - qemu_fflush(f); + if (!add_to_iovec(f, f->buf + f->buf_index, 1, false)) { + f->buf_index++; + if (f->buf_index == IO_BUF_SIZE) { + qemu_fflush(f); + } } } @@ -717,10 +730,11 @@ ssize_t qemu_put_compression_data(QEMUFile *f, z_stream *stream, } qemu_put_be32(f, blen); - add_to_iovec(f, f->buf + f->buf_index, blen, false); - f->buf_index += blen; - if (f->buf_index == IO_BUF_SIZE) { - qemu_fflush(f); + if (!add_to_iovec(f, f->buf + f->buf_index, blen, false)) { + f->buf_index += blen; + if (f->buf_index == IO_BUF_SIZE) { + qemu_fflush(f); + } } return blen + sizeof(int32_t); }
In add_to_iovec(), qemu_fflush() will be called if iovec is full. If this happens, buf_index is reset. Currently, this is not checked and buf_index would always been adjust with buf size. This is not harmful, but will waste some space in file buffer. This patch make add_to_iovec() return 1 when it has flushed the file. Then the caller could check the return value to see whether it is necessary to adjust the buf_index any more. Signed-off-by: Wei Yang <richardw.yang@linux.intel.com> --- migration/qemu-file.c | 42 ++++++++++++++++++++++++++++-------------- 1 file changed, 28 insertions(+), 14 deletions(-)