From mboxrd@z Thu Jan  1 00:00:00 1970
Received: from eggs.gnu.org ([2001:4830:134:3::10]:47956)
	by lists.gnu.org with esmtp (Exim 4.71)
	(envelope-from <paolo.bonzini@gmail.com>) id 1VoGRG-0003zD-Vv
	for qemu-devel@nongnu.org; Wed, 04 Dec 2013 12:40:51 -0500
Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71)
	(envelope-from <paolo.bonzini@gmail.com>) id 1VoGR8-0005YF-Bg
	for qemu-devel@nongnu.org; Wed, 04 Dec 2013 12:40:42 -0500
Received: from mail-ee0-x22b.google.com ([2a00:1450:4013:c00::22b]:48575)
	by eggs.gnu.org with esmtp (Exim 4.71)
	(envelope-from <paolo.bonzini@gmail.com>) id 1VoGR8-0005Xz-2e
	for qemu-devel@nongnu.org; Wed, 04 Dec 2013 12:40:34 -0500
Received: by mail-ee0-f43.google.com with SMTP id c13so2581020eek.16
	for <qemu-devel@nongnu.org>; Wed, 04 Dec 2013 09:40:33 -0800 (PST)
Sender: Paolo Bonzini <paolo.bonzini@gmail.com>
Message-ID: <529F690E.9020802@redhat.com>
Date: Wed, 04 Dec 2013 18:40:30 +0100
From: Paolo Bonzini <pbonzini@redhat.com>
MIME-Version: 1.0
References: <1381417639-22547-1-git-send-email-pbonzini@redhat.com>
	<1381417639-22547-4-git-send-email-pbonzini@redhat.com>
	<20131204140649.GA27759@stefanha-thinkpad.redhat.com>
In-Reply-To: <20131204140649.GA27759@stefanha-thinkpad.redhat.com>
Content-Type: text/plain; charset=ISO-8859-1
Content-Transfer-Encoding: 7bit
Subject: Re: [Qemu-devel] [PATCH 3/4] dataplane: change vring API to use
	VirtQueueElement
List-Id: <qemu-devel.nongnu.org>
List-Unsubscribe: <https://lists.nongnu.org/mailman/options/qemu-devel>,
	<mailto:qemu-devel-request@nongnu.org?subject=unsubscribe>
List-Archive: <http://lists.nongnu.org/archive/html/qemu-devel>
List-Post: <mailto:qemu-devel@nongnu.org>
List-Help: <mailto:qemu-devel-request@nongnu.org?subject=help>
List-Subscribe: <https://lists.nongnu.org/mailman/listinfo/qemu-devel>,
	<mailto:qemu-devel-request@nongnu.org?subject=subscribe>
To: Stefan Hajnoczi <stefanha@gmail.com>
Cc: qemu-devel@nongnu.org, stefanha@redhat.com

Il 04/12/2013 15:06, Stefan Hajnoczi ha scritto:
> On Thu, Oct 10, 2013 at 05:07:18PM +0200, Paolo Bonzini wrote:
>> @@ -298,30 +278,31 @@ static void handle_notify(EventNotifier *e)
>>          vring_disable_notification(s->vdev, &s->vring);
>>  
>>          for (;;) {
>> -            head = vring_pop(s->vdev, &s->vring, iov, end, &out_num, &in_num);
>> -            if (head < 0) {
>> +            ret = vring_pop(s->vdev, &s->vring, &elem);
>> +            if (ret < 0) {
>> +                assert(elem == NULL);
>>                  break; /* no more requests */
>>              }
>>  
>> -            trace_virtio_blk_data_plane_process_request(s, out_num, in_num,
>> -                                                        head);
>> +            trace_virtio_blk_data_plane_process_request(s, elem->out_num,
>> +                                                        elem->in_num, elem->index);
>>  
>> -            if (process_request(&s->ioqueue, iov, out_num, in_num, head) < 0) {
>> +            if (process_request(&s->ioqueue, elem) < 0) {
>>                  vring_set_broken(&s->vring);
>> +                vring_push(&s->vring, elem, 0);
> 
> If we give up on the vring I don't think we should push the element
> back.  It may cause the guest to panic.
> 
> I guess what we really need here is to unmap scatter-gather buffers and
> delete elem.

That's what already happens actually.  vring_push has


+    g_slice_free(VirtQueueElement, elem);
+
     /* Don't touch vring if a fatal error occurred */
     if (vring->broken) {
         return;

in this patch and

+    for (i = 0; i < elem->out_num; i++) {
+        vring_unmap(elem->out_sg[i].iov_base, false);
+    }
+
+    for (i = 0; i < elem->in_num; i++) {
+        vring_unmap(elem->in_sg[i].iov_base, true);
+    }

     g_slice_free(VirtQueueElement, elem);

in the next one.

Though I admit vring_push isn't such a great name and API.  I can add
instead a vring_free_element function.  Do you think vring_push should
call it, or should the caller do that?

Paolo