Re: [PATCH net-next 2/2] vsock/virtio: avoid enqueue packets when work queue is empty
From: Stefano Garzarella <sgarzare@redhat.com>
Date: 2024-06-14 14:38:04
Also in:
kvm, virtualization
On Fri, Jun 14, 2024 at 03:55:43PM GMT, Luigi Leonardi wrote:
From: Marco Pinna <redacted> 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
Cool, thanks for this improvement! Can you also report your host CPU detail?
quoted hunk ↗ jump to hunk
Co-developed-by: Luigi Leonardi <redacted> Signed-off-by: Luigi Leonardi <redacted> Signed-off-by: Marco Pinna <redacted> --- net/vmw_vsock/virtio_transport.c | 32 ++++++++++++++++++++++++++++++-- 1 file changed, 30 insertions(+), 2 deletions(-)diff --git a/net/vmw_vsock/virtio_transport.c b/net/vmw_vsock/virtio_transport.c index c930235ecaec..e89bf87282b2 100644 --- a/net/vmw_vsock/virtio_transport.c +++ b/net/vmw_vsock/virtio_transport.c@@ -214,7 +214,9 @@ virtio_transport_send_pkt(struct sk_buff *skb){ struct virtio_vsock_hdr *hdr; struct virtio_vsock *vsock; + bool use_worker = true; int len = skb->len; + int ret = -1;
Please define ret in the block we use it. Also, we don't need to initialize it.
quoted hunk ↗ jump to hunk
hdr = virtio_vsock_hdr(skb);@@ -235,8 +237,34 @@ virtio_transport_send_pkt(struct sk_buff *skb)if (virtio_vsock_skb_reply(skb)) atomic_inc(&vsock->queued_replies); - virtio_vsock_skb_queue_tail(&vsock->send_pkt_queue, skb); - queue_work(virtio_vsock_workqueue, &vsock->send_pkt_work); + /* If the send_pkt_queue is empty there is no need to enqueue the packet.
We should clarify which queue. I mean we are always queueing the packet somewhere, or in the internal queue for the worker or in the virtqueue, so this comment is not really clear.
+ * Just put it on the ringbuff using virtio_transport_send_skb.
ringbuff? Do you mean virtqueue?
+ */ +
we can avoid this empty line.
+ if (skb_queue_empty_lockless(&vsock->send_pkt_queue)) {
+ bool restart_rx = false;
+ struct virtqueue *vq;... `int ret;` here.
+ + mutex_lock(&vsock->tx_lock); + + vq = vsock->vqs[VSOCK_VQ_TX]; + + ret = virtio_transport_send_skb(skb, vq, vsock, &restart_rx);
Ah, at the end we don't need `ret` at all. What about just `if (!virtio_transport_send_skb())`?
+ if (ret == 0) {
+ use_worker = false;
+ virtqueue_kick(vq);
+ }
+
+ mutex_unlock(&vsock->tx_lock);
+
+ if (restart_rx)
+ queue_work(virtio_vsock_workqueue, &vsock->rx_work);
+ }
+
+ if (use_worker) {
+ virtio_vsock_skb_queue_tail(&vsock->send_pkt_queue, skb);
+ queue_work(virtio_vsock_workqueue, &vsock->send_pkt_work);
+ }
out_rcu:
rcu_read_unlock();
--
2.45.2