On 2024/2/6 11:08, Jason Wang wrote:

...

quoted

+
+static void wait_for_interrupt(struct vq_info *vq)
+{
+       unsigned long long val;
+
+       poll(&vq->fds, 1, -1);

It's not good to wait indefinitely.

How about a timeout value of 100ms as below?
poll(&vq->fds, 1, 100);

quoted

+
+       if (vq->fds.revents & POLLIN)
+               read(vq->fds.fd, &val, sizeof(val));
+}
+
+static void verify_res_buf(char *res_buf)
+{
+       int i;
+
+       for (i = ETHER_HDR_LEN; i < TEST_BUF_LEN; i++)
+               assert(res_buf[i] == (char)i);
+}
+
+static void run_tx_test(struct vdev_info *dev, struct vq_info *vq,
+                       bool delayed, int bufs)
+{
+       long long spurious = 0;
+       struct scatterlist sl;
+       unsigned int len;
+       int r;
+
+       for (;;) {
+               long started_before = vq->started;
+               long completed_before = vq->completed;
+
+               virtqueue_disable_cb(vq->vq);
+               do {
+                       while (vq->started < bufs &&
+                              (vq->started - vq->completed) < 1) {
+                               sg_init_one(&sl, dev->test_buf, HDR_LEN + TEST_BUF_LEN);
+                               r = virtqueue_add_outbuf(vq->vq, &sl, 1,
+                                                        dev->test_buf + vq->started,
+                                                        GFP_ATOMIC);
+                               if (unlikely(r != 0))
+                                       break;
+
+                               ++vq->started;

If we never decrease started/completed shouldn't we use unsigned here?
(as well as completed)

Otherwise we may get unexpected results for vq->started as well as
vq->completed.

We have "vq->started < bufs" checking before the increasing as above,
and there is 'assert(nbufs > 0)' when getting optarg in main(), which
means we never allow started/completed to be greater than nbufs as
my understanding.

quoted

+
+                               if (unlikely(!virtqueue_kick(vq->vq))) {
+                                       r = -1;
+                                       break;
+                               }
+                       }
+
+                       if (vq->started >= bufs)
+                               r = -1;

Which condition do we reach here?

It is also a copy & paste of virtio_test.c
It means we have finished adding the outbuf in virtqueue, and set 'r'
to be '-1' so that we can break the inner while loop if there is no
result for virtqueue_get_buf() as my understanding.

quoted

+
+                       /* Flush out completed bufs if any */
+                       while (virtqueue_get_buf(vq->vq, &len)) {
+                               int n;
+
+                               n = recvfrom(dev->sock, dev->res_buf, TEST_BUF_LEN, 0, NULL, NULL);
+                               assert(n == TEST_BUF_LEN);
+                               verify_res_buf(dev->res_buf);
+
+                               ++vq->completed;
+                               r = 0;
+                       }
+               } while (r == 0);
+
+               if (vq->completed == completed_before && vq->started == started_before)
+                       ++spurious;
+
+               assert(vq->completed <= bufs);
+               assert(vq->started <= bufs);
+               if (vq->completed == bufs)
+                       break;
+
+               if (delayed) {
+                       if (virtqueue_enable_cb_delayed(vq->vq))
+                               wait_for_interrupt(vq);
+               } else {
+                       if (virtqueue_enable_cb(vq->vq))
+                               wait_for_interrupt(vq);
+               }

This could be simplified with

if (delayed)
else

wait_for_interrupt(vq)

I am not sure if I understand the above comment.
The wait_for_interrupt() is only called conditionally depending on the
returning of virtqueue_enable_cb_delayed() and virtqueue_enable_cb().

quoted

+       }
+       printf("TX spurious wakeups: 0x%llx started=0x%lx completed=0x%lx\n",
+              spurious, vq->started, vq->completed);
+}
+

...

quoted

+
+                       /* Flush out completed bufs if any */
+                       while (virtqueue_get_buf(vq->vq, &len)) {
+                               struct ether_header *eh;
+
+                               eh = (struct ether_header *)(dev->res_buf + HDR_LEN);
+
+                               /* tun netdev is up and running, ignore the
+                                * non-TEST_PTYPE packet.
+                                */
+                               if (eh->ether_type != htons(TEST_PTYPE)) {
+                                       ++vq->completed;
+                                       r = 0;
+                                       continue;
+                               }
+
+                               assert(len == TEST_BUF_LEN + HDR_LEN);
+                               verify_res_buf(dev->res_buf + HDR_LEN);

Let's simply the logic here:

if (ether_type == htons()) {
    assert()
    verify_res_buf()
}

r = 0;
++vq->completed;

Sure.

Others look good.

Thanks for the reviewing.

Thanks

.