Re: [PATCH net-next 2/2] vsock/virtio: avoid enqueue packets when work queue is empty
Hello, thanks for working on this! I have some minor thoughts. On Fri, Jun 14, 2024 at 03:55:43PM +0200, Luigi Leonardi wrote: > From: Marco Pinna > > This introduces an optimization in virtio_transport_send_pkt: > when the work queue (send_pkt_queue) is empty the packet is > put directly in the virtqueue reducing latency. > > In the following benchmark (pingpong mode) the host sends > a payload to the guest and waits for the same payload back. > > Tool: Fio version 3.37-56 > Env: Phys host + L1 Guest > Payload: 4k > Runtime-per-test: 50s > Mode: pingpong (h-g-h) > Test runs: 50 > Type: SOCK_STREAM > > Before (Linux 6.8.11) > -- > mean(1st percentile): 722.45 ns > mean(overall): 1686.23 ns > mean(99th percentile): 35379.27 ns > > After > -- > mean(1st percentile): 602.62 ns > mean(overall): 1248.83 ns > mean(99th percentile): 17557.33 ns > I think It would be interesting to know what exactly the test does, and, if the test is triggering the improvement, i.e., the better results are due to enqueuing packets directly to the virtqueue instead of letting the worker does it. If I understand correctly, this patch focuses on the case in which the worker queue is empty. I think the test can always send packets at a frequency so the worker queue is always empty, but maybe, this is a corner case and most of the time the worker queue is not empty in a non-testing environment. Matias
Re: [PATCH net-next 2/2] vsock/virtio: avoid enqueue packets when work queue is empty
On Tue, Jun 18, 2024 at 07:05:54PM +0200, Luigi Leonardi wrote: > Hi Stefano and Matias, > > @Stefano Thanks for your review(s)! I'll send a V2 by the end of the week. > > @Matias > > Thanks for your feedback! > > > I think It would be interesting to know what exactly the test does > > It's relatively easy: I used fio's pingpong mode. This mode is specifically > for measuring the latency, the way it works is by sending packets, > in my case, from the host to the guest. and waiting for the other side > to send them back. The latency I wrote in the commit is the "completion > latency". The total throughput on my system is around 16 Gb/sec. > Thanks for the explanation! > > if the test is triggering the improvement > > Yes! I did some additional testing and I can confirm you that during this > test, the worker queue is never used! > Cool. > > If I understand correctly, this patch focuses on the > > case in which the worker queue is empty > > Correct! > > > I think the test can always send packets at a frequency so the worker queue > > is always empty. but maybe, this is a corner case and most of the time the > > worker queue is not empty in a non-testing environment. > > I'm not sure about this, but IMHO this optimization is free, there is no > penalty for using it, in the worst case the system will work as usual. > In any case, I'm more than happy to do some additional testing, do you have > anything in mind? > Sure!, this is very a interesting improvement and I am in favor for that! I was only thinking out loud ;) I asked previous questions because, in my mind, I was thinking that this improvement would trigger only for the first bunch of packets, i.e., when the worker queue is empty so its effect would be seen "only at the beginning of the transmission" until the worker-queue begins to fill. If I understand correctly, the worker-queue starts to fill just after the virtqueue is full, am I right? Matias
[PATCH net-next v2] vsock/virtio: add support for MSG_PEEK
This patch adds support for MSG_PEEK. In such a case, packets are not
removed from the rx_queue and credit updates are not sent.
Signed-off-by: Matias Ezequiel Vara Larsen
Reviewed-by: Stefano Garzarella
Tested-by: Stefano Garzarella
---
net/vmw_vsock/virtio_transport_common.c | 55 +++--
1 file changed, 52 insertions(+), 3 deletions(-)
diff --git a/net/vmw_vsock/virtio_transport_common.c
b/net/vmw_vsock/virtio_transport_common.c
index 94cc0fa..cf15751 100644
--- a/net/vmw_vsock/virtio_transport_common.c
+++ b/net/vmw_vsock/virtio_transport_common.c
@@ -264,6 +264,55 @@ static int virtio_transport_send_credit_update(struct
vsock_sock *vsk,
}
static ssize_t
+virtio_transport_stream_do_peek(struct vsock_sock *vsk,
+ struct msghdr *msg,
+ size_t len)
+{
+ struct virtio_vsock_sock *vvs = vsk->trans;
+ struct virtio_vsock_pkt *pkt;
+ size_t bytes, total = 0, off;
+ int err = -EFAULT;
+
+ spin_lock_bh(&vvs->rx_lock);
+
+ list_for_each_entry(pkt, &vvs->rx_queue, list) {
+ off = pkt->off;
+
+ if (total == len)
+ break;
+
+ while (total < len && off < pkt->len) {
+ bytes = len - total;
+ if (bytes > pkt->len - off)
+ bytes = pkt->len - off;
+
+ /* sk_lock is held by caller so no one else can dequeue.
+* Unlock rx_lock since memcpy_to_msg() may sleep.
+*/
+ spin_unlock_bh(&vvs->rx_lock);
+
+ err = memcpy_to_msg(msg, pkt->buf + off, bytes);
+ if (err)
+ goto out;
+
+ spin_lock_bh(&vvs->rx_lock);
+
+ total += bytes;
+ off += bytes;
+ }
+ }
+
+ spin_unlock_bh(&vvs->rx_lock);
+
+ return total;
+
+out:
+ if (total)
+ err = total;
+ return err;
+}
+
+static ssize_t
virtio_transport_stream_do_dequeue(struct vsock_sock *vsk,
struct msghdr *msg,
size_t len)
@@ -330,9 +379,9 @@ virtio_transport_stream_dequeue(struct vsock_sock *vsk,
size_t len, int flags)
{
if (flags & MSG_PEEK)
- return -EOPNOTSUPP;
-
- return virtio_transport_stream_do_dequeue(vsk, msg, len);
+ return virtio_transport_stream_do_peek(vsk, msg, len);
+ else
+ return virtio_transport_stream_do_dequeue(vsk, msg, len);
}
EXPORT_SYMBOL_GPL(virtio_transport_stream_dequeue);
--
2.7.4
