Re: [PATCH v2 04/10] vhost: modify vhost_log_write() for broader users

[Dongli Zhang]

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@xxxxxxxxxx> 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@xxxxxxxxxx>
>> Signed-off-by: Dongli Zhang <dongli.zhang@xxxxxxxxxx>
>> ---
>>  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