Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 23.128.96.31 as permitted sender) client-ip=23.128.96.31;
MIME-Version: 1.0
References: <20231205083444.3029239-1-lulu@redhat.com> <20231205083444.3029239-7-lulu@redhat.com>
In-Reply-To: <20231205083444.3029239-7-lulu@redhat.com>
From:   Jason Wang <jasowang@redhat.com>
Date:   Thu, 7 Dec 2023 16:46:15 +0800
Message-ID: <CACGkMEvycvCUjjdKvK50dmoQbDL79Xc9s4YDMVed9BUv+Gf0yg@mail.gmail.com>
Subject: Re: [PATCH v3 6/7] vduse: Update the vq_info in ioctl VDUSE_VQ_GET_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, Dec 5, 2023 at 4:35=E2=80=AFPM Cindy Lu <lulu@redhat.com> wrote:
>
> Once the reconnect memory pages are mapped to userspace, the userspace
> application will update the "reconnected" bit in the
> "struct vduse_dev_reconnect_data".
> The kernel will then check this bit. If it is not set to
> "VDUSE_NOT_RECONNECT", it means that the application has been
> reconnected, and the kernel will synchronize the vq information.

Can you explain why we need such a flag?

>
> Signed-off-by: Cindy Lu <lulu@redhat.com>
> ---
>  drivers/vdpa/vdpa_user/vduse_dev.c | 24 +++++++++++++++++++++++-
>  1 file changed, 23 insertions(+), 1 deletion(-)
>
> diff --git a/drivers/vdpa/vdpa_user/vduse_dev.c b/drivers/vdpa/vdpa_user/=
vduse_dev.c
> index f55f415629de..422f1aedebac 100644
> --- a/drivers/vdpa/vdpa_user/vduse_dev.c
> +++ b/drivers/vdpa/vdpa_user/vduse_dev.c
> @@ -1193,6 +1193,9 @@ static long vduse_dev_ioctl(struct file *file, unsi=
gned int cmd,
>                 struct vduse_vq_info vq_info;
>                 struct vduse_virtqueue *vq;
>                 u32 index;
> +               unsigned long vaddr;
> +               struct vduse_vq_state *vq_reconnect;
> +               struct vduse_dev_reconnect_data *dev_reconnect;
>
>                 ret =3D -EFAULT;
>                 if (copy_from_user(&vq_info, argp, sizeof(vq_info)))
> @@ -1209,6 +1212,12 @@ static long vduse_dev_ioctl(struct file *file, uns=
igned int cmd,
>                 vq_info.device_addr =3D vq->device_addr;
>                 vq_info.num =3D vq->num;
>
> +               vaddr =3D dev->vdpa_reconnect_vaddr;
> +               dev_reconnect =3D (struct vduse_dev_reconnect_data *)vadd=
r;
> +
> +               vaddr =3D vq->vdpa_reconnect_vaddr;
> +               vq_reconnect =3D (struct vduse_vq_state *)vaddr;
> +
>                 if (dev->driver_features & BIT_ULL(VIRTIO_F_RING_PACKED))=
 {
>                         vq_info.packed.last_avail_counter =3D
>                                 vq->state.packed.last_avail_counter;
> @@ -1218,9 +1227,22 @@ static long vduse_dev_ioctl(struct file *file, uns=
igned int cmd,
>                                 vq->state.packed.last_used_counter;
>                         vq_info.packed.last_used_idx =3D
>                                 vq->state.packed.last_used_idx;
> -               } else
> +                       /*check if the vq is reconnect, if yes then updat=
e the info*/
> +                       if (dev_reconnect->reconnected !=3D VDUSE_NOT_REC=
ONNECT) {

So the dev_reconnect is shared, how to synchronize between them?

> +                               vq_info.packed.last_avail_idx =3D
> +                                       vq_reconnect->packed.last_avail_i=
dx;
> +                               vq_info.packed.last_avail_counter =3D
> +                                       vq_reconnect->packed.last_avail_c=
ounter;
> +                       }
> +               } else {
>                         vq_info.split.avail_index =3D
>                                 vq->state.split.avail_index;
> +                       /*check if the vq is reconnect, if yes then updat=
e the info*/
> +                       if (dev_reconnect->reconnected !=3D VDUSE_NOT_REC=
ONNECT) {

If this only check is !=3D, I'd define it as VDUSE_RECONNECT to ease the
code logic.

> +                               vq_info.split.avail_index =3D
> +                                       vq_reconnect->split.avail_index;
> +                       }
> +               }

It looks like I miss something here

If I read the code correctly, vq_reconnect is written by userspace, so
userspace query it again via the get_vq_info()? If this is true,
userspace can just ignore the index via GET_VQ_INFO() or is it just
for consistency?

Thanks


>
>                 vq_info.ready =3D vq->ready;
>
> --
> 2.34.3
>