Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 23.128.96.33 as permitted sender) client-ip=23.128.96.33;
MIME-Version: 1.0
References: <20231121073050.287080-1-lulu@redhat.com> <20231121073050.287080-4-lulu@redhat.com>
 <CACGkMEtzXS8GwRN_anprGEEJhoGxSbj8pBCTF22RPVL484fhYA@mail.gmail.com>
In-Reply-To: <CACGkMEtzXS8GwRN_anprGEEJhoGxSbj8pBCTF22RPVL484fhYA@mail.gmail.com>
From:   Cindy Lu <lulu@redhat.com>
Date:   Sat, 25 Nov 2023 23:52:27 +0800
Message-ID: <CACLfguV8MF+JO0QkHZ+zJV3ceZvhTqiBz0kAU6AQQHPc3jL_vA@mail.gmail.com>
Subject: Re: [PATCH v2 3/5] vduse: Add new ioctl VDUSE_GET_RECONNECT_INFO
To:     Jason Wang <jasowang@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, Nov 22, 2023 at 2:29=E2=80=AFPM Jason Wang <jasowang@redhat.com> wr=
ote:
>
> 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_use=
r/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, u=
nsigned 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?
sure we can remove this IOCTL
>
> > +
> > +               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 =
set this bit to 1
> > + *               while reconnecting.kernel will use this bit to indeti=
fy if this is
> > + *               reconnect
>
> Typos.
>
will fix this
> > + * @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?
>
will remove virtio_net_config here
> her
> > +
> > +/**
> > + * 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_co=
unter
>
> 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?
>
I didn't get here, Do you mean set_vq_state/get_vq_state ?
Thanks
Cindy
> 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
> >
>