Re: [PATCH net-next v4 5/5] tools: virtio: introduce vhost_net_test

[Jason Wang]

On Sun, Feb 4, 2024 at 11:50 AM Yunsheng Lin <linyunsheng@xxxxxxxxxx> wrote:
>
> On 2024/2/4 9:30, Jason Wang wrote:
> > On Fri, Feb 2, 2024 at 8:24 PM Yunsheng Lin <linyunsheng@huaweicom> wrote:
> >>
> >> On 2024/2/2 12:05, Jason Wang wrote:
> >>> On Tue, Jan 30, 2024 at 7:38 PM Yunsheng Lin <linyunsheng@xxxxxxxxxx> wrote:
> >>>>
> >>>> 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.

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

>
> >
> >> We just need to ensure the batch <= desc_num,
> >> and the 'r == -ENOSPC' checking seems to be unnecessary.
> >>
> >>>
> >>>> +                                           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

>
> >
> > Thanks
> >
> > .
> >
>