Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::3:1 as permitted sender) client-ip=2620:137:e000::3:1;
MIME-Version: 1.0
References: <20231011064208.2143245-1-lulu@redhat.com> <20231011064208.2143245-4-lulu@redhat.com>
In-Reply-To: <20231011064208.2143245-4-lulu@redhat.com>
From:   Jason Wang <jasowang@redhat.com>
Date:   Wed, 18 Oct 2023 11:23:06 +0800
Message-ID: <CACGkMEtuavpV=D0CPad2EhaQ_Xkr6TNa2jgFer-JO2SwSvyoeg@mail.gmail.com>
Subject: Re: [PATCH v1 3/4] 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 Wed, Oct 11, 2023 at 2:42=E2=80=AFPM Cindy Lu <lulu@redhat.com> wrote:
>
> In VDUSE_GET_RECONNECT_INFO, the Userspace App can get the map size

I'm not sure why this is needed, the structure that mmaped to
userspace should belong to uAPI then userspace can do versions there?

> and The number of mapping memory pages from the kernel.

Userspace knows how many vq at most, why is this still needed?

> 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         | 43 ++++++++++++++++++++++++++++++
>  2 files changed, 58 insertions(+)
>
> diff --git a/drivers/vdpa/vdpa_user/vduse_dev.c b/drivers/vdpa/vdpa_user/=
vduse_dev.c
> index 05e72d752fb6..0f15e7ac716b 100644
> --- a/drivers/vdpa/vdpa_user/vduse_dev.c
> +++ b/drivers/vdpa/vdpa_user/vduse_dev.c
> @@ -1347,6 +1347,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;
> +
> +               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..5ccac535fba6 100644
> --- a/include/uapi/linux/vduse.h
> +++ b/include/uapi/linux/vduse.h
> @@ -350,4 +350,47 @@ struct vduse_dev_response {
>         };
>  };
>
> +/**
> + * struct vhost_reconnect_data - saved the reconnect info for device
> + * @reconnect_time: reconnect time for this device. userspace APP needs =
to do ++
> + *                  while reconnecting

This commnet needs tweaking as I don't think "++" is good English. And
you need to explain why we need this.

> + * @version; version for userspace APP

It's the version of uAPI not the userspace APP. And in order to be
able for a correct bootstrap, this needs to be the first field instead
of the second.

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

I wonder if we need just more than this, for example the number of
active queues, or this is what does @nr_vrings mean?

> + * @nr_vrings; number of vqs
> + */
> +
> +struct vhost_reconnect_data {
> +       __u32 reconnect_time;
> +       __u32 version;
> +       __u64 features;
> +       __u8 status;
> +       __u32 nr_vrings;
> +};
> +
> +/**
> + * struct vhost_reconnect_vring -saved the reconnect info for vqs
> + * @last_avail_idx: device available index
> + * @avail_wrap_counter: Driver ring wrap counter
> + */
> +struct vhost_reconnect_vring {
> +       __u16 last_avail_idx;
> +       __u16 avail_wrap_counter;
> +};

Do we need the last_used_idx and last_used_wrap_counter? If not,
please document why.

> +
> +/**
> + * 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 check

Did you mean "sanity check"?

Thanks

> + */
> +
> +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
>