On 2024/2/4 9:30, Jason Wang wrote:

On Fri, Feb 2, 2024 at 8:24 PM Yunsheng Lin [off-list ref] wrote:

quoted

On 2024/2/2 12:05, Jason Wang wrote:

quoted

On Tue, Jan 30, 2024 at 7:38 PM Yunsheng Lin [off-list ref] wrote:

quoted

introduce vhost_net_test basing on virtio_test to test
vhost_net changing in the kernel.

Let's describe what kind of test is being done and how it is done here.

How about something like below:

This patch introduces testing for both vhost_net tx and rx.
Steps for vhost_net tx testing:
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

Steps for vhost_net rx testing::
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

It looks like some important details were lost, e.g the logic for batching etc.

I am supposeing you are referring to the virtio desc batch handling，
right?

It was a copy & paste code of virtio_test.c, I was thinking about removing
the virtio desc batch handling for now, as this patchset does not require
that to do the testing, it mainly depend on the "sock->sk->sk_sndbuf" to
be INT_MAX to call vhost_net_build_xdp(), which seems to be the default
case for vhost_net.

quoted

...

quoted

quoted

quoted

+static void vdev_create_socket(struct vdev_info *dev)
+{
+       struct ifreq ifr;
+
+       dev->sock = socket(AF_PACKET, SOCK_RAW, htons(TEST_PTYPE));
+       assert(dev->sock != -1);
+
+       snprintf(ifr.ifr_name, IFNAMSIZ, "tun_%d", getpid());

Nit: it might be better to accept the device name instead of repeating
the snprintf trick here, this would facilitate the future changes.

I am not sure I understand what did you mean by "accept the device name"
here.

The above is used to get ifindex of the tun netdevice created in
tun_alloc(), so that we can use it in vdev_send_packet() to send
a packet using the tun netdevice created in tun_alloc(). Is there
anything obvious I missed here?

I meant a const char *ifname for this function and let the caller to
pass the name.

Sure.

quoted

quoted

quoted

quoted

+
+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 &&

Drivers usually maintain a #free_slots, this can help to avoid the
trick for checking ENOSPC?

The above "(vq->started - vq->completed) < batch" seems to ensure that
the 'r' can't be '-ENOSPC'?

Well, if this is true any reason we still check ENOSPEC here?

As mentioned above, It was a copy & paste code of virtio_test.c.
Will remove 'r == -ENOSPC' checking.

quoted

We just need to ensure the batch <= desc_num,
and the 'r == -ENOSPC' checking seems to be unnecessary.

quoted

quoted

+                                           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;
+
+                               eh = (struct ether_header *)(dev->res_buf + HDR_LEN);
+
+                               /* tun netdev is up and running, ignore the
+                                * non-TEST_PTYPE packet.
+                                */

I wonder if it's better to set up some kind of qdisc to avoid the
unexpected packet here, or is it too complicated?

Yes, at least I don't know to do that yet.

For example, the blackhole qdisc?

It seems the blackhole_enqueue() just drop everything, which includes
the packet sent using the raw socket in vdev_send_packet()?

We may bypass qdisc for the raw socket in vdev_send_packet()，but it
means other caller may bypass qdisc, and even cook up a packet with
ethertype being ETH_P_LOOPBACK, there is part of the reason I added a
simple payload verification in verify_res_buf().

Thanks

.