On 2024/2/6 11:08, Jason Wang wrote:
...
>> +
>> +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);
>
>> +
>> + 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.
>
>> +
>> + 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.
>
>> +
>> + /* 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().
>
>> + }
>> + printf("TX spurious wakeups: 0x%llx started=0x%lx completed=0x%lx\n",
>> + spurious, vq->started, vq->completed);
>> +}
>> +
...
>> +
>> + /* 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
>
> .
>
