Received-SPF: pass (google.com: domain of linux-kernel+bounces-54414-linux.lists.archive=gmail.com@vger.kernel.org designates 2604:1380:4601:e00::3 as permitted sender) client-ip=2604:1380:4601:e00::3;
Subject: Re: [PATCH net-next v5 5/5] tools: virtio: introduce vhost_net_test
To: Jason Wang <jasowang@redhat.com>
CC: <davem@davemloft.net>, <kuba@kernel.org>, <pabeni@redhat.com>,
	<netdev@vger.kernel.org>, <linux-kernel@vger.kernel.org>, "Michael S.
 Tsirkin" <mst@redhat.com>, Xuan Zhuo <xuanzhuo@linux.alibaba.com>,
	<virtualization@lists.linux.dev>
References: <20240205124506.57670-1-linyunsheng@huawei.com>
 <20240205124506.57670-6-linyunsheng@huawei.com>
 <CACGkMEsKHOefArPd56RAYPsJE8kf=jGb6B-V6eNJiViCAD7GYA@mail.gmail.com>
From: Yunsheng Lin <linyunsheng@huawei.com>
Message-ID: <7416cf5f-b6dc-285f-6acc-f437dcb74a42@huawei.com>
Date: Tue, 6 Feb 2024 15:23:57 +0800
User-Agent: Mozilla/5.0 (Windows NT 10.0; WOW64; rv:52.0) Gecko/20100101
 Thunderbird/52.2.0
Precedence: bulk
MIME-Version: 1.0
In-Reply-To: <CACGkMEsKHOefArPd56RAYPsJE8kf=jGb6B-V6eNJiViCAD7GYA@mail.gmail.com>
Content-Type: text/plain; charset="utf-8"
Content-Language: en-US
Content-Transfer-Encoding: 7bit

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
> 
> .
>