From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([208.118.235.92]:36797) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1UPWt0-0002mX-W9 for qemu-devel@nongnu.org; Tue, 09 Apr 2013 07:38:53 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1UPWsz-0005BI-FF for qemu-devel@nongnu.org; Tue, 09 Apr 2013 07:38:50 -0400 Received: from mx1.redhat.com ([209.132.183.28]:3223) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1UPWsz-0005BC-83 for qemu-devel@nongnu.org; Tue, 09 Apr 2013 07:38:49 -0400 Received: from int-mx10.intmail.prod.int.phx2.redhat.com (int-mx10.intmail.prod.int.phx2.redhat.com [10.5.11.23]) by mx1.redhat.com (8.14.4/8.14.4) with ESMTP id r39Bcmsr023095 (version=TLSv1/SSLv3 cipher=DHE-RSA-AES256-SHA bits=256 verify=OK) for ; Tue, 9 Apr 2013 07:38:48 -0400 From: Juan Quintela In-Reply-To: <1365420597-5506-3-git-send-email-pbonzini@redhat.com> (Paolo Bonzini's message of "Mon, 8 Apr 2013 13:29:55 +0200") References: <1365420597-5506-1-git-send-email-pbonzini@redhat.com> <1365420597-5506-3-git-send-email-pbonzini@redhat.com> Date: Tue, 09 Apr 2013 13:38:55 +0200 Message-ID: <87d2u4djcg.fsf@elfo.elfo> MIME-Version: 1.0 Content-Type: text/plain Subject: Re: [Qemu-devel] [PATCH 2/4] migration: use a single I/O operation when writev_buffer is not defined Reply-To: quintela@redhat.com List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Paolo Bonzini Cc: kwolf@redhat.com, owasserm@redhat.com, qemu-devel@nongnu.org Paolo Bonzini wrote: > The recent patches to use vectored I/O for RAM migration caused a > regression in savevm speed. To restore previous performance, > add data to the buffer in qemu_put_buffer_async whenever writev_buffer > is not available in the QEMUFile. > > Signed-off-by: Paolo Bonzini > --- > savevm.c | 49 ++++++++++++++++++++++++++++++++++--------------- > 1 file changed, 34 insertions(+), 15 deletions(-) > > diff --git a/savevm.c b/savevm.c > index c952c41..b32e0a7 100644 > --- a/savevm.c > +++ b/savevm.c > @@ -525,27 +525,24 @@ static void qemu_file_set_error(QEMUFile *f, int ret) > static void qemu_fflush(QEMUFile *f) > { > ssize_t ret = 0; > - int i = 0; > > if (!f->ops->writev_buffer && !f->ops->put_buffer) { > return; > } > > - if (f->is_write && f->iovcnt > 0) { > + if (f->is_write) { > if (f->ops->writev_buffer) { > - ret = f->ops->writev_buffer(f->opaque, f->iov, f->iovcnt); > - if (ret >= 0) { > - f->pos += ret; Code was there, and it is a case of of style, but adding 0 to one ammount is a NOP. > + if (f->iovcnt > 0) { > + ret = f->ops->writev_buffer(f->opaque, f->iov, f->iovcnt); > } > } else { > - for (i = 0; i < f->iovcnt && ret >= 0; i++) { > - ret = f->ops->put_buffer(f->opaque, f->iov[i].iov_base, f->pos, > - f->iov[i].iov_len); > - if (ret >= 0) { > - f->pos += ret; > - } > + if (f->buf_index > 0) { > + ret = f->ops->put_buffer(f->opaque, f->buf, f->pos, f->buf_index); > } > } > + if (ret >= 0) { > + f->pos += ret; > + } > f->buf_index = 0; > f->iovcnt = 0; > } > @@ -640,6 +637,11 @@ static void add_to_iovec(QEMUFile *f, const uint8_t *buf, int size) > > void qemu_put_buffer_async(QEMUFile *f, const uint8_t *buf, int size) > { > + if (!f->ops->writev_buffer) { > + qemu_put_buffer(f, buf, size); > + return; > + } > + This > if (f->last_error) { > return; > } > @@ -673,8 +675,17 @@ 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->buf_index += l; > - qemu_put_buffer_async(f, f->buf + (f->buf_index - l), l); > + f->bytes_xfer += size; > + if (f->ops->writev_buffer) { > + add_to_iovec(f, f->buf + f->buf_index, l); > + f->buf_index += l; > + } else { > + f->is_write = 1; > + f->buf_index += l; > + if (f->buf_index == IO_BUF_SIZE) { > + qemu_fflush(f); > + } > + } And this > if (qemu_file_get_error(f)) { > break; > } > @@ -697,8 +708,16 @@ 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); > - f->buf_index++; > + if (f->ops->writev_buffer) { > + add_to_iovec(f, f->buf + f->buf_index, 1); > + f->buf_index++; > + } else { > + f->is_write = 1; > + f->buf_index++; > + if (f->buf_index == IO_BUF_SIZE) { > + qemu_fflush(f); > + } > + } And this is not needed if we just put the code inside add_to_iovec() no? Users of add_to_iovec() should not care if we are using vritev or put_buffer() right. and as a second note. Having qemu_put_buffer_async() calling qemu_put_buffer() and qemu_put_buffer() calling qemu_put_buffer_async() is ..... weird? > } > > static void qemu_file_skip(QEMUFile *f, int size)