On Fri, Sep 21, 2018 at 08:26:58PM +0800, Tiwei Bie wrote:

On Fri, Sep 21, 2018 at 12:33:03PM +0200, Jens Freimann wrote:
[...]

quoted

 static inline int
-desc_is_used(struct vring_desc_packed *desc, struct vring *vr)
+_desc_is_used(struct vring_desc_packed *desc)
 {
 	uint16_t used, avail;

 	used = !!(desc->flags & VRING_DESC_F_USED(1));
 	avail = !!(desc->flags & VRING_DESC_F_AVAIL(1));

-	return used == avail && used == vr->used_wrap_counter;
+	return used == avail;
+
+}
+
+static inline int
+desc_is_used(struct vring_desc_packed *desc, struct vring *vr)
+{
+	uint16_t used;
+
+	used = !!(desc->flags & VRING_DESC_F_USED(1));
+
+	return _desc_is_used(desc) && used == vr->used_wrap_counter;
 }

 /* The standard layout for the ring is a continuous chunk of memory which

diff --git a/drivers/net/virtio/virtio_rxtx.c b/drivers/net/virtio/virtio_rxtx.c
index eb891433e..ea6300563 100644
--- a/drivers/net/virtio/virtio_rxtx.c
+++ b/drivers/net/virtio/virtio_rxtx.c

@@ -38,6 +38,7 @@
 #define  VIRTIO_DUMP_PACKET(m, len) do { } while (0)
 #endif

+
 int
 virtio_dev_rx_queue_done(void *rxq, uint16_t offset)
 {

@@ -165,6 +166,31 @@ virtqueue_dequeue_rx_inorder(struct virtqueue *vq,
 #endif

 /* Cleanup from completed transmits. */
+static void
+virtio_xmit_cleanup_packed(struct virtqueue *vq)
+{
+	uint16_t idx;
+	uint16_t size = vq->vq_nentries;
+	struct vring_desc_packed *desc = vq->vq_ring.desc_packed;
+	struct vq_desc_extra *dxp;
+
+	idx = vq->vq_used_cons_idx;
+	while (_desc_is_used(&desc[idx]) &&

We can't just compare the AVAIL bit and USED bit to
check whether a desc is used.

We can't compare with the current wrap counter value as well
because it won't match the flags in descriptors. So check against
used_wrap_counter ^= 1 then?

quoted

+	       vq->vq_free_cnt < size) {
+		dxp = &vq->vq_descx[idx];

The code is still assuming the descs will be written
back by device in order. The vq->vq_descx[] needs to
be managed e.g. as a list to support the out-of-order
processing. IOW, we can't assume vq->vq_descx[idx]
is corresponding to desc[idx] when device may write
back the descs out of order.

I changed it to not assume this in other spots but missed this one.  I
will check more carefully and add code to make vq_descx entries a list.

Thanks for the review!

regards,
Jens