From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:42157) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1YvKbl-0006ob-WC for qemu-devel@nongnu.org; Thu, 21 May 2015 03:09:35 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1YvKbh-0005O8-VB for qemu-devel@nongnu.org; Thu, 21 May 2015 03:09:33 -0400 Received: from mx1.redhat.com ([209.132.183.28]:46083) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1YvKbh-0005O1-O6 for qemu-devel@nongnu.org; Thu, 21 May 2015 03:09:29 -0400 Date: Thu, 21 May 2015 12:39:01 +0530 From: Amit Shah Message-ID: <20150521070901.GM15452@grmbl.mre> References: <1429031053-4454-1-git-send-email-dgilbert@redhat.com> <1429031053-4454-9-git-send-email-dgilbert@redhat.com> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: <1429031053-4454-9-git-send-email-dgilbert@redhat.com> 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: "Dr. David Alan Gilbert (git)" Cc: aarcange@redhat.com, yamahata@private.email.ne.jp, quintela@redhat.com, qemu-devel@nongnu.org, pbonzini@redhat.com, david@gibson.dropbear.id.au, yayanghy@cn.fujitsu.com 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. > + * 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. > + return done; How about just 'break' instead of return? > + } else { > + first = false; > + memcpy(buf, src, res); > + buf += res; In either case (break or return), the 'else' can be dropped.. Amit