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

[Stefan Hajnoczi]

On Wed, Dec 04, 2013 at 06:40:30PM +0100, Paolo Bonzini wrote:
> 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?

I think vring_push() should free the VirtQueueElement.

We just need to expose vring_free_element() so that handle_notify() can
call it without pushing bogus buffers back to the guest.

Stefan