Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::3:4 as permitted sender) client-ip=2620:137:e000::3:4;
MIME-Version: 1.0
References: <20231205083444.3029239-1-lulu@redhat.com> <20231205083444.3029239-3-lulu@redhat.com>
In-Reply-To: <20231205083444.3029239-3-lulu@redhat.com>
From:   Jason Wang <jasowang@redhat.com>
Date:   Thu, 7 Dec 2023 16:46:18 +0800
Message-ID: <CACGkMEt26rDCQAH8Oh-pnEDFb-ShAMhL3kyejWGEkw_Sdo4OUw@mail.gmail.com>
Subject: Re: [PATCH v3 2/7] vduse: Add new uAPI for vduse reconnection
To:     Cindy Lu <lulu@redhat.com>
Cc:     mst@redhat.com, xieyongji@bytedance.com,
        linux-kernel@vger.kernel.org, maxime.coquelin@redhat.com
Content-Type: text/plain; charset="UTF-8"
Content-Transfer-Encoding: quoted-printable
Precedence: bulk

On Tue, Dec 5, 2023 at 4:35=E2=80=AFPM Cindy Lu <lulu@redhat.com> wrote:
>
> To synchronize the information for reconnection, add a new structure
> struct vduse_dev_reconnect_data to save the device-related information,
> Add the VDUSE_RECONNCT_MMAP_SIZE for the size of mapped memory for each v=
q
> and device status.
>
> Signed-off-by: Cindy Lu <lulu@redhat.com>
> ---
>  include/uapi/linux/vduse.h | 22 ++++++++++++++++++++++
>  1 file changed, 22 insertions(+)
>
> diff --git a/include/uapi/linux/vduse.h b/include/uapi/linux/vduse.h
> index 11bd48c72c6c..c22838247814 100644
> --- a/include/uapi/linux/vduse.h
> +++ b/include/uapi/linux/vduse.h
> @@ -350,4 +350,26 @@ struct vduse_dev_response {
>         };
>  };
>
> +/**
> + * struct vduse_dev_reconnect_data - saved the reconnect info for device
> + * @version; version for userspace APP

I guess it should be the version of the reconnection protocol not the
userspace application.

> + * @reconnected: indetify if this is reconnected.userspace APP needs set=
 this
> + *                to VDUSE_RECONNECT, while reconnecting.kernel will use=
 this
> + *                to indetify if this is reconnect

Typos, I think checkpatch may help or you can tweak your editor to
enable spell checkings.

> + * @features; Device features negotiated in the last connect.

This seems to cause confusion, let's use driver_features instead.

But can't we just get driver_features via VDUSE_DEV_GET_FEATURES?

> + * @status; Device status in last reconnect
> + */
> +
> +struct vduse_dev_reconnect_data {
> +       __u32 version;
> +#define VDUSE_RECONNECT 1
> +#define VDUSE_NOT_RECONNECT 0
> +       __u32 reconnected;
> +       __u64 features;
> +       __u8 status;
> +};

For status, VDUSE currently forwards the request to the userspace:

static int vduse_dev_set_status(struct vduse_dev *dev, u8 status)
{
        struct vduse_dev_msg msg =3D { 0 };

        msg.req.type =3D VDUSE_SET_STATUS;
        msg.req.s.status =3D status;

        return vduse_dev_msg_sync(dev, &msg);
}

Do we need to handle the case where the user space crashes when
dealing with this message? For example, driver set DRIVER_OK but VDUSE
application crashes when dealing with DRIVER_OK.

At the uAPI level, it probably requires to fetch inflight VDUSE
requests. It may help for the case where dealing with crash at feature
negotiation and configuration space access.

> +
> +/* the reconnection mmap size for each VQ and dev status */
> +#define VDUSE_RECONNCT_MMAP_SIZE PAGE_SIZE

PAGE_SIZE should not belong to uAPI. Userspace can query it via e.g sysconf=
 etc.

Btw, what is the virtqueue part for this? I'd expect there should be
at least the part of inflight descriptors?


> +
>  #endif /* _UAPI_VDUSE_H_ */
> --
> 2.34.3
>