Re: [Qemu-devel] [PATCH v4 09/11] virtio-net: update the head descriptor in a chain lastly

[Wei Xu]

On Wed, Feb 20, 2019 at 10:34:32AM +0800, Jason Wang wrote:
> 
> On 2019/2/20 上午9:54, Wei Xu wrote:
> >On Tue, Feb 19, 2019 at 09:09:33PM +0800, Jason Wang wrote:
> >>On 2019/2/19 下午6:51, Wei Xu wrote:
> >>>On Tue, Feb 19, 2019 at 03:23:01PM +0800, Jason Wang wrote:
> >>>>On 2019/2/14 下午12:26, address@hidden wrote:
> >>>>>From: Wei Xu <address@hidden>
> >>>>>
> >>>>>This is a helper for packed ring.
> >>>>>
> >>>>>To support packed ring, the head descriptor in a chain should be updated
> >>>>>lastly since no 'avail_idx' like in packed ring to explicitly tell the
> >>>>>driver side that all payload is ready after having done the chain, so
> >>>>>the head is always visible immediately.
> >>>>>
> >>>>>This patch fills the header after done all the other ones.
> >>>>>
> >>>>>Signed-off-by: Wei Xu <address@hidden>
> >>>>It's really odd to workaround API issue in the implementation of device.
> >>>>Please introduce batched used updating helpers instead.
> >>>Can you elaborate a bit more? I don't get it as well.
> >>>
> >>>The exact batch as vhost-net or dpdk pmd is not supported by userspace
> >>>backend. The change here is to keep the header descriptor updated at
> >>>last in case of a chaining descriptors and the helper might not help
> >>>too much.
> >>>
> >>>Wei
> >>
> >>Of course we can add batching support why not?
> >It is always good to improve performance with anything, while probably
> >this could be done in another separate batch, also we need to bear
> >in mind that usually qemu userspace backend is not the first option for
> >performance oriented user.
> 
> 
> The point is to hide layout specific things from device emulation. If it
> helps for performance, it could be treated as a good byproduct.
> 
> 
> >
> >AFAICT, virtqueue_fill() is a generic API for all relevant userspace virtio
> >devices that do not support batching , without touching virtqueue_fill(),
> >supporting batching changes the meaning of the parameter 'idx' which should
> >be kept overall.
> >
> >To fix it, I got two proposals so far:
> >1). batching support(two APIs needed to keep compatibility)
> >2). save a head elem for a vq instead of caching an array of elems like 
> >vhost,
> >     and introduce a new API(virtqueue_chain_fill()) functioning with an
> >     additional parameter 'more' to the current virtqueue_fill() to indicate 
> > if
> >     there are more descriptor(s) coming in a chain.
> >
> >Either way it changes the API somehow and it does not seem to be clean and 
> >clear
> >as wanted.
> 
> 
> It's as simple as accepting an array of elems in e.g
> virtqueue_fill_batched()?

It is trivial for both, my concern is an array elements need to be allocated 
dynamically
due to vq size which no any other devices are using, a head would be enough for 
2.

> 
> 
> >
> >Any better idea?
> >
> >>Your code assumes the device know the virtio layout specific assumption
> >>whih breaks the layer. Device should not care about the actual layout.
> >>
> >Good point, but anyway, change to virtio-net receiving code path is
> >unavoidable to support split and packed rings, and batching is like a new
> >feature somehow.
> 
> 
> It's ok to change the code as a result of introducing of generic helper but
> it's bad to change to code for working around a bad API.

Agree.

Wei

> 
> Thanks
> 
> 
> >
> >Wei
> >>Thanks
> >>
> >>
> >>>>Thanks
> >>>>
> >>>>
> >>>>>---
> >>>>>  hw/net/virtio-net.c | 11 ++++++++++-
> >>>>>  1 file changed, 10 insertions(+), 1 deletion(-)
> >>>>>
> >>>>>diff --git a/hw/net/virtio-net.c b/hw/net/virtio-net.c
> >>>>>index 3f319ef..330abea 100644
> >>>>>--- a/hw/net/virtio-net.c
> >>>>>+++ b/hw/net/virtio-net.c
> >>>>>@@ -1251,6 +1251,8 @@ static ssize_t 
> >>>>>virtio_net_receive_rcu(NetClientState *nc, const uint8_t *buf,
> >>>>>      struct virtio_net_hdr_mrg_rxbuf mhdr;
> >>>>>      unsigned mhdr_cnt = 0;
> >>>>>      size_t offset, i, guest_offset;
> >>>>>+    VirtQueueElement head;
> >>>>>+    int head_len = 0;
> >>>>>      if (!virtio_net_can_receive(nc)) {
> >>>>>          return -1;
> >>>>>@@ -1328,7 +1330,13 @@ static ssize_t 
> >>>>>virtio_net_receive_rcu(NetClientState *nc, const uint8_t *buf,
> >>>>>          }
> >>>>>          /* signal other side */
> >>>>>-        virtqueue_fill(q->rx_vq, elem, total, i++);
> >>>>>+        if (i == 0) {
> >>>>>+            head_len = total;
> >>>>>+            head = *elem;
> >>>>>+        } else {
> >>>>>+            virtqueue_fill(q->rx_vq, elem, len, i);
> >>>>>+        }
> >>>>>+        i++;
> >>>>>          g_free(elem);
> >>>>>      }
> >>>>>@@ -1339,6 +1347,7 @@ static ssize_t 
> >>>>>virtio_net_receive_rcu(NetClientState *nc, const uint8_t *buf,
> >>>>>                       &mhdr.num_buffers, sizeof mhdr.num_buffers);
> >>>>>      }
> >>>>>+    virtqueue_fill(q->rx_vq, &head, head_len, 0);
> >>>>>      virtqueue_flush(q->rx_vq, i);
> >>>>>      virtio_notify(vdev, q->rx_vq);
>