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