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@oracle.com>
Signed-off-by: Dongli Zhang <dongli.zhang@oracle.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.
+ */
+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;
}
- /* 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);
diff --git a/drivers/vhost/vhost.h b/drivers/vhost/vhost.h
index bb75a292d50c..5de5941988fe 100644
--- a/drivers/vhost/vhost.h
+++ b/drivers/vhost/vhost.h
@@ -224,7 +224,7 @@ bool vhost_vq_avail_empty(struct vhost_dev *, struct vhost_virtqueue *);
bool vhost_enable_notify(struct vhost_dev *, struct vhost_virtqueue *);
int vhost_log_write(struct vhost_virtqueue *vq, struct vhost_log *log,
- unsigned int log_num, u64 len,
+ unsigned int log_num, u64 len, bool partial,
struct iovec *iov, int count);
int vq_meta_prefetch(struct vhost_virtqueue *vq);
--
2.39.3On Tue, Mar 18, 2025 at 7:51 AM Dongli Zhang <dongli.zhang@oracle.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@oracle.com>
> Signed-off-by: Dongli Zhang <dongli.zhang@oracle.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.
> +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?
> }
> - /* 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);
> diff --git a/drivers/vhost/vhost.h b/drivers/vhost/vhost.h
> index bb75a292d50c..5de5941988fe 100644
> --- a/drivers/vhost/vhost.h
> +++ b/drivers/vhost/vhost.h
> @@ -224,7 +224,7 @@ bool vhost_vq_avail_empty(struct vhost_dev *, struct vhost_virtqueue *);
> bool vhost_enable_notify(struct vhost_dev *, struct vhost_virtqueue *);
>
> int vhost_log_write(struct vhost_virtqueue *vq, struct vhost_log *log,
> - unsigned int log_num, u64 len,
> + unsigned int log_num, u64 len, bool partial,
> struct iovec *iov, int count);
> int vq_meta_prefetch(struct vhost_virtqueue *vq);
>
> --
> 2.39.3
>
Thanks
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@oracle.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@oracle.com>
>> Signed-off-by: Dongli Zhang <dongli.zhang@oracle.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);
Thank you very much!
Dongli Zhang
On Thu, Mar 20, 2025 at 12:38 AM Dongli Zhang <dongli.zhang@oracle.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@oracle.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@oracle.com>
> >> Signed-off-by: Dongli Zhang <dongli.zhang@oracle.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
>
© 2016 - 2025 Red Hat, Inc.