Re: [Qemu-devel] [PATCH v6 08/47] Add qemu_get_buffer_less_copy to avoid copies some of the time

["Dr. David Alan Gilbert" <dgilbert@redhat.com>]

* Amit Shah (amit.shah@redhat.com) wrote:
> On (Tue) 14 Apr 2015 [18:03:34], Dr. David Alan Gilbert (git) wrote:
> > From: "Dr. David Alan Gilbert" <dgilbert@redhat.com>
> > 
> > qemu_get_buffer always copies the data it reads to a users buffer,
> > however in many cases the file buffer inside qemu_file could be given
> > back to the caller, avoiding the copy.  This isn't always possible
> > depending on the size and alignment of the data.
> > 
> > Thus 'qemu_get_buffer_less_copy' either copies the data to a supplied
> > buffer or updates a pointer to the internal buffer if convenient.
> > 
> > Signed-off-by: Dr. David Alan Gilbert <dgilbert@redhat.com>
> > ---
> >  include/migration/qemu-file.h |  2 ++
> >  migration/qemu-file.c         | 45 +++++++++++++++++++++++++++++++++++++++++++
> >  2 files changed, 47 insertions(+)
> > 
> > diff --git a/include/migration/qemu-file.h b/include/migration/qemu-file.h
> > index 3fe545e..4cac58f 100644
> > --- a/include/migration/qemu-file.h
> > +++ b/include/migration/qemu-file.h
> > @@ -159,6 +159,8 @@ void qemu_put_be32(QEMUFile *f, unsigned int v);
> >  void qemu_put_be64(QEMUFile *f, uint64_t v);
> >  int qemu_peek_buffer(QEMUFile *f, uint8_t **buf, int size, size_t offset);
> >  int qemu_get_buffer(QEMUFile *f, uint8_t *buf, int size);
> > +int qemu_get_buffer_less_copy(QEMUFile *f, uint8_t **buf, int size);
> > +
> >  /*
> >   * Note that you can only peek continuous bytes from where the current pointer
> >   * is; you aren't guaranteed to be able to peak to +n bytes unless you've
> > diff --git a/migration/qemu-file.c b/migration/qemu-file.c
> > index 8dc5767..ec3a598 100644
> > --- a/migration/qemu-file.c
> > +++ b/migration/qemu-file.c
> > @@ -426,6 +426,51 @@ int qemu_get_buffer(QEMUFile *f, uint8_t *buf, int size)
> >  }
> >  
> >  /*
> > + * Read 'size' bytes of data from the file.
> > + * 'size' can be larger than the internal buffer.
> > + *
> > + * The data:
> > + *   may be held on an internal buffer (in which case *buf is updated
> > + *     to point to it) that is valid until the next qemu_file operation.
> > + * OR
> > + *   will be copied to the *buf that was passed in.
> > + *
> > + * The code tries to avoid the copy if possible.
> 
> So is it expected that callers will store the originally-allocated
> start location, so that g_free can be called on the correct location
> in either case?  If that's a requirement, this text needs to be
> updated.
> 
> If not (alternative idea below), text needs to be updated as well.

see reply below.

> 
> > + * It will return size bytes unless there was an error, in which case it will
> > + * return as many as it managed to read (assuming blocking fd's which
> > + * all current QEMUFile are)
> > + */
> > +int qemu_get_buffer_less_copy(QEMUFile *f, uint8_t **buf, int size)
> > +{
> > +    int pending = size;
> > +    int done = 0;
> > +    bool first = true;
> > +
> > +    while (pending > 0) {
> > +        int res;
> > +        uint8_t *src;
> > +
> > +        res = qemu_peek_buffer(f, &src, MIN(pending, IO_BUF_SIZE), 0);
> > +        if (res == 0) {
> > +            return done;
> > +        }
> > +        qemu_file_skip(f, res);
> > +        done += res;
> > +        pending -= res;
> > +        if (first && res == size) {
> > +            *buf = src;
> 
> So we've got to assume that buf was allocated by the calling function,
> and since we're modifying the pointer (alternative idea to one above),
> should we unallocate it here?  Can lead to a bad g_free later, or a
> memleak.

My use tends to involve a buffer allocated once:

        uint8_t *mybuffer = g_malloc(...)

        while (aloop) {
            uint8_t *ourdata = mybuffer;

            if (qemu_get_buffer_less_copy(f, &ourdata, size)...) {
                do something with *ourdata
            }
  
        }
        g_free(mybuffer);

The pointer that's passed into qemu_get_buffer_less_copy is only a copy
of the allocation pointer, and thus you're not losing anything when it
changes it.

I've added the following text, does this make it clearer?

 * Note: Since **buf may get changed, the caller should take care to
 *       keep a pointer to the original buffer if it needs to deallocate it.

> > +            return done;
> 
> How about just 'break' instead of return?

Changed.

> 
> > +        } else {
> > +            first = false;
> > +            memcpy(buf, src, res);
> > +            buf += res;
> 
> In either case (break or return), the 'else' can be dropped..

Changed.

Thanks,

> 
> 		Amit
--
Dr. David Alan Gilbert / dgilbert@redhat.com / Manchester, UK