From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:35132) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1YvM6d-0004ql-RS for qemu-devel@nongnu.org; Thu, 21 May 2015 04:45:33 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1YvM6a-0006on-GU for qemu-devel@nongnu.org; Thu, 21 May 2015 04:45:31 -0400 Received: from mx1.redhat.com ([209.132.183.28]:59038) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1YvM6a-0006oe-8N for qemu-devel@nongnu.org; Thu, 21 May 2015 04:45:28 -0400 Date: Thu, 21 May 2015 09:45:19 +0100 From: "Dr. David Alan Gilbert" Message-ID: <20150521084519.GC2129@work-vm> References: <1429031053-4454-1-git-send-email-dgilbert@redhat.com> <1429031053-4454-9-git-send-email-dgilbert@redhat.com> <20150521070901.GM15452@grmbl.mre> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: <20150521070901.GM15452@grmbl.mre> Subject: Re: [Qemu-devel] [PATCH v6 08/47] Add qemu_get_buffer_less_copy to avoid copies some of the time List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Amit Shah Cc: aarcange@redhat.com, yamahata@private.email.ne.jp, quintela@redhat.com, qemu-devel@nongnu.org, pbonzini@redhat.com, david@gibson.dropbear.id.au * 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" > > > > 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 > > --- > > 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