Message ID | 20230419134329.346825-1-maxime.coquelin@redhat.com |
---|---|
Headers |
Return-Path: <linux-kernel-owner@vger.kernel.org> Delivered-To: ouuuleilei@gmail.com Received: by 2002:a59:b0ea:0:b0:3b6:4342:cba0 with SMTP id b10csp407219vqo; Wed, 19 Apr 2023 06:54:39 -0700 (PDT) X-Google-Smtp-Source: AKy350YGZD8l3RGHYrw6QPqM4qj/zN0DBMC8RmvHxP/AxvBv9b3j3UvW6l/pw1/R547NcCRBYkoM X-Received: by 2002:a17:903:50f:b0:1a2:8871:b408 with SMTP id jn15-20020a170903050f00b001a28871b408mr4970154plb.36.1681912479288; Wed, 19 Apr 2023 06:54:39 -0700 (PDT) ARC-Seal: i=1; a=rsa-sha256; t=1681912479; cv=none; d=google.com; s=arc-20160816; b=fm0lsSc+sxu+MHHkSRtvzPpb+pacnxfNjZzmN7UY+QOGeJJSnUR0FuKuEP53IQe6dC u8LMDsnIdBtb+CR/vAoNcxP9vdcjqrwvtCgo6qxwJdw++o3YA9yYYaubI3fI3pcOpna7 AIqVqA3B1jtYktG7A1YtEHol0tA2O4n7KqtFpEtlBduQx1T47x/ALo55NAfkLpb6qbR9 1y4S04Y/lC89PYQJwZ7xZzxlCvc6vLkU0vchHbjR9KyMMM0W5cQ9g4egK9Prk9yXCHXe eBtHxslsIHTIaZBb1AY4JWJa0t7RDLimtC0jDmMv12wsnoKix3Xo8P/wd9hTp94yD2+O ot/Q== ARC-Message-Signature: i=1; a=rsa-sha256; c=relaxed/relaxed; d=google.com; s=arc-20160816; h=list-id:precedence:content-transfer-encoding:mime-version :message-id:date:subject:cc:to:from:dkim-signature; bh=MK0UGxGV6JsBO8AYL7soTcS9wXMNV0whUDQXJnSnGMA=; b=FdqkSrjIsl7uIcFGmJ/Yf/l1yzddV+Qt1zItB+GAyH8L3RR2i/kk7vZWYQgrxitsz1 0kmRzEWi5kyc3aP2gbYf/3gP7MNFV79QKBBofJG3u8e+dEfXaPGNHuh7tms0qQRRVMjw cyus0Sgu4TSlLrPGUlajOH9MFJ79t606dVmuh+1PtfGlW6B27lzml1zTodX6O4jDmw+3 CVnnsMQdz3iS8uMXWFCSrwC7CsK31s6I/c2EYQBPwx/YxKT1MKrRp7uFeSk2RrJcitG6 RsLGH/jLEAXyd6WEdqyuGRsNntb6v67yRHnpYZ2lsErMmmVHDvY/1uOcglwbopJojJL8 4JUw== ARC-Authentication-Results: i=1; mx.google.com; dkim=pass header.i=@redhat.com header.s=mimecast20190719 header.b="E/Z1giyb"; spf=pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) smtp.mailfrom=linux-kernel-owner@vger.kernel.org; dmarc=pass (p=NONE sp=NONE dis=NONE) header.from=redhat.com Received: from out1.vger.email (out1.vger.email. [2620:137:e000::1:20]) by mx.google.com with ESMTP id p18-20020a170902ead200b001a1af4f6089si8042794pld.155.2023.04.19.06.54.27; Wed, 19 Apr 2023 06:54:39 -0700 (PDT) Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) client-ip=2620:137:e000::1:20; Authentication-Results: mx.google.com; dkim=pass header.i=@redhat.com header.s=mimecast20190719 header.b="E/Z1giyb"; spf=pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) smtp.mailfrom=linux-kernel-owner@vger.kernel.org; dmarc=pass (p=NONE sp=NONE dis=NONE) header.from=redhat.com Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S233325AbjDSNol (ORCPT <rfc822;peter110.wang@gmail.com> + 99 others); Wed, 19 Apr 2023 09:44:41 -0400 Received: from lindbergh.monkeyblade.net ([23.128.96.19]:50370 "EHLO lindbergh.monkeyblade.net" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S233169AbjDSNoj (ORCPT <rfc822;linux-kernel@vger.kernel.org>); Wed, 19 Apr 2023 09:44:39 -0400 Received: from us-smtp-delivery-124.mimecast.com (us-smtp-delivery-124.mimecast.com [170.10.133.124]) by lindbergh.monkeyblade.net (Postfix) with ESMTPS id 3BF5015606 for <linux-kernel@vger.kernel.org>; Wed, 19 Apr 2023 06:43:54 -0700 (PDT) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=redhat.com; s=mimecast20190719; t=1681911833; h=from:from:reply-to:subject:subject:date:date:message-id:message-id: to:to:cc:cc:mime-version:mime-version: content-transfer-encoding:content-transfer-encoding; bh=MK0UGxGV6JsBO8AYL7soTcS9wXMNV0whUDQXJnSnGMA=; b=E/Z1giyboWmWIUuHyi0+9SARPUavcONUdUx+9/04iV80Lr9ZQg2TLCdUxFhBagYRvReJcB BuQv+ppZQTKgNLT3UivSK+iX9hS8jRE2xiopMEWS+tyTw3jm5FaPMVFdf2kAZp+aLnEKS/ +XEUC5z7GfETTB32Rr/VfCacp3SRJo4= Received: from mimecast-mx02.redhat.com (mimecast-mx02.redhat.com [66.187.233.88]) by relay.mimecast.com with ESMTP with STARTTLS (version=TLSv1.2, cipher=TLS_ECDHE_RSA_WITH_AES_256_GCM_SHA384) id us-mta-612-4rw8mCRpPNme_IUbRr3wmA-1; Wed, 19 Apr 2023 09:43:50 -0400 X-MC-Unique: 4rw8mCRpPNme_IUbRr3wmA-1 Received: from smtp.corp.redhat.com (int-mx09.intmail.prod.int.rdu2.redhat.com [10.11.54.9]) (using TLSv1.2 with cipher AECDH-AES256-SHA (256/256 bits)) (No client certificate requested) by mimecast-mx02.redhat.com (Postfix) with ESMTPS id CC1B2101A554; Wed, 19 Apr 2023 13:43:49 +0000 (UTC) Received: from max-t490s.redhat.com (unknown [10.39.208.29]) by smtp.corp.redhat.com (Postfix) with ESMTP id BDAB6492B04; Wed, 19 Apr 2023 13:43:47 +0000 (UTC) From: Maxime Coquelin <maxime.coquelin@redhat.com> To: xieyongji@bytedance.com, jasowang@redhat.com, mst@redhat.com, david.marchand@redhat.com Cc: linux-kernel@vger.kernel.org, virtualization@lists.linux-foundation.org, netdev@vger.kernel.org, xuanzhuo@linux.alibaba.com, eperezma@redhat.com, Maxime Coquelin <maxime.coquelin@redhat.com> Subject: [RFC 0/2] vduse: add support for networking devices Date: Wed, 19 Apr 2023 15:43:27 +0200 Message-Id: <20230419134329.346825-1-maxime.coquelin@redhat.com> MIME-Version: 1.0 Content-Transfer-Encoding: 8bit X-Scanned-By: MIMEDefang 3.1 on 10.11.54.9 X-Spam-Status: No, score=-2.1 required=5.0 tests=BAYES_00,DKIMWL_WL_HIGH, DKIM_SIGNED,DKIM_VALID,DKIM_VALID_AU,DKIM_VALID_EF,RCVD_IN_DNSWL_NONE, RCVD_IN_MSPIKE_H2,SPF_HELO_NONE,SPF_NONE,T_SCC_BODY_TEXT_LINE, URIBL_BLOCKED autolearn=ham autolearn_force=no version=3.4.6 X-Spam-Checker-Version: SpamAssassin 3.4.6 (2021-04-09) on lindbergh.monkeyblade.net Precedence: bulk List-ID: <linux-kernel.vger.kernel.org> X-Mailing-List: linux-kernel@vger.kernel.org X-getmail-retrieved-from-mailbox: =?utf-8?q?INBOX?= X-GMAIL-THRID: =?utf-8?q?1763613060168942407?= X-GMAIL-MSGID: =?utf-8?q?1763613060168942407?= |
Series |
vduse: add support for networking devices
|
|
Message
Maxime Coquelin
April 19, 2023, 1:43 p.m. UTC
This small series enables virtio-net device type in VDUSE. With it, basic operation have been tested, both with virtio-vdpa and vhost-vdpa using DPDK Vhost library series adding VDUSE support [0] using split rings layout. Control queue support (and so multiqueue) has also been tested, but require a Kernel series from Jason Wang relaxing control queue polling [1] to function reliably. Other than that, we have identified a few gaps: 1. Reconnection: a. VDUSE_VQ_GET_INFO ioctl() returns always 0 for avail index, even after the virtqueue has already been processed. Is that expected? I have tried instead to get the driver's avail index directly from the avail ring, but it does not seem reliable as I sometimes get "id %u is not a head!\n" warnings. Also such solution would not be possible with packed ring, as we need to know the wrap counters values. b. Missing IOCTLs: it would be handy to have new IOCTLs to query Virtio device status, and retrieve the config space set at VDUSE_CREATE_DEV time. 2. VDUSE application as non-root: We need to run the VDUSE application as non-root. There is some race between the time the UDEV rule is applied and the time the device starts being used. Discussing with Jason, he suggested we may have a VDUSE daemon run as root that would create the VDUSE device, manages its rights and then pass its file descriptor to the VDUSE app. However, with current IOCTLs, it means the VDUSE daemon would need to know several information that belongs to the VDUSE app implementing the device such as supported Virtio features, config space, etc... If we go that route, maybe we should have a control IOCTL to create the device which would just pass the device type. Then another device IOCTL to perform the initialization. Would that make sense? 3. Coredump: In order to be able to perform post-mortem analysis, DPDK Vhost library marks pages used for vrings and descriptors buffers as MADV_DODUMP using madvise(). However with VDUSE it fails with -EINVAL. My understanding is that we set VM_DONTEXPAND flag to the VMAs and madvise's MADV_DODUMP fails if it is present. I'm not sure to understand why madvise would prevent MADV_DODUMP if VM_DONTEXPAND is set. Any thoughts? [0]: https://patchwork.dpdk.org/project/dpdk/list/?series=27594&state=%2A&archive=both [1]: https://lore.kernel.org/lkml/CACGkMEtgrxN3PPwsDo4oOsnsSLJfEmBEZ0WvjGRr3whU+QasUg@mail.gmail.com/T/ Maxime Coquelin (2): vduse: validate block features only with block devices vduse: enable Virtio-net device type drivers/vdpa/vdpa_user/vduse_dev.c | 11 +++++++---- 1 file changed, 7 insertions(+), 4 deletions(-)
Comments
On Wed, Apr 19, 2023 at 9:43 PM Maxime Coquelin <maxime.coquelin@redhat.com> wrote: > > This small series enables virtio-net device type in VDUSE. > With it, basic operation have been tested, both with > virtio-vdpa and vhost-vdpa using DPDK Vhost library series > adding VDUSE support [0] using split rings layout. > > Control queue support (and so multiqueue) has also been > tested, but require a Kernel series from Jason Wang > relaxing control queue polling [1] to function reliably. > > Other than that, we have identified a few gaps: > > 1. Reconnection: > a. VDUSE_VQ_GET_INFO ioctl() returns always 0 for avail > index, even after the virtqueue has already been > processed. Is that expected? I have tried instead to > get the driver's avail index directly from the avail > ring, but it does not seem reliable as I sometimes get > "id %u is not a head!\n" warnings. Also such solution > would not be possible with packed ring, as we need to > know the wrap counters values. Looking at the codes, it only returns the value that is set via set_vq_state(). I think it is expected to be called before the datapath runs. So when bound to virtio-vdpa, it is expected to return 0. But we need to fix the packed virtqueue case, I wonder if we need to call set_vq_state() explicitly in virtio-vdpa before starting the device. When bound to vhost-vdpa, Qemu will call VHOST_SET_VRING_BASE which will end up a call to set_vq_state(). Unfortunately, it doesn't support packed ring which needs some extension. > > b. Missing IOCTLs: it would be handy to have new IOCTLs to > query Virtio device status, What's the use case of this ioctl? It looks to me userspace is notified on each status change now: static int vduse_dev_set_status(struct vduse_dev *dev, u8 status) { struct vduse_dev_msg msg = { 0 }; msg.req.type = VDUSE_SET_STATUS; msg.req.s.status = status; return vduse_dev_msg_sync(dev, &msg); } > and retrieve the config > space set at VDUSE_CREATE_DEV time. In order to be safe, VDUSE avoids writable config space. Otherwise drivers could block on config writing forever. That's why we don't do it now. We need to harden the config write before we can proceed to this I think. > > 2. VDUSE application as non-root: > We need to run the VDUSE application as non-root. There > is some race between the time the UDEV rule is applied > and the time the device starts being used. Discussing > with Jason, he suggested we may have a VDUSE daemon run > as root that would create the VDUSE device, manages its > rights and then pass its file descriptor to the VDUSE > app. However, with current IOCTLs, it means the VDUSE > daemon would need to know several information that > belongs to the VDUSE app implementing the device such > as supported Virtio features, config space, etc... > If we go that route, maybe we should have a control > IOCTL to create the device which would just pass the > device type. Then another device IOCTL to perform the > initialization. Would that make sense? I think so. We can hear from others. > > 3. Coredump: > In order to be able to perform post-mortem analysis, DPDK > Vhost library marks pages used for vrings and descriptors > buffers as MADV_DODUMP using madvise(). However with > VDUSE it fails with -EINVAL. My understanding is that we > set VM_DONTEXPAND flag to the VMAs and madvise's > MADV_DODUMP fails if it is present. I'm not sure to > understand why madvise would prevent MADV_DODUMP if > VM_DONTEXPAND is set. Any thoughts? Adding Peter who may know the answer. Thanks > > [0]: https://patchwork.dpdk.org/project/dpdk/list/?series=27594&state=%2A&archive=both > [1]: https://lore.kernel.org/lkml/CACGkMEtgrxN3PPwsDo4oOsnsSLJfEmBEZ0WvjGRr3whU+QasUg@mail.gmail.com/T/ > > Maxime Coquelin (2): > vduse: validate block features only with block devices > vduse: enable Virtio-net device type > > drivers/vdpa/vdpa_user/vduse_dev.c | 11 +++++++---- > 1 file changed, 7 insertions(+), 4 deletions(-) > > -- > 2.39.2 >
On Wed, Apr 19, 2023 at 9:44 PM Maxime Coquelin <maxime.coquelin@redhat.com> wrote: > > This small series enables virtio-net device type in VDUSE. > With it, basic operation have been tested, both with > virtio-vdpa and vhost-vdpa using DPDK Vhost library series > adding VDUSE support [0] using split rings layout. > > Control queue support (and so multiqueue) has also been > tested, but require a Kernel series from Jason Wang > relaxing control queue polling [1] to function reliably. > > Other than that, we have identified a few gaps: > > 1. Reconnection: > a. VDUSE_VQ_GET_INFO ioctl() returns always 0 for avail > index, even after the virtqueue has already been > processed. Is that expected? I have tried instead to > get the driver's avail index directly from the avail > ring, but it does not seem reliable as I sometimes get > "id %u is not a head!\n" warnings. Also such solution > would not be possible with packed ring, as we need to > know the wrap counters values. > I'm not sure how to handle the reconnection in the vhost-user-net case. Can we use a tmpfs file to track inflight I/O like this [1] [1] https://qemu-project.gitlab.io/qemu/interop/vhost-user.html#inflight-i-o-tracking > b. Missing IOCTLs: it would be handy to have new IOCTLs to > query Virtio device status, and retrieve the config > space set at VDUSE_CREATE_DEV time. > VDUSE_GET_STATUS ioctl might be needed. Or can we use a tmpfs file to save/restore that info. > 2. VDUSE application as non-root: > We need to run the VDUSE application as non-root. There > is some race between the time the UDEV rule is applied > and the time the device starts being used. Discussing > with Jason, he suggested we may have a VDUSE daemon run > as root that would create the VDUSE device, manages its > rights and then pass its file descriptor to the VDUSE > app. However, with current IOCTLs, it means the VDUSE > daemon would need to know several information that > belongs to the VDUSE app implementing the device such > as supported Virtio features, config space, etc... > If we go that route, maybe we should have a control > IOCTL to create the device which would just pass the > device type. Then another device IOCTL to perform the > initialization. Would that make sense? > I think we can reuse the VDUSE_CREATE_DEV ioctl (just use name, device_id and vendor_id) for control device here, and add a new ioctl VDUSE_DEV_SETUP to do device initialization. Thanks, Yongji
On 4/20/23 06:34, Jason Wang wrote: > On Wed, Apr 19, 2023 at 9:43 PM Maxime Coquelin > <maxime.coquelin@redhat.com> wrote: >> >> This small series enables virtio-net device type in VDUSE. >> With it, basic operation have been tested, both with >> virtio-vdpa and vhost-vdpa using DPDK Vhost library series >> adding VDUSE support [0] using split rings layout. >> >> Control queue support (and so multiqueue) has also been >> tested, but require a Kernel series from Jason Wang >> relaxing control queue polling [1] to function reliably. >> >> Other than that, we have identified a few gaps: >> >> 1. Reconnection: >> a. VDUSE_VQ_GET_INFO ioctl() returns always 0 for avail >> index, even after the virtqueue has already been >> processed. Is that expected? I have tried instead to >> get the driver's avail index directly from the avail >> ring, but it does not seem reliable as I sometimes get >> "id %u is not a head!\n" warnings. Also such solution >> would not be possible with packed ring, as we need to >> know the wrap counters values. > > Looking at the codes, it only returns the value that is set via > set_vq_state(). I think it is expected to be called before the > datapath runs. > > So when bound to virtio-vdpa, it is expected to return 0. But we need > to fix the packed virtqueue case, I wonder if we need to call > set_vq_state() explicitly in virtio-vdpa before starting the device. > > When bound to vhost-vdpa, Qemu will call VHOST_SET_VRING_BASE which > will end up a call to set_vq_state(). Unfortunately, it doesn't > support packed ring which needs some extension. > >> >> b. Missing IOCTLs: it would be handy to have new IOCTLs to >> query Virtio device status, > > What's the use case of this ioctl? It looks to me userspace is > notified on each status change now: > > static int vduse_dev_set_status(struct vduse_dev *dev, u8 status) > { > struct vduse_dev_msg msg = { 0 }; > > msg.req.type = VDUSE_SET_STATUS; > msg.req.s.status = status; > > return vduse_dev_msg_sync(dev, &msg); > } The idea was to be able to query the status at reconnect time, and neither having to assume its value nor having to store its value in a file (the status could change while the VDUSE application is stopped, but maybe it would receive the notification at reconnect). I will prototype using a tmpfs file to save needed information, and see if it works. >> and retrieve the config >> space set at VDUSE_CREATE_DEV time. > > In order to be safe, VDUSE avoids writable config space. Otherwise > drivers could block on config writing forever. That's why we don't do > it now. The idea was not to make the config space writable, but just to be able to fetch what was filled at VDUSE_CREATE_DEV time. With the tmpfs file, we can avoid doing that and just save the config space there. > We need to harden the config write before we can proceed to this I think. > >> >> 2. VDUSE application as non-root: >> We need to run the VDUSE application as non-root. There >> is some race between the time the UDEV rule is applied >> and the time the device starts being used. Discussing >> with Jason, he suggested we may have a VDUSE daemon run >> as root that would create the VDUSE device, manages its >> rights and then pass its file descriptor to the VDUSE >> app. However, with current IOCTLs, it means the VDUSE >> daemon would need to know several information that >> belongs to the VDUSE app implementing the device such >> as supported Virtio features, config space, etc... >> If we go that route, maybe we should have a control >> IOCTL to create the device which would just pass the >> device type. Then another device IOCTL to perform the >> initialization. Would that make sense? > > I think so. We can hear from others. > >> >> 3. Coredump: >> In order to be able to perform post-mortem analysis, DPDK >> Vhost library marks pages used for vrings and descriptors >> buffers as MADV_DODUMP using madvise(). However with >> VDUSE it fails with -EINVAL. My understanding is that we >> set VM_DONTEXPAND flag to the VMAs and madvise's >> MADV_DODUMP fails if it is present. I'm not sure to >> understand why madvise would prevent MADV_DODUMP if >> VM_DONTEXPAND is set. Any thoughts? > > Adding Peter who may know the answer. Thanks! Maxime > Thanks > >> >> [0]: https://patchwork.dpdk.org/project/dpdk/list/?series=27594&state=%2A&archive=both >> [1]: https://lore.kernel.org/lkml/CACGkMEtgrxN3PPwsDo4oOsnsSLJfEmBEZ0WvjGRr3whU+QasUg@mail.gmail.com/T/ >> >> Maxime Coquelin (2): >> vduse: validate block features only with block devices >> vduse: enable Virtio-net device type >> >> drivers/vdpa/vdpa_user/vduse_dev.c | 11 +++++++---- >> 1 file changed, 7 insertions(+), 4 deletions(-) >> >> -- >> 2.39.2 >> >
On 4/20/23 10:13, Yongji Xie wrote: > On Wed, Apr 19, 2023 at 9:44 PM Maxime Coquelin > <maxime.coquelin@redhat.com> wrote: >> >> This small series enables virtio-net device type in VDUSE. >> With it, basic operation have been tested, both with >> virtio-vdpa and vhost-vdpa using DPDK Vhost library series >> adding VDUSE support [0] using split rings layout. >> >> Control queue support (and so multiqueue) has also been >> tested, but require a Kernel series from Jason Wang >> relaxing control queue polling [1] to function reliably. >> >> Other than that, we have identified a few gaps: >> >> 1. Reconnection: >> a. VDUSE_VQ_GET_INFO ioctl() returns always 0 for avail >> index, even after the virtqueue has already been >> processed. Is that expected? I have tried instead to >> get the driver's avail index directly from the avail >> ring, but it does not seem reliable as I sometimes get >> "id %u is not a head!\n" warnings. Also such solution >> would not be possible with packed ring, as we need to >> know the wrap counters values. >> > > I'm not sure how to handle the reconnection in the vhost-user-net > case. Can we use a tmpfs file to track inflight I/O like this [1] We don't have inflight IOs with DPDK Vhsot library for net devices. But yes, a solution is to have a tmpfs file to save needed data. Advantage of this solution is it makes it possible to reconnect with packed ring in case of application crash, as the wrap counter values would not be lost. > [1] https://qemu-project.gitlab.io/qemu/interop/vhost-user.html#inflight-i-o-tracking > >> b. Missing IOCTLs: it would be handy to have new IOCTLs to >> query Virtio device status, and retrieve the config >> space set at VDUSE_CREATE_DEV time. >> > > VDUSE_GET_STATUS ioctl might be needed. Or can we use a tmpfs file to > save/restore that info. > >> 2. VDUSE application as non-root: >> We need to run the VDUSE application as non-root. There >> is some race between the time the UDEV rule is applied >> and the time the device starts being used. Discussing >> with Jason, he suggested we may have a VDUSE daemon run >> as root that would create the VDUSE device, manages its >> rights and then pass its file descriptor to the VDUSE >> app. However, with current IOCTLs, it means the VDUSE >> daemon would need to know several information that >> belongs to the VDUSE app implementing the device such >> as supported Virtio features, config space, etc... >> If we go that route, maybe we should have a control >> IOCTL to create the device which would just pass the >> device type. Then another device IOCTL to perform the >> initialization. Would that make sense? >> > > I think we can reuse the VDUSE_CREATE_DEV ioctl (just use name, > device_id and vendor_id) for control device here, and add a new ioctl > VDUSE_DEV_SETUP to do device initialization. OK. If we do that, could we also provide the possibility to pass an UUID at VDUSE_DEV_SETUP time? It could be useful if we save information in a tmpfs file, in order to be able at reconnect time to ensure the tmpfs file UUID matches with the VDUSE device UUID, and so avoid restoring a leftover tmpfs file of a previously detroyed then re-created VDUSE device. Would that make sense? Regards, Maxime > Thanks, > Yongji >
On Thu, Apr 20, 2023 at 12:34:06PM +0800, Jason Wang wrote: > > 3. Coredump: > > In order to be able to perform post-mortem analysis, DPDK > > Vhost library marks pages used for vrings and descriptors > > buffers as MADV_DODUMP using madvise(). However with > > VDUSE it fails with -EINVAL. My understanding is that we > > set VM_DONTEXPAND flag to the VMAs and madvise's > > MADV_DODUMP fails if it is present. I'm not sure to > > understand why madvise would prevent MADV_DODUMP if > > VM_DONTEXPAND is set. Any thoughts? > > Adding Peter who may know the answer. I don't.. but I had a quick look, it seems that VM_DONTEXPAND was kind of reused (and I'm not sure whether it's an abuse or not so far..) to represent device driver pages since removal of VM_RESERVED: https://lore.kernel.org/all/20120731103457.20182.88454.stgit@zurg/ https://lore.kernel.org/all/20120731103503.20182.94365.stgit@zurg/ But I think that change at least breaks hugetlb once so there's the explicit hugetlb check to recover that behavior back: https://lore.kernel.org/all/20180930054629.29150-1-daniel@linux.ibm.com/ Thanks,
On Thu, Apr 20, 2023 at 10:16 PM Maxime Coquelin <maxime.coquelin@redhat.com> wrote: > > > > On 4/20/23 06:34, Jason Wang wrote: > > On Wed, Apr 19, 2023 at 9:43 PM Maxime Coquelin > > <maxime.coquelin@redhat.com> wrote: > >> > >> This small series enables virtio-net device type in VDUSE. > >> With it, basic operation have been tested, both with > >> virtio-vdpa and vhost-vdpa using DPDK Vhost library series > >> adding VDUSE support [0] using split rings layout. > >> > >> Control queue support (and so multiqueue) has also been > >> tested, but require a Kernel series from Jason Wang > >> relaxing control queue polling [1] to function reliably. > >> > >> Other than that, we have identified a few gaps: > >> > >> 1. Reconnection: > >> a. VDUSE_VQ_GET_INFO ioctl() returns always 0 for avail > >> index, even after the virtqueue has already been > >> processed. Is that expected? I have tried instead to > >> get the driver's avail index directly from the avail > >> ring, but it does not seem reliable as I sometimes get > >> "id %u is not a head!\n" warnings. Also such solution > >> would not be possible with packed ring, as we need to > >> know the wrap counters values. > > > > Looking at the codes, it only returns the value that is set via > > set_vq_state(). I think it is expected to be called before the > > datapath runs. > > > > So when bound to virtio-vdpa, it is expected to return 0. But we need > > to fix the packed virtqueue case, I wonder if we need to call > > set_vq_state() explicitly in virtio-vdpa before starting the device. > > > > When bound to vhost-vdpa, Qemu will call VHOST_SET_VRING_BASE which > > will end up a call to set_vq_state(). Unfortunately, it doesn't > > support packed ring which needs some extension. > > > >> > >> b. Missing IOCTLs: it would be handy to have new IOCTLs to > >> query Virtio device status, > > > > What's the use case of this ioctl? It looks to me userspace is > > notified on each status change now: > > > > static int vduse_dev_set_status(struct vduse_dev *dev, u8 status) > > { > > struct vduse_dev_msg msg = { 0 }; > > > > msg.req.type = VDUSE_SET_STATUS; > > msg.req.s.status = status; > > > > return vduse_dev_msg_sync(dev, &msg); > > } > > The idea was to be able to query the status at reconnect time, and > neither having to assume its value nor having to store its value in a > file (the status could change while the VDUSE application is stopped, > but maybe it would receive the notification at reconnect). I see. > > I will prototype using a tmpfs file to save needed information, and see > if it works. It might work but then the API is not self contained. Maybe it's better to have a dedicated ioctl. > > >> and retrieve the config > >> space set at VDUSE_CREATE_DEV time. > > > > In order to be safe, VDUSE avoids writable config space. Otherwise > > drivers could block on config writing forever. That's why we don't do > > it now. > > The idea was not to make the config space writable, but just to be able > to fetch what was filled at VDUSE_CREATE_DEV time. > > With the tmpfs file, we can avoid doing that and just save the config > space there. Same as the case for status. Thanks > > > We need to harden the config write before we can proceed to this I think. > > > >> > >> 2. VDUSE application as non-root: > >> We need to run the VDUSE application as non-root. There > >> is some race between the time the UDEV rule is applied > >> and the time the device starts being used. Discussing > >> with Jason, he suggested we may have a VDUSE daemon run > >> as root that would create the VDUSE device, manages its > >> rights and then pass its file descriptor to the VDUSE > >> app. However, with current IOCTLs, it means the VDUSE > >> daemon would need to know several information that > >> belongs to the VDUSE app implementing the device such > >> as supported Virtio features, config space, etc... > >> If we go that route, maybe we should have a control > >> IOCTL to create the device which would just pass the > >> device type. Then another device IOCTL to perform the > >> initialization. Would that make sense? > > > > I think so. We can hear from others. > > > >> > >> 3. Coredump: > >> In order to be able to perform post-mortem analysis, DPDK > >> Vhost library marks pages used for vrings and descriptors > >> buffers as MADV_DODUMP using madvise(). However with > >> VDUSE it fails with -EINVAL. My understanding is that we > >> set VM_DONTEXPAND flag to the VMAs and madvise's > >> MADV_DODUMP fails if it is present. I'm not sure to > >> understand why madvise would prevent MADV_DODUMP if > >> VM_DONTEXPAND is set. Any thoughts? > > > > Adding Peter who may know the answer. > > Thanks! > Maxime > > > Thanks > > > >> > >> [0]: https://patchwork.dpdk.org/project/dpdk/list/?series=27594&state=%2A&archive=both > >> [1]: https://lore.kernel.org/lkml/CACGkMEtgrxN3PPwsDo4oOsnsSLJfEmBEZ0WvjGRr3whU+QasUg@mail.gmail.com/T/ > >> > >> Maxime Coquelin (2): > >> vduse: validate block features only with block devices > >> vduse: enable Virtio-net device type > >> > >> drivers/vdpa/vdpa_user/vduse_dev.c | 11 +++++++---- > >> 1 file changed, 7 insertions(+), 4 deletions(-) > >> > >> -- > >> 2.39.2 > >> > > >
On Fri, Apr 21, 2023 at 2:39 AM Peter Xu <peterx@redhat.com> wrote: > > On Thu, Apr 20, 2023 at 12:34:06PM +0800, Jason Wang wrote: > > > 3. Coredump: > > > In order to be able to perform post-mortem analysis, DPDK > > > Vhost library marks pages used for vrings and descriptors > > > buffers as MADV_DODUMP using madvise(). However with > > > VDUSE it fails with -EINVAL. My understanding is that we > > > set VM_DONTEXPAND flag to the VMAs and madvise's > > > MADV_DODUMP fails if it is present. I'm not sure to > > > understand why madvise would prevent MADV_DODUMP if > > > VM_DONTEXPAND is set. Any thoughts? > > > > Adding Peter who may know the answer. > > I don't.. but I had a quick look, it seems that VM_DONTEXPAND was kind of > reused (and I'm not sure whether it's an abuse or not so far..) to > represent device driver pages since removal of VM_RESERVED: Interesting, but there're indeed cases where VM_DONTEXPAND is used by non-driver codes. The pages mapped by VDUSE are not device driver pages but the IOTLB and virtqueue. Btw the definition of VM_SPECIAL doesn't say anything related to dump: /* * Special vmas that are non-mergable, non-mlock()able. */ #define VM_SPECIAL (VM_IO | VM_DONTEXPAND | VM_PFNMAP | VM_MIXEDMAP) > > https://lore.kernel.org/all/20120731103457.20182.88454.stgit@zurg/ > https://lore.kernel.org/all/20120731103503.20182.94365.stgit@zurg/ > > But I think that change at least breaks hugetlb once so there's the > explicit hugetlb check to recover that behavior back: > > https://lore.kernel.org/all/20180930054629.29150-1-daniel@linux.ibm.com/ This seems similar to the case of VDUSE. Thanks > > Thanks, > > -- > Peter Xu >
On 4/21/23 07:51, Jason Wang wrote: > On Thu, Apr 20, 2023 at 10:16 PM Maxime Coquelin > <maxime.coquelin@redhat.com> wrote: >> >> >> >> On 4/20/23 06:34, Jason Wang wrote: >>> On Wed, Apr 19, 2023 at 9:43 PM Maxime Coquelin >>> <maxime.coquelin@redhat.com> wrote: >>>> >>>> This small series enables virtio-net device type in VDUSE. >>>> With it, basic operation have been tested, both with >>>> virtio-vdpa and vhost-vdpa using DPDK Vhost library series >>>> adding VDUSE support [0] using split rings layout. >>>> >>>> Control queue support (and so multiqueue) has also been >>>> tested, but require a Kernel series from Jason Wang >>>> relaxing control queue polling [1] to function reliably. >>>> >>>> Other than that, we have identified a few gaps: >>>> >>>> 1. Reconnection: >>>> a. VDUSE_VQ_GET_INFO ioctl() returns always 0 for avail >>>> index, even after the virtqueue has already been >>>> processed. Is that expected? I have tried instead to >>>> get the driver's avail index directly from the avail >>>> ring, but it does not seem reliable as I sometimes get >>>> "id %u is not a head!\n" warnings. Also such solution >>>> would not be possible with packed ring, as we need to >>>> know the wrap counters values. >>> >>> Looking at the codes, it only returns the value that is set via >>> set_vq_state(). I think it is expected to be called before the >>> datapath runs. >>> >>> So when bound to virtio-vdpa, it is expected to return 0. But we need >>> to fix the packed virtqueue case, I wonder if we need to call >>> set_vq_state() explicitly in virtio-vdpa before starting the device. >>> >>> When bound to vhost-vdpa, Qemu will call VHOST_SET_VRING_BASE which >>> will end up a call to set_vq_state(). Unfortunately, it doesn't >>> support packed ring which needs some extension. >>> >>>> >>>> b. Missing IOCTLs: it would be handy to have new IOCTLs to >>>> query Virtio device status, >>> >>> What's the use case of this ioctl? It looks to me userspace is >>> notified on each status change now: >>> >>> static int vduse_dev_set_status(struct vduse_dev *dev, u8 status) >>> { >>> struct vduse_dev_msg msg = { 0 }; >>> >>> msg.req.type = VDUSE_SET_STATUS; >>> msg.req.s.status = status; >>> >>> return vduse_dev_msg_sync(dev, &msg); >>> } >> >> The idea was to be able to query the status at reconnect time, and >> neither having to assume its value nor having to store its value in a >> file (the status could change while the VDUSE application is stopped, >> but maybe it would receive the notification at reconnect). > > I see. > >> >> I will prototype using a tmpfs file to save needed information, and see >> if it works. > > It might work but then the API is not self contained. Maybe it's > better to have a dedicated ioctl. > >> >>>> and retrieve the config >>>> space set at VDUSE_CREATE_DEV time. >>> >>> In order to be safe, VDUSE avoids writable config space. Otherwise >>> drivers could block on config writing forever. That's why we don't do >>> it now. >> >> The idea was not to make the config space writable, but just to be able >> to fetch what was filled at VDUSE_CREATE_DEV time. >> >> With the tmpfs file, we can avoid doing that and just save the config >> space there. > > Same as the case for status. I have cooked a DPDK patch to support reconnect with a tmpfs file as suggested by Yongji: https://gitlab.com/mcoquelin/dpdk-next-virtio/-/commit/53913f2b1155b02c44d5d3d298aafd357e7a8c48 That's still rough around the edges, but it seems to work reliably for the testing I have done so far. We'll certainly want to use the tmpfs memory to directly store available indexes and wrap counters to avoid introducing overhead in the datapath. The tricky part will be to manage NUMA affinity. Regards, Maxime > > Thanks > >> >>> We need to harden the config write before we can proceed to this I think. >>> >>>> >>>> 2. VDUSE application as non-root: >>>> We need to run the VDUSE application as non-root. There >>>> is some race between the time the UDEV rule is applied >>>> and the time the device starts being used. Discussing >>>> with Jason, he suggested we may have a VDUSE daemon run >>>> as root that would create the VDUSE device, manages its >>>> rights and then pass its file descriptor to the VDUSE >>>> app. However, with current IOCTLs, it means the VDUSE >>>> daemon would need to know several information that >>>> belongs to the VDUSE app implementing the device such >>>> as supported Virtio features, config space, etc... >>>> If we go that route, maybe we should have a control >>>> IOCTL to create the device which would just pass the >>>> device type. Then another device IOCTL to perform the >>>> initialization. Would that make sense? >>> >>> I think so. We can hear from others. >>> >>>> >>>> 3. Coredump: >>>> In order to be able to perform post-mortem analysis, DPDK >>>> Vhost library marks pages used for vrings and descriptors >>>> buffers as MADV_DODUMP using madvise(). However with >>>> VDUSE it fails with -EINVAL. My understanding is that we >>>> set VM_DONTEXPAND flag to the VMAs and madvise's >>>> MADV_DODUMP fails if it is present. I'm not sure to >>>> understand why madvise would prevent MADV_DODUMP if >>>> VM_DONTEXPAND is set. Any thoughts? >>> >>> Adding Peter who may know the answer. >> >> Thanks! >> Maxime >> >>> Thanks >>> >>>> >>>> [0]: https://patchwork.dpdk.org/project/dpdk/list/?series=27594&state=%2A&archive=both >>>> [1]: https://lore.kernel.org/lkml/CACGkMEtgrxN3PPwsDo4oOsnsSLJfEmBEZ0WvjGRr3whU+QasUg@mail.gmail.com/T/ >>>> >>>> Maxime Coquelin (2): >>>> vduse: validate block features only with block devices >>>> vduse: enable Virtio-net device type >>>> >>>> drivers/vdpa/vdpa_user/vduse_dev.c | 11 +++++++---- >>>> 1 file changed, 7 insertions(+), 4 deletions(-) >>>> >>>> -- >>>> 2.39.2 >>>> >>> >> >
On Fri, Apr 21, 2023 at 10:28 PM Maxime Coquelin <maxime.coquelin@redhat.com> wrote: > > > > On 4/21/23 07:51, Jason Wang wrote: > > On Thu, Apr 20, 2023 at 10:16 PM Maxime Coquelin > > <maxime.coquelin@redhat.com> wrote: > >> > >> > >> > >> On 4/20/23 06:34, Jason Wang wrote: > >>> On Wed, Apr 19, 2023 at 9:43 PM Maxime Coquelin > >>> <maxime.coquelin@redhat.com> wrote: > >>>> > >>>> This small series enables virtio-net device type in VDUSE. > >>>> With it, basic operation have been tested, both with > >>>> virtio-vdpa and vhost-vdpa using DPDK Vhost library series > >>>> adding VDUSE support [0] using split rings layout. > >>>> > >>>> Control queue support (and so multiqueue) has also been > >>>> tested, but require a Kernel series from Jason Wang > >>>> relaxing control queue polling [1] to function reliably. > >>>> > >>>> Other than that, we have identified a few gaps: > >>>> > >>>> 1. Reconnection: > >>>> a. VDUSE_VQ_GET_INFO ioctl() returns always 0 for avail > >>>> index, even after the virtqueue has already been > >>>> processed. Is that expected? I have tried instead to > >>>> get the driver's avail index directly from the avail > >>>> ring, but it does not seem reliable as I sometimes get > >>>> "id %u is not a head!\n" warnings. Also such solution > >>>> would not be possible with packed ring, as we need to > >>>> know the wrap counters values. > >>> > >>> Looking at the codes, it only returns the value that is set via > >>> set_vq_state(). I think it is expected to be called before the > >>> datapath runs. > >>> > >>> So when bound to virtio-vdpa, it is expected to return 0. But we need > >>> to fix the packed virtqueue case, I wonder if we need to call > >>> set_vq_state() explicitly in virtio-vdpa before starting the device. > >>> > >>> When bound to vhost-vdpa, Qemu will call VHOST_SET_VRING_BASE which > >>> will end up a call to set_vq_state(). Unfortunately, it doesn't > >>> support packed ring which needs some extension. > >>> > >>>> > >>>> b. Missing IOCTLs: it would be handy to have new IOCTLs to > >>>> query Virtio device status, > >>> > >>> What's the use case of this ioctl? It looks to me userspace is > >>> notified on each status change now: > >>> > >>> static int vduse_dev_set_status(struct vduse_dev *dev, u8 status) > >>> { > >>> struct vduse_dev_msg msg = { 0 }; > >>> > >>> msg.req.type = VDUSE_SET_STATUS; > >>> msg.req.s.status = status; > >>> > >>> return vduse_dev_msg_sync(dev, &msg); > >>> } > >> > >> The idea was to be able to query the status at reconnect time, and > >> neither having to assume its value nor having to store its value in a > >> file (the status could change while the VDUSE application is stopped, > >> but maybe it would receive the notification at reconnect). > > > > I see. > > > >> > >> I will prototype using a tmpfs file to save needed information, and see > >> if it works. > > > > It might work but then the API is not self contained. Maybe it's > > better to have a dedicated ioctl. > > > >> > >>>> and retrieve the config > >>>> space set at VDUSE_CREATE_DEV time. > >>> > >>> In order to be safe, VDUSE avoids writable config space. Otherwise > >>> drivers could block on config writing forever. That's why we don't do > >>> it now. > >> > >> The idea was not to make the config space writable, but just to be able > >> to fetch what was filled at VDUSE_CREATE_DEV time. > >> > >> With the tmpfs file, we can avoid doing that and just save the config > >> space there. > > > > Same as the case for status. > > I have cooked a DPDK patch to support reconnect with a tmpfs file as > suggested by Yongji: > > https://gitlab.com/mcoquelin/dpdk-next-virtio/-/commit/53913f2b1155b02c44d5d3d298aafd357e7a8c48 This seems tricky, for example for status: dev->log->status = dev->status; What if we crash here? > > That's still rough around the edges, but it seems to work reliably > for the testing I have done so far. We'll certainly want to use the > tmpfs memory to directly store available indexes and wrap counters to > avoid introducing overhead in the datapath. That's fine, we probably need a chapter in the kernel doc to describe the reliable reconnection but it is not limited to tmpfs. > The tricky part will be to > manage NUMA affinity. This part is not clear to me, what affinity should we care about? There's a sysfs that was invented by YongJi for virtqueue affinity management recently. Thanks > > Regards, > Maxime > > > > > Thanks > > > >> > >>> We need to harden the config write before we can proceed to this I think. > >>> > >>>> > >>>> 2. VDUSE application as non-root: > >>>> We need to run the VDUSE application as non-root. There > >>>> is some race between the time the UDEV rule is applied > >>>> and the time the device starts being used. Discussing > >>>> with Jason, he suggested we may have a VDUSE daemon run > >>>> as root that would create the VDUSE device, manages its > >>>> rights and then pass its file descriptor to the VDUSE > >>>> app. However, with current IOCTLs, it means the VDUSE > >>>> daemon would need to know several information that > >>>> belongs to the VDUSE app implementing the device such > >>>> as supported Virtio features, config space, etc... > >>>> If we go that route, maybe we should have a control > >>>> IOCTL to create the device which would just pass the > >>>> device type. Then another device IOCTL to perform the > >>>> initialization. Would that make sense? > >>> > >>> I think so. We can hear from others. > >>> > >>>> > >>>> 3. Coredump: > >>>> In order to be able to perform post-mortem analysis, DPDK > >>>> Vhost library marks pages used for vrings and descriptors > >>>> buffers as MADV_DODUMP using madvise(). However with > >>>> VDUSE it fails with -EINVAL. My understanding is that we > >>>> set VM_DONTEXPAND flag to the VMAs and madvise's > >>>> MADV_DODUMP fails if it is present. I'm not sure to > >>>> understand why madvise would prevent MADV_DODUMP if > >>>> VM_DONTEXPAND is set. Any thoughts? > >>> > >>> Adding Peter who may know the answer. > >> > >> Thanks! > >> Maxime > >> > >>> Thanks > >>> > >>>> > >>>> [0]: https://patchwork.dpdk.org/project/dpdk/list/?series=27594&state=%2A&archive=both > >>>> [1]: https://lore.kernel.org/lkml/CACGkMEtgrxN3PPwsDo4oOsnsSLJfEmBEZ0WvjGRr3whU+QasUg@mail.gmail.com/T/ > >>>> > >>>> Maxime Coquelin (2): > >>>> vduse: validate block features only with block devices > >>>> vduse: enable Virtio-net device type > >>>> > >>>> drivers/vdpa/vdpa_user/vduse_dev.c | 11 +++++++---- > >>>> 1 file changed, 7 insertions(+), 4 deletions(-) > >>>> > >>>> -- > >>>> 2.39.2 > >>>> > >>> > >> > > >
On Sun, Apr 23, 2023 at 2:31 PM Jason Wang <jasowang@redhat.com> wrote: > > On Fri, Apr 21, 2023 at 10:28 PM Maxime Coquelin > <maxime.coquelin@redhat.com> wrote: > > > > > > > > On 4/21/23 07:51, Jason Wang wrote: > > > On Thu, Apr 20, 2023 at 10:16 PM Maxime Coquelin > > > <maxime.coquelin@redhat.com> wrote: > > >> > > >> > > >> > > >> On 4/20/23 06:34, Jason Wang wrote: > > >>> On Wed, Apr 19, 2023 at 9:43 PM Maxime Coquelin > > >>> <maxime.coquelin@redhat.com> wrote: > > >>>> > > >>>> This small series enables virtio-net device type in VDUSE. > > >>>> With it, basic operation have been tested, both with > > >>>> virtio-vdpa and vhost-vdpa using DPDK Vhost library series > > >>>> adding VDUSE support [0] using split rings layout. > > >>>> > > >>>> Control queue support (and so multiqueue) has also been > > >>>> tested, but require a Kernel series from Jason Wang > > >>>> relaxing control queue polling [1] to function reliably. > > >>>> > > >>>> Other than that, we have identified a few gaps: > > >>>> > > >>>> 1. Reconnection: > > >>>> a. VDUSE_VQ_GET_INFO ioctl() returns always 0 for avail > > >>>> index, even after the virtqueue has already been > > >>>> processed. Is that expected? I have tried instead to > > >>>> get the driver's avail index directly from the avail > > >>>> ring, but it does not seem reliable as I sometimes get > > >>>> "id %u is not a head!\n" warnings. Also such solution > > >>>> would not be possible with packed ring, as we need to > > >>>> know the wrap counters values. > > >>> > > >>> Looking at the codes, it only returns the value that is set via > > >>> set_vq_state(). I think it is expected to be called before the > > >>> datapath runs. > > >>> > > >>> So when bound to virtio-vdpa, it is expected to return 0. But we need > > >>> to fix the packed virtqueue case, I wonder if we need to call > > >>> set_vq_state() explicitly in virtio-vdpa before starting the device. > > >>> > > >>> When bound to vhost-vdpa, Qemu will call VHOST_SET_VRING_BASE which > > >>> will end up a call to set_vq_state(). Unfortunately, it doesn't > > >>> support packed ring which needs some extension. > > >>> > > >>>> > > >>>> b. Missing IOCTLs: it would be handy to have new IOCTLs to > > >>>> query Virtio device status, > > >>> > > >>> What's the use case of this ioctl? It looks to me userspace is > > >>> notified on each status change now: > > >>> > > >>> static int vduse_dev_set_status(struct vduse_dev *dev, u8 status) > > >>> { > > >>> struct vduse_dev_msg msg = { 0 }; > > >>> > > >>> msg.req.type = VDUSE_SET_STATUS; > > >>> msg.req.s.status = status; > > >>> > > >>> return vduse_dev_msg_sync(dev, &msg); > > >>> } > > >> > > >> The idea was to be able to query the status at reconnect time, and > > >> neither having to assume its value nor having to store its value in a > > >> file (the status could change while the VDUSE application is stopped, > > >> but maybe it would receive the notification at reconnect). > > > > > > I see. > > > > > >> > > >> I will prototype using a tmpfs file to save needed information, and see > > >> if it works. > > > > > > It might work but then the API is not self contained. Maybe it's > > > better to have a dedicated ioctl. > > > > > >> > > >>>> and retrieve the config > > >>>> space set at VDUSE_CREATE_DEV time. > > >>> > > >>> In order to be safe, VDUSE avoids writable config space. Otherwise > > >>> drivers could block on config writing forever. That's why we don't do > > >>> it now. > > >> > > >> The idea was not to make the config space writable, but just to be able > > >> to fetch what was filled at VDUSE_CREATE_DEV time. > > >> > > >> With the tmpfs file, we can avoid doing that and just save the config > > >> space there. > > > > > > Same as the case for status. > > > > I have cooked a DPDK patch to support reconnect with a tmpfs file as > > suggested by Yongji: > > > > https://gitlab.com/mcoquelin/dpdk-next-virtio/-/commit/53913f2b1155b02c44d5d3d298aafd357e7a8c48 > > This seems tricky, for example for status: > > dev->log->status = dev->status; > > What if we crash here? > The message will be re-sent by the kernel if it's not replied. But I think it would be better if we can restore it via some ioctl. Thanks, Yongji
On Sun, Apr 23, 2023 at 4:22 PM Yongji Xie <xieyongji@bytedance.com> wrote: > > On Sun, Apr 23, 2023 at 2:31 PM Jason Wang <jasowang@redhat.com> wrote: > > > > On Fri, Apr 21, 2023 at 10:28 PM Maxime Coquelin > > <maxime.coquelin@redhat.com> wrote: > > > > > > > > > > > > On 4/21/23 07:51, Jason Wang wrote: > > > > On Thu, Apr 20, 2023 at 10:16 PM Maxime Coquelin > > > > <maxime.coquelin@redhat.com> wrote: > > > >> > > > >> > > > >> > > > >> On 4/20/23 06:34, Jason Wang wrote: > > > >>> On Wed, Apr 19, 2023 at 9:43 PM Maxime Coquelin > > > >>> <maxime.coquelin@redhat.com> wrote: > > > >>>> > > > >>>> This small series enables virtio-net device type in VDUSE. > > > >>>> With it, basic operation have been tested, both with > > > >>>> virtio-vdpa and vhost-vdpa using DPDK Vhost library series > > > >>>> adding VDUSE support [0] using split rings layout. > > > >>>> > > > >>>> Control queue support (and so multiqueue) has also been > > > >>>> tested, but require a Kernel series from Jason Wang > > > >>>> relaxing control queue polling [1] to function reliably. > > > >>>> > > > >>>> Other than that, we have identified a few gaps: > > > >>>> > > > >>>> 1. Reconnection: > > > >>>> a. VDUSE_VQ_GET_INFO ioctl() returns always 0 for avail > > > >>>> index, even after the virtqueue has already been > > > >>>> processed. Is that expected? I have tried instead to > > > >>>> get the driver's avail index directly from the avail > > > >>>> ring, but it does not seem reliable as I sometimes get > > > >>>> "id %u is not a head!\n" warnings. Also such solution > > > >>>> would not be possible with packed ring, as we need to > > > >>>> know the wrap counters values. > > > >>> > > > >>> Looking at the codes, it only returns the value that is set via > > > >>> set_vq_state(). I think it is expected to be called before the > > > >>> datapath runs. > > > >>> > > > >>> So when bound to virtio-vdpa, it is expected to return 0. But we need > > > >>> to fix the packed virtqueue case, I wonder if we need to call > > > >>> set_vq_state() explicitly in virtio-vdpa before starting the device. > > > >>> > > > >>> When bound to vhost-vdpa, Qemu will call VHOST_SET_VRING_BASE which > > > >>> will end up a call to set_vq_state(). Unfortunately, it doesn't > > > >>> support packed ring which needs some extension. > > > >>> > > > >>>> > > > >>>> b. Missing IOCTLs: it would be handy to have new IOCTLs to > > > >>>> query Virtio device status, > > > >>> > > > >>> What's the use case of this ioctl? It looks to me userspace is > > > >>> notified on each status change now: > > > >>> > > > >>> static int vduse_dev_set_status(struct vduse_dev *dev, u8 status) > > > >>> { > > > >>> struct vduse_dev_msg msg = { 0 }; > > > >>> > > > >>> msg.req.type = VDUSE_SET_STATUS; > > > >>> msg.req.s.status = status; > > > >>> > > > >>> return vduse_dev_msg_sync(dev, &msg); > > > >>> } > > > >> > > > >> The idea was to be able to query the status at reconnect time, and > > > >> neither having to assume its value nor having to store its value in a > > > >> file (the status could change while the VDUSE application is stopped, > > > >> but maybe it would receive the notification at reconnect). > > > > > > > > I see. > > > > > > > >> > > > >> I will prototype using a tmpfs file to save needed information, and see > > > >> if it works. > > > > > > > > It might work but then the API is not self contained. Maybe it's > > > > better to have a dedicated ioctl. > > > > > > > >> > > > >>>> and retrieve the config > > > >>>> space set at VDUSE_CREATE_DEV time. > > > >>> > > > >>> In order to be safe, VDUSE avoids writable config space. Otherwise > > > >>> drivers could block on config writing forever. That's why we don't do > > > >>> it now. > > > >> > > > >> The idea was not to make the config space writable, but just to be able > > > >> to fetch what was filled at VDUSE_CREATE_DEV time. > > > >> > > > >> With the tmpfs file, we can avoid doing that and just save the config > > > >> space there. > > > > > > > > Same as the case for status. > > > > > > I have cooked a DPDK patch to support reconnect with a tmpfs file as > > > suggested by Yongji: > > > > > > https://gitlab.com/mcoquelin/dpdk-next-virtio/-/commit/53913f2b1155b02c44d5d3d298aafd357e7a8c48 > > > > This seems tricky, for example for status: > > > > dev->log->status = dev->status; > > > > What if we crash here? > > > > The message will be re-sent by the kernel if it's not replied. But I > think it would be better if we can restore it via some ioctl. Yes, the point is, without a get ioctl, it's very hard to audit the code. Thanks > > Thanks, > Yongji >