Re: [PATCH net-next 6/6] tools: virtio: introduce vhost_net_test
From: Yunsheng Lin
Date: Thu Jan 04 2024 - 21:09:40 EST
On 2024/1/5 0:17, Eugenio Perez Martin wrote:
> On Wed, Jan 3, 2024 at 11:00 AM Yunsheng Lin <linyunsheng@xxxxxxxxxx> wrote:
...
>> +
>> +static void run_tx_test(struct vdev_info *dev, struct vq_info *vq,
>> + bool delayed, int batch, int bufs)
>> +{
>> + const bool random_batch = batch == RANDOM_BATCH;
>> + 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 {
>> + if (random_batch)
>> + batch = (random() % vq->vring.num) + 1;
>> +
>> + while (vq->started < bufs &&
>> + (vq->started - vq->completed) < batch) {
>> + 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)) {
>> + if (r == -ENOSPC &&
>> + vq->started > started_before)
>> + r = 0;
>> + else
>> + r = -1;
>> + break;
>> + }
>> +
>> + ++vq->started;
>> +
>> + if (unlikely(!virtqueue_kick(vq->vq))) {
>> + r = -1;
>> + break;
>> + }
>> + }
>> +
>> + if (vq->started >= bufs)
>> + r = -1;
>> +
>> + /* Flush out completed bufs if any */
>> + while (virtqueue_get_buf(vq->vq, &len)) {
>> + int n, i;
>> +
>> + n = recvfrom(dev->sock, dev->res_buf, TEST_BUF_LEN, 0, NULL, NULL);
>> + assert(n == TEST_BUF_LEN);
>> +
>> + for (i = ETHER_HDR_LEN; i < n; i++)
>> + assert(dev->res_buf[i] == (char)i);
>> +
>> + ++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);
>> + }
>> + }
>> + printf("TX spurious wakeups: 0x%llx started=0x%lx completed=0x%lx\n",
>> + spurious, vq->started, vq->completed);
>> +}
>> +
>> +static void run_rx_test(struct vdev_info *dev, struct vq_info *vq,
>> + bool delayed, int batch, int bufs)
>> +{
>> + const bool random_batch = batch == RANDOM_BATCH;
>> + long long spurious = 0;
>> + struct scatterlist sl;
>> + unsigned int len;
>> + int r;
>> +
>> + for (;;) {
>> + long started_before = vq->started;
>> + long completed_before = vq->completed;
>> +
>> + do {
>> + if (random_batch)
>> + batch = (random() % vq->vring.num) + 1;
>> +
>> + while (vq->started < bufs &&
>> + (vq->started - vq->completed) < batch) {
>> + sg_init_one(&sl, dev->res_buf, HDR_LEN + TEST_BUF_LEN);
>> +
>> + r = virtqueue_add_inbuf(vq->vq, &sl, 1,
>> + dev->res_buf + vq->started,
>> + GFP_ATOMIC);
>> + if (unlikely(r != 0)) {
>> + if (r == -ENOSPC &&
>> + vq->started > started_before)
>> + r = 0;
>> + else
>> + r = -1;
>> + break;
>> + }
>> +
>> + ++vq->started;
>> +
>> + vdev_send_packet(dev);
>> +
>> + if (unlikely(!virtqueue_kick(vq->vq))) {
>> + r = -1;
>> + break;
>> + }
>> + }
>> +
>> + if (vq->started >= bufs)
>> + r = -1;
>> +
>> + /* Flush out completed bufs if any */
>> + while (virtqueue_get_buf(vq->vq, &len)) {
>> + struct ether_header *eh;
>> + int i;
>> +
>> + 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);
>> +
>> + for (i = ETHER_HDR_LEN; i < TEST_BUF_LEN; i++)
>> + assert(dev->res_buf[i + HDR_LEN] == (char)i);
>> +
>> + ++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;
>> + }
>> +
>> + printf("RX spurious wakeups: 0x%llx started=0x%lx completed=0x%lx\n",
>> + spurious, vq->started, vq->completed);
>> +}
>
> Hi!
>
> I think the tests are great! Maybe both run_rx_test and run_tx_test
> (and virtio_test.c:run_test) can be merged into a common function with
> some callbacks? Not sure if it will complicate the code more.
I looked at it more closely, it seems using callback just make the code
more complicated without obvious benefit, as the main steps is different
for tx and rx.
For tx:
1. Prepare a out buf
2. Kick the vhost_net to do tx processing
3. Do the receiving in the tun side
4. verify the data received by tun is correct
For rx:
1. Prepare a in buf
2. Do the sending in the tun side
3. Kick the vhost_net to do rx processing
4. verify the data received by vhost_net is correct
I could refactor out a common function to do the data verifying for step 4
of both tx and rx.
For virtio_test.c:run_test, it does not seems to be using the vhost_net directly,
it uses shim vhost_test.ko module, and it seems to only have step 1 & 3 of
vhost_net_test' rx testing. The new vhost_net_test should be able to replace the
virtio_test, I thought about deleting the virtio_test after adding the vhost_net_test,
but I am not familiar enough with virtio to do the judgement yet.
>
> Thanks!
>