Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::3:2 as permitted sender) client-ip=2620:137:e000::3:2;
MIME-Version: 1.0
References: <20231121073050.287080-1-lulu@redhat.com> <20231121073050.287080-4-lulu@redhat.com>
In-Reply-To: <20231121073050.287080-4-lulu@redhat.com>
From:   Jason Wang <jasowang@redhat.com>
Date:   Wed, 22 Nov 2023 14:28:53 +0800
Message-ID: <CACGkMEtzXS8GwRN_anprGEEJhoGxSbj8pBCTF22RPVL484fhYA@mail.gmail.com>
Subject: Re: [PATCH v2 3/5] vduse: Add new ioctl VDUSE_GET_RECONNECT_INFO
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, Nov 21, 2023 at 3:31=E2=80=AFPM Cindy Lu <lulu@redhat.com> wrote:
>
> In VDUSE_GET_RECONNECT_INFO, the Userspace App can get the map size
> and The number of mapping memory pages from the kernel. The userspace
> App can use this information to map the pages.
>
> Add struct vhost_reconnect_data/vhost_reconnect_vring for sync the
> information in reconnection
>
> Signed-off-by: Cindy Lu <lulu@redhat.com>
> ---
>  drivers/vdpa/vdpa_user/vduse_dev.c | 15 +++++++++
>  include/uapi/linux/vduse.h         | 50 ++++++++++++++++++++++++++++++
>  2 files changed, 65 insertions(+)
>
> diff --git a/drivers/vdpa/vdpa_user/vduse_dev.c b/drivers/vdpa/vdpa_user/=
vduse_dev.c
> index ccb30e98bab5..d0fe9a7e86ab 100644
> --- a/drivers/vdpa/vdpa_user/vduse_dev.c
> +++ b/drivers/vdpa/vdpa_user/vduse_dev.c
> @@ -1343,6 +1343,21 @@ static long vduse_dev_ioctl(struct file *file, uns=
igned int cmd,
>                 ret =3D 0;
>                 break;
>         }
> +       case VDUSE_GET_RECONNECT_INFO: {
> +               struct vduse_reconnect_mmap_info info;
> +
> +               ret =3D -EFAULT;
> +               if (copy_from_user(&info, argp, sizeof(info)))
> +                       break;
> +
> +               info.size =3D PAGE_SIZE;
> +               info.max_index =3D dev->vq_num + 1;

It looks to be both PAGE_SIZE and vq_num is the well knowledge that
doesn't require a query?

> +
> +               if (copy_to_user(argp, &info, sizeof(info)))
> +                       break;
> +               ret =3D 0;
> +               break;
> +       }
>         default:
>                 ret =3D -ENOIOCTLCMD;
>                 break;
> diff --git a/include/uapi/linux/vduse.h b/include/uapi/linux/vduse.h
> index 11bd48c72c6c..c0b7133aebfd 100644
> --- a/include/uapi/linux/vduse.h
> +++ b/include/uapi/linux/vduse.h
> @@ -3,6 +3,7 @@
>  #define _UAPI_VDUSE_H_
>
>  #include <linux/types.h>
> +#include <linux/virtio_net.h>
>
>  #define VDUSE_BASE     0x81
>
> @@ -350,4 +351,53 @@ struct vduse_dev_response {
>         };
>  };
>
> +/**
> + * struct vhost_reconnect_data - saved the reconnect info for device
> + * @version; version for userspace APP
> + * @reconnected: indetify if this is reconnected. userspace APP needs se=
t this bit to 1
> + *               while reconnecting.kernel will use this bit to indetify=
 if this is
> + *               reconnect

Typos.

> + * @features; Device features negotiated in the last connect.
> + * @status; Device status in last reconnect
> + * @nr_vrings; number of active vqs in last connect

What's the meaning of "active vqs"? Is it the #active_queue_pairs? If
yes, couldn't we get it from the virtio_net_config?

> + * @struct virtio_net_config config; the config in last connect
> + */
> +
> +struct vhost_reconnect_data {
> +       __u32 version;
> +       __u32 reconnected;
> +       __u64 features;
> +       __u8 status;
> +       __u32 nr_vrings;
> +       struct virtio_net_config config;

This seems like a layer violation. The fields here needs to be type
agnostic or we should introduce a new device specific area with a
union.

Or can we simply invent VDUSE_DEV_GET_CONFIG() to do this?

> +};
> +
> +/**
> + * struct vhost_reconnect_vring -saved the reconnect info for vqs
> + * when use split mode will only use last_avail_idx
> + * when use packed mode will use both last_avail_idx and avail_wrap_coun=
ter

How about last_used_idx and last_used_wrap_counter.

Btw, vDPA or vhost-vDPA has a good uAPI for this, can we reuse that?

Thanks

> + * userspace App
> + * @last_avail_idx: device last available index
> + * @avail_wrap_counter: Driver ring wrap counter
> + */
> +struct vhost_reconnect_vring {
> +       __u16 last_avail_idx;
> +       __u16 avail_wrap_counter;
> +};
> +
> +/**
> + * struct vduse_reconnect_mmap_info
> + * @size: mapping memory size, here we use page_size
> + * @max_index: the number of pages allocated in kernel,just
> + * use for sanity check
> + */
> +
> +struct vduse_reconnect_mmap_info {
> +       __u32 size;
> +       __u32 max_index;
> +};
> +
> +#define VDUSE_GET_RECONNECT_INFO \
> +       _IOWR(VDUSE_BASE, 0x1b, struct vduse_reconnect_mmap_info)
> +
>  #endif /* _UAPI_VDUSE_H_ */
> --
> 2.34.3
>