Re: [PATCH v3 2/7] vduse: Add new uAPI for vduse reconnection

[Jason Wang]

On Tue, Dec 5, 2023 at 4:35 PM Cindy Lu <lulu@xxxxxxxxxx> 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 vq
> and device status.
>
> Signed-off-by: Cindy Lu <lulu@xxxxxxxxxx>
> ---
>  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 = { 0 };

        msg.req.type = VDUSE_SET_STATUS;
        msg.req.s.status = 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
>