On Sun, Feb 4, 2024 at 11:50 AM Yunsheng Lin [off-list ref] wrote:

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

quoted

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?

Yes.

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.

Ok.

quoted

quoted

...

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

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.

I see.

quoted

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()?

I vaguely remember there's a mode that af_packet will bypass the qdisc
but I might be wrong.

But rethink of this, though it facilitates the code but it introduces
unnecessary dependencies like black hole which seems to be suboptimal.

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

Fine.

Thanks

quoted

Thanks

.