Message-ID: <CACGkMEvrrENCeBFkVeBDgvPo=dizcxA9EOaLeN7MWTH_qjnnfQ@mail.gmail.com>
Date: Thu, 20 Mar 2025 09:41:37 +0800
From: Jason Wang <jasowang@...hat.com>
To: Dongli Zhang <dongli.zhang@...cle.com>
Cc: virtualization@...ts.linux.dev, kvm@...r.kernel.org, 
	netdev@...r.kernel.org, mst@...hat.com, michael.christie@...cle.com, 
	pbonzini@...hat.com, stefanha@...hat.com, eperezma@...hat.com, 
	joao.m.martins@...cle.com, joe.jin@...cle.com, si-wei.liu@...cle.com, 
	linux-kernel@...r.kernel.org
Subject: Re: [PATCH v2 04/10] vhost: modify vhost_log_write() for broader users

On Thu, Mar 20, 2025 at 12:38 AM Dongli Zhang <dongli.zhang@...cle.com> wrote:
>
> Hi Jason,
>
> On 3/17/25 6:12 PM, Jason Wang wrote:
> > On Tue, Mar 18, 2025 at 7:51 AM Dongli Zhang <dongli.zhang@...cle.com> wrote:
> >>
> >> Currently, the only user of vhost_log_write() is vhost-net. The 'len'
> >> argument prevents logging of pages that are not tainted by the RX path.
> >>
> >> Adjustments are needed since more drivers (i.e. vhost-scsi) begin using
> >> vhost_log_write(). So far vhost-net RX path may only partially use pages
> >> shared by the last vring descriptor. Unlike vhost-net, vhost-scsi always
> >> logs all pages shared via vring descriptors. To accommodate this, a new
> >> argument 'partial' is introduced. This argument works alongside 'len' to
> >> indicate whether the driver should log all pages of a vring descriptor, or
> >> only pages that are tainted by the driver.
> >>
> >> In addition, removes BUG().
> >>
> >> Suggested-by: Joao Martins <joao.m.martins@...cle.com>
> >> Signed-off-by: Dongli Zhang <dongli.zhang@...cle.com>
> >> ---
> >>  drivers/vhost/net.c   |  2 +-
> >>  drivers/vhost/vhost.c | 28 +++++++++++++++++-----------
> >>  drivers/vhost/vhost.h |  2 +-
> >>  3 files changed, 19 insertions(+), 13 deletions(-)
> >>
> >> diff --git a/drivers/vhost/net.c b/drivers/vhost/net.c
> >> index b9b9e9d40951..0e5d82bfde76 100644
> >> --- a/drivers/vhost/net.c
> >> +++ b/drivers/vhost/net.c
> >> @@ -1219,7 +1219,7 @@ static void handle_rx(struct vhost_net *net)
> >>                 if (nvq->done_idx > VHOST_NET_BATCH)
> >>                         vhost_net_signal_used(nvq);
> >>                 if (unlikely(vq_log))
> >> -                       vhost_log_write(vq, vq_log, log, vhost_len,
> >> +                       vhost_log_write(vq, vq_log, log, vhost_len, true,
> >>                                         vq->iov, in);
> >>                 total_len += vhost_len;
> >>         } while (likely(!vhost_exceeds_weight(vq, ++recv_pkts, total_len)));
> >> diff --git a/drivers/vhost/vhost.c b/drivers/vhost/vhost.c
> >> index 9ac25d08f473..db3b30aba940 100644
> >> --- a/drivers/vhost/vhost.c
> >> +++ b/drivers/vhost/vhost.c
> >> @@ -2304,8 +2304,14 @@ static int log_used(struct vhost_virtqueue *vq, u64 used_offset, u64 len)
> >>         return 0;
> >>  }
> >>
> >> -int vhost_log_write(struct vhost_virtqueue *vq, struct vhost_log *log,
> >> -                   unsigned int log_num, u64 len, struct iovec *iov, int count)
> >> +/*
> >> + * 'len' is used only when 'partial' is true, to indicate whether the
> >> + * entire length of each descriptor is logged.
> >> + */
> >
> > While at it, let's document all the parameters here.
>
> Sure.
>
> >
> >> +int vhost_log_write(struct vhost_virtqueue *vq,
> >> +                   struct vhost_log *log, unsigned int log_num,
> >> +                   u64 len, bool partial,
> >> +                   struct iovec *iov, int count)
> >>  {
> >>         int i, r;
> >>
> >> @@ -2323,19 +2329,19 @@ int vhost_log_write(struct vhost_virtqueue *vq, struct vhost_log *log,
> >>         }
> >>
> >>         for (i = 0; i < log_num; ++i) {
> >> -               u64 l = min(log[i].len, len);
> >> +               u64 l = partial ? min(log[i].len, len) : log[i].len;
> >> +
> >>                 r = log_write(vq->log_base, log[i].addr, l);
> >>                 if (r < 0)
> >>                         return r;
> >> -               len -= l;
> >> -               if (!len) {
> >> -                       if (vq->log_ctx)
> >> -                               eventfd_signal(vq->log_ctx);
> >> -                       return 0;
> >> -               }
> >> +
> >> +               if (partial)
> >> +                       len -= l;
> >
> > I wonder if it's simpler to just tweak the caller to call with the
> > correct len (or probably U64_MAX) in this case?
>
> To "tweak the caller to call with the correct len" may need to sum the length
> of all log[i].
>
> Regarding U64_MAX, would you like something below? That is, only use 'len'
> when it isn't U64_MAX.
>
> diff --git a/drivers/vhost/vhost.c b/drivers/vhost/vhost.c
> index 9ac25d08f473..5b49de05e752 100644
> --- a/drivers/vhost/vhost.c
> +++ b/drivers/vhost/vhost.c
> @@ -2327,15 +2327,14 @@ int vhost_log_write(struct vhost_virtqueue *vq, struct vhost_log *log,
>                 r = log_write(vq->log_base, log[i].addr, l);
>                 if (r < 0)
>                         return r;
> -               len -= l;
> -               if (!len) {
> -                       if (vq->log_ctx)
> -                               eventfd_signal(vq->log_ctx);
> -                       return 0;
> -               }
> +
> +               if (len != U64_MAX) ---> It is impossible to have len = U64_MAX from vhost-net
> +                       len -= l;        How about keeping those two lines?
>         }
> -       /* Length written exceeds what we have stored. This is a bug. */
> -       BUG();
> +
> +       if (vq->log_ctx)
> +               eventfd_signal(vq->log_ctx);
> +
>         return 0;
>  }
>  EXPORT_SYMBOL_GPL(vhost_log_write);

Something like this.

Thanks

>
>
> Thank you very much!
>
> Dongli Zhang
>

Powered by blists - more mailing lists