Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 23.128.96.33 as permitted sender) client-ip=23.128.96.33;
MIME-Version: 1.0
References: <20231205113444.63015-1-linyunsheng@huawei.com>
 <20231205113444.63015-7-linyunsheng@huawei.com> <CACGkMEvVezZnHK-gRWY+MUd_6awnprb024scqPNmMQ05P8rWTQ@mail.gmail.com>
 <424670ab-23d8-663b-10cb-d88906767956@huawei.com>
In-Reply-To: <424670ab-23d8-663b-10cb-d88906767956@huawei.com>
From:   Jason Wang <jasowang@redhat.com>
Date:   Tue, 12 Dec 2023 12:35:04 +0800
Message-ID: <CACGkMEsMdP1B-9RaqibJYfFsd_qJpB+Kta5BnyD_WXH=W2w_OQ@mail.gmail.com>
Subject: Re: [PATCH net-next 6/6] tools: virtio: introduce vhost_net_test
To:     Yunsheng Lin <linyunsheng@huawei.com>
Cc:     davem@davemloft.net, kuba@kernel.org, pabeni@redhat.com,
        netdev@vger.kernel.org, linux-kernel@vger.kernel.org,
        "Michael S. Tsirkin" <mst@redhat.com>,
        Xuan Zhuo <xuanzhuo@linux.alibaba.com>,
        virtualization@lists.linux.dev
Content-Type: text/plain; charset="UTF-8"
Content-Transfer-Encoding: quoted-printable
Precedence: bulk

On Thu, Dec 7, 2023 at 7:28=E2=80=AFPM Yunsheng Lin <linyunsheng@huawei.com=
> wrote:
>
> On 2023/12/7 14:00, Jason Wang wrote:
> > On Tue, Dec 5, 2023 at 7:35=E2=80=AFPM Yunsheng Lin <linyunsheng@huawei=
.com> wrote:
> ...
>
> >> +
> >> +static int tun_alloc(void)
> >> +{
> >> +       struct ifreq ifr;
> >> +       int fd, e;
> >> +
> >> +       fd =3D open("/dev/net/tun", O_RDWR);
> >> +       if (fd < 0) {
> >> +               perror("Cannot open /dev/net/tun");
> >> +               return fd;
> >> +       }
> >> +
> >> +       memset(&ifr, 0, sizeof(ifr));
> >> +
> >> +       ifr.ifr_flags =3D IFF_TUN | IFF_NO_PI;
> >
> > Why did you use IFF_TUN but not IFF_TAP here?
>
> To be honest, no particular reason, I just picked IFF_TUN and it happened
> to work for me to test changing in vhost_net_build_xdp().
>
> Is there a particular reason you perfer the IFF_TAP over IFF_TUN?

No preference here. It only matters if you want to test L2 or L3.


>
> >
> >> +       strncpy(ifr.ifr_name, "tun0", IFNAMSIZ);
> >
> > tun0 is pretty common if there's a VPN. Do we need some randomized name=
 here?
>
> How about something like below?
>
> snprintf(ifr.ifr_name, IFNAMSIZ, "tun_%d", getpid());
>
> >
> >
> >> +
> >> +       e =3D ioctl(fd, TUNSETIFF, (void *) &ifr);
> >> +       if (e < 0) {
> >> +               perror("ioctl[TUNSETIFF]");
> >> +               close(fd);
> >> +               return e;
> >> +       }
> >> +
> >> +       return fd;
> >> +}
> >> +
> >> +/* Unused */
> >> +void *__kmalloc_fake, *__kfree_ignore_start, *__kfree_ignore_end;
> >
> > Why do we need trick like these here?
>
> That is because of the below error:
> tools/virtio/./linux/kernel.h:58: undefined reference to `__kmalloc_fake'
>
> when virtio_ring.c is compiled in the userspace, the kmalloc raleted func=
tion
> is implemented in tools/virtio/./linux/kernel.h, which requires those var=
ibles
> to be defined.
>
> >
> >> +
> >> +struct vq_info {
> >> +       int kick;
> >> +       int call;
> >> +       int num;
> >> +       int idx;
> >> +       void *ring;
> >> +       /* copy used for control */
> >> +       struct vring vring;
> >> +       struct virtqueue *vq;
> >> +};
> >> +
> >> +struct vdev_info {
> >> +       struct virtio_device vdev;
> >> +       int control;
> >> +       struct pollfd fds[1];
> >> +       struct vq_info vqs[1];
> >> +       int nvqs;
> >> +       void *buf;
> >> +       size_t buf_size;
> >> +       struct vhost_memory *mem;
> >> +};
> >> +
> >> +static struct vhost_vring_file no_backend =3D { .index =3D 1, .fd =3D=
 -1 },
> >> +                                    backend =3D { .index =3D 1, .fd =
=3D 1 };
> >
> > A magic number like fd =3D 1 is pretty confusing.
> >
> > And I don't see why we need global variables here.
>
> I was using the virtio_test.c as reference, will try to remove it
> if it is possible.
>
> >
> >> +static const struct vhost_vring_state null_state =3D {};
> >> +
>
> ..
>
> >> +
> >> +done:
> >> +       backend.fd =3D tun_alloc();
> >> +       assert(backend.fd >=3D 0);
> >> +       vdev_info_init(&dev, features);
> >> +       vq_info_add(&dev, 256);
> >> +       run_test(&dev, &dev.vqs[0], delayed, batch, reset, nbufs);
> >
> > I'd expect we are testing some basic traffic here. E.g can we use a
> > packet socket then we can test both tx and rx?
>
> Yes, only rx for tun is tested.
> Do you have an idea how to test the tx too? As I am not familar enough
> with vhost_net and tun yet.

Maybe you can have a packet socket to bind to the tun/tap. Then you can tes=
t:

1) TAP RX: by write a packet via virtqueue through vhost_net and read
it from packet socket
2) TAP TX:  by write via packet socket and read it from the virtqueue
through vhost_net

Thanks

>
> >
> > Thanks
>