Re: [Qemu-devel] [PATCH 3/4] dataplane: change vring API to use VirtQueueElement

[Paolo Bonzini <pbonzini@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