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

[Yunsheng Lin]

On 2024/2/4 9:30, Jason Wang wrote:
> On Fri, Feb 2, 2024 at 8:24 PM Yunsheng Lin <linyunsheng@xxxxxxxxxx> 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?

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.

> 
>>

..

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

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

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