Re: [RFC PATCH 2/9] vringh: remove vringh_iov and unite to vringh_kiov

From: Shunsuke Mie
Date: Tue Dec 27 2022 - 02:06:11 EST

2022年12月27日(火) 15:04 Jason Wang <jasowang@xxxxxxxxxx>:
>
> On Tue, Dec 27, 2022 at 10:25 AM Shunsuke Mie <mie@xxxxxxxxxx> wrote:
> >
> > struct vringh_iov is defined to hold userland addresses. However, to use
> > common function, __vring_iov, finally the vringh_iov converts to the
> > vringh_kiov with simple cast. It includes compile time check code to make
> > sure it can be cast correctly.
> >
> > To simplify the code, this patch removes the struct vringh_iov and unifies
> > APIs to struct vringh_kiov.
> >
> > Signed-off-by: Shunsuke Mie <mie@xxxxxxxxxx>
>
> While at this, I wonder if we need to go further, that is, switch to
> using an iov iterator instead of a vringh customized one.
I didn't see the iov iterator yet, thank you for informing me.
Is that iov_iter? https://lwn.net/Articles/625077/
> Thanks
>
> > ---
> > drivers/vhost/vringh.c | 32 ++++++------------------------
> > include/linux/vringh.h | 45 ++++--------------------------------------
> > 2 files changed, 10 insertions(+), 67 deletions(-)
> >
> > diff --git a/drivers/vhost/vringh.c b/drivers/vhost/vringh.c
> > index 828c29306565..aa3cd27d2384 100644
> > --- a/drivers/vhost/vringh.c
> > +++ b/drivers/vhost/vringh.c
> > @@ -691,8 +691,8 @@ EXPORT_SYMBOL(vringh_init_user);
> > * calling vringh_iov_cleanup() to release the memory, even on error!
> > */
> > int vringh_getdesc_user(struct vringh *vrh,
> > - struct vringh_iov *riov,
> > - struct vringh_iov *wiov,
> > + struct vringh_kiov *riov,
> > + struct vringh_kiov *wiov,
> > bool (*getrange)(struct vringh *vrh,
> > u64 addr, struct vringh_range *r),
> > u16 *head)
> > @@ -708,26 +708,6 @@ int vringh_getdesc_user(struct vringh *vrh,
> > if (err == vrh->vring.num)
> > return 0;
> >
> > - /* We need the layouts to be the identical for this to work */
> > - BUILD_BUG_ON(sizeof(struct vringh_kiov) != sizeof(struct vringh_iov));
> > - BUILD_BUG_ON(offsetof(struct vringh_kiov, iov) !=
> > - offsetof(struct vringh_iov, iov));
> > - BUILD_BUG_ON(offsetof(struct vringh_kiov, i) !=
> > - offsetof(struct vringh_iov, i));
> > - BUILD_BUG_ON(offsetof(struct vringh_kiov, used) !=
> > - offsetof(struct vringh_iov, used));
> > - BUILD_BUG_ON(offsetof(struct vringh_kiov, max_num) !=
> > - offsetof(struct vringh_iov, max_num));
> > - BUILD_BUG_ON(sizeof(struct iovec) != sizeof(struct kvec));
> > - BUILD_BUG_ON(offsetof(struct iovec, iov_base) !=
> > - offsetof(struct kvec, iov_base));
> > - BUILD_BUG_ON(offsetof(struct iovec, iov_len) !=
> > - offsetof(struct kvec, iov_len));
> > - BUILD_BUG_ON(sizeof(((struct iovec *)NULL)->iov_base)
> > - != sizeof(((struct kvec *)NULL)->iov_base));
> > - BUILD_BUG_ON(sizeof(((struct iovec *)NULL)->iov_len)
> > - != sizeof(((struct kvec *)NULL)->iov_len));
> > -
> > *head = err;
> > err = __vringh_iov(vrh, *head, (struct vringh_kiov *)riov,
> > (struct vringh_kiov *)wiov,
> > @@ -740,14 +720,14 @@ int vringh_getdesc_user(struct vringh *vrh,
> > EXPORT_SYMBOL(vringh_getdesc_user);
> >
> > /**
> > - * vringh_iov_pull_user - copy bytes from vring_iov.
> > + * vringh_iov_pull_user - copy bytes from vring_kiov.
> > * @riov: the riov as passed to vringh_getdesc_user() (updated as we consume)
> > * @dst: the place to copy.
> > * @len: the maximum length to copy.
> > *
> > * Returns the bytes copied <= len or a negative errno.
> > */
> > -ssize_t vringh_iov_pull_user(struct vringh_iov *riov, void *dst, size_t len)
> > +ssize_t vringh_iov_pull_user(struct vringh_kiov *riov, void *dst, size_t len)
> > {
> > return vringh_iov_xfer(NULL, (struct vringh_kiov *)riov,
> > dst, len, xfer_from_user);
> > @@ -755,14 +735,14 @@ ssize_t vringh_iov_pull_user(struct vringh_iov *riov, void *dst, size_t len)
> > EXPORT_SYMBOL(vringh_iov_pull_user);
> >
> > /**
> > - * vringh_iov_push_user - copy bytes into vring_iov.
> > + * vringh_iov_push_user - copy bytes into vring_kiov.
> > * @wiov: the wiov as passed to vringh_getdesc_user() (updated as we consume)
> > * @src: the place to copy from.
> > * @len: the maximum length to copy.
> > *
> > * Returns the bytes copied <= len or a negative errno.
> > */
> > -ssize_t vringh_iov_push_user(struct vringh_iov *wiov,
> > +ssize_t vringh_iov_push_user(struct vringh_kiov *wiov,
> > const void *src, size_t len)
> > {
> > return vringh_iov_xfer(NULL, (struct vringh_kiov *)wiov,
> > diff --git a/include/linux/vringh.h b/include/linux/vringh.h
> > index 1991a02c6431..733d948e8123 100644
> > --- a/include/linux/vringh.h
> > +++ b/include/linux/vringh.h
> > @@ -79,18 +79,6 @@ struct vringh_range {
> > u64 offset;
> > };
> >
> > -/**
> > - * struct vringh_iov - iovec mangler.
> > - *
> > - * Mangles iovec in place, and restores it.
> > - * Remaining data is iov + i, of used - i elements.
> > - */
> > -struct vringh_iov {
> > - struct iovec *iov;
> > - size_t consumed; /* Within iov[i] */
> > - unsigned i, used, max_num;
> > -};
> > -
> > /**
> > * struct vringh_kiov - kvec mangler.
> > *
> > @@ -113,44 +101,19 @@ int vringh_init_user(struct vringh *vrh, u64 features,
> > vring_avail_t __user *avail,
> > vring_used_t __user *used);
> >
> > -static inline void vringh_iov_init(struct vringh_iov *iov,
> > - struct iovec *iovec, unsigned num)
> > -{
> > - iov->used = iov->i = 0;
> > - iov->consumed = 0;
> > - iov->max_num = num;
> > - iov->iov = iovec;
> > -}
> > -
> > -static inline void vringh_iov_reset(struct vringh_iov *iov)
> > -{
> > - iov->iov[iov->i].iov_len += iov->consumed;
> > - iov->iov[iov->i].iov_base -= iov->consumed;
> > - iov->consumed = 0;
> > - iov->i = 0;
> > -}
> > -
> > -static inline void vringh_iov_cleanup(struct vringh_iov *iov)
> > -{
> > - if (iov->max_num & VRINGH_IOV_ALLOCATED)
> > - kfree(iov->iov);
> > - iov->max_num = iov->used = iov->i = iov->consumed = 0;
> > - iov->iov = NULL;
> > -}
> > -
> > /* Convert a descriptor into iovecs. */
> > int vringh_getdesc_user(struct vringh *vrh,
> > - struct vringh_iov *riov,
> > - struct vringh_iov *wiov,
> > + struct vringh_kiov *riov,
> > + struct vringh_kiov *wiov,
> > bool (*getrange)(struct vringh *vrh,
> > u64 addr, struct vringh_range *r),
> > u16 *head);
> >
> > /* Copy bytes from readable vsg, consuming it (and incrementing wiov->i). */
> > -ssize_t vringh_iov_pull_user(struct vringh_iov *riov, void *dst, size_t len);
> > +ssize_t vringh_iov_pull_user(struct vringh_kiov *riov, void *dst, size_t len);
> >
> > /* Copy bytes into writable vsg, consuming it (and incrementing wiov->i). */
> > -ssize_t vringh_iov_push_user(struct vringh_iov *wiov,
> > +ssize_t vringh_iov_push_user(struct vringh_kiov *wiov,
> > const void *src, size_t len);
> >
> > /* Mark a descriptor as used. */
> > --
> > 2.25.1
> >
>
Best,
Shunsuke