* [PATCH net-next 1/2] vsock/virtio: refactor virtio_transport_send_pkt_work [not found] <20240614135543.31515-1-luigi.leonardi@outlook.com> @ 2024-06-14 13:55 ` Luigi Leonardi 2024-06-14 14:37 ` Stefano Garzarella 2024-06-14 13:55 ` [PATCH net-next 2/2] vsock/virtio: avoid enqueue packets when work queue is empty Luigi Leonardi 1 sibling, 1 reply; 8+ messages in thread From: Luigi Leonardi @ 2024-06-14 13:55 UTC (permalink / raw) To: sgarzare, edumazet, virtualization, netdev, kuba, kvm, stefanha, pabeni, davem Cc: Marco Pinna, Luigi Leonardi From: Marco Pinna <marco.pinn95@gmail.com> This is a preliminary patch to introduce an optimization to the enqueue system. All the code used to enqueue a packet into the virtqueue is removed from virtio_transport_send_pkt_work() and moved to the new virtio_transport_send_skb() function. Co-developed-by: Luigi Leonardi <luigi.leonardi@outlook.com> Signed-off-by: Luigi Leonardi <luigi.leonardi@outlook.com> Signed-off-by: Marco Pinna <marco.pinn95@gmail.com> --- net/vmw_vsock/virtio_transport.c | 134 +++++++++++++++++-------------- 1 file changed, 74 insertions(+), 60 deletions(-) diff --git a/net/vmw_vsock/virtio_transport.c b/net/vmw_vsock/virtio_transport.c index 43d405298857..c930235ecaec 100644 --- a/net/vmw_vsock/virtio_transport.c +++ b/net/vmw_vsock/virtio_transport.c @@ -94,6 +94,78 @@ static u32 virtio_transport_get_local_cid(void) return ret; } +/* Caller need to hold vsock->tx_lock on vq */ +static int virtio_transport_send_skb(struct sk_buff *skb, struct virtqueue *vq, + struct virtio_vsock *vsock, bool *restart_rx) +{ + int ret, in_sg = 0, out_sg = 0; + struct scatterlist **sgs; + bool reply; + + reply = virtio_vsock_skb_reply(skb); + sgs = vsock->out_sgs; + sg_init_one(sgs[out_sg], virtio_vsock_hdr(skb), + sizeof(*virtio_vsock_hdr(skb))); + out_sg++; + + if (!skb_is_nonlinear(skb)) { + if (skb->len > 0) { + sg_init_one(sgs[out_sg], skb->data, skb->len); + out_sg++; + } + } else { + struct skb_shared_info *si; + int i; + + /* If skb is nonlinear, then its buffer must contain + * only header and nothing more. Data is stored in + * the fragged part. + */ + WARN_ON_ONCE(skb_headroom(skb) != sizeof(*virtio_vsock_hdr(skb))); + + si = skb_shinfo(skb); + + for (i = 0; i < si->nr_frags; i++) { + skb_frag_t *skb_frag = &si->frags[i]; + void *va; + + /* We will use 'page_to_virt()' for the userspace page + * here, because virtio or dma-mapping layers will call + * 'virt_to_phys()' later to fill the buffer descriptor. + * We don't touch memory at "virtual" address of this page. + */ + va = page_to_virt(skb_frag_page(skb_frag)); + sg_init_one(sgs[out_sg], + va + skb_frag_off(skb_frag), + skb_frag_size(skb_frag)); + out_sg++; + } + } + + ret = virtqueue_add_sgs(vq, sgs, out_sg, in_sg, skb, GFP_KERNEL); + /* Usually this means that there is no more space available in + * the vq + */ + if (ret < 0) + goto out; + + virtio_transport_deliver_tap_pkt(skb); + + if (reply) { + struct virtqueue *rx_vq = vsock->vqs[VSOCK_VQ_RX]; + int val; + + val = atomic_dec_return(&vsock->queued_replies); + + /* Do we now have resources to resume rx processing? */ + if (val + 1 == virtqueue_get_vring_size(rx_vq)) + *restart_rx = true; + } + +out: + return ret; +} + static void virtio_transport_send_pkt_work(struct work_struct *work) { @@ -111,77 +183,19 @@ virtio_transport_send_pkt_work(struct work_struct *work) vq = vsock->vqs[VSOCK_VQ_TX]; for (;;) { - int ret, in_sg = 0, out_sg = 0; - struct scatterlist **sgs; struct sk_buff *skb; - bool reply; + int ret; skb = virtio_vsock_skb_dequeue(&vsock->send_pkt_queue); if (!skb) break; - reply = virtio_vsock_skb_reply(skb); - sgs = vsock->out_sgs; - sg_init_one(sgs[out_sg], virtio_vsock_hdr(skb), - sizeof(*virtio_vsock_hdr(skb))); - out_sg++; - - if (!skb_is_nonlinear(skb)) { - if (skb->len > 0) { - sg_init_one(sgs[out_sg], skb->data, skb->len); - out_sg++; - } - } else { - struct skb_shared_info *si; - int i; + ret = virtio_transport_send_skb(skb, vq, vsock, &restart_rx); - /* If skb is nonlinear, then its buffer must contain - * only header and nothing more. Data is stored in - * the fragged part. - */ - WARN_ON_ONCE(skb_headroom(skb) != sizeof(*virtio_vsock_hdr(skb))); - - si = skb_shinfo(skb); - - for (i = 0; i < si->nr_frags; i++) { - skb_frag_t *skb_frag = &si->frags[i]; - void *va; - - /* We will use 'page_to_virt()' for the userspace page - * here, because virtio or dma-mapping layers will call - * 'virt_to_phys()' later to fill the buffer descriptor. - * We don't touch memory at "virtual" address of this page. - */ - va = page_to_virt(skb_frag_page(skb_frag)); - sg_init_one(sgs[out_sg], - va + skb_frag_off(skb_frag), - skb_frag_size(skb_frag)); - out_sg++; - } - } - - ret = virtqueue_add_sgs(vq, sgs, out_sg, in_sg, skb, GFP_KERNEL); - /* Usually this means that there is no more space available in - * the vq - */ if (ret < 0) { virtio_vsock_skb_queue_head(&vsock->send_pkt_queue, skb); break; } - - virtio_transport_deliver_tap_pkt(skb); - - if (reply) { - struct virtqueue *rx_vq = vsock->vqs[VSOCK_VQ_RX]; - int val; - - val = atomic_dec_return(&vsock->queued_replies); - - /* Do we now have resources to resume rx processing? */ - if (val + 1 == virtqueue_get_vring_size(rx_vq)) - restart_rx = true; - } - added = true; } -- 2.45.2 ^ permalink raw reply related [flat|nested] 8+ messages in thread
* Re: [PATCH net-next 1/2] vsock/virtio: refactor virtio_transport_send_pkt_work 2024-06-14 13:55 ` [PATCH net-next 1/2] vsock/virtio: refactor virtio_transport_send_pkt_work Luigi Leonardi @ 2024-06-14 14:37 ` Stefano Garzarella 0 siblings, 0 replies; 8+ messages in thread From: Stefano Garzarella @ 2024-06-14 14:37 UTC (permalink / raw) To: Luigi Leonardi Cc: edumazet, virtualization, netdev, kuba, kvm, stefanha, pabeni, davem, Marco Pinna On Fri, Jun 14, 2024 at 03:55:42PM GMT, Luigi Leonardi wrote: >From: Marco Pinna <marco.pinn95@gmail.com> > >This is a preliminary patch to introduce an optimization to >the enqueue system. > >All the code used to enqueue a packet into the virtqueue >is removed from virtio_transport_send_pkt_work() >and moved to the new virtio_transport_send_skb() function. > >Co-developed-by: Luigi Leonardi <luigi.leonardi@outlook.com> >Signed-off-by: Luigi Leonardi <luigi.leonardi@outlook.com> >Signed-off-by: Marco Pinna <marco.pinn95@gmail.com> >--- > net/vmw_vsock/virtio_transport.c | 134 +++++++++++++++++-------------- > 1 file changed, 74 insertions(+), 60 deletions(-) > >diff --git a/net/vmw_vsock/virtio_transport.c b/net/vmw_vsock/virtio_transport.c >index 43d405298857..c930235ecaec 100644 >--- a/net/vmw_vsock/virtio_transport.c >+++ b/net/vmw_vsock/virtio_transport.c >@@ -94,6 +94,78 @@ static u32 virtio_transport_get_local_cid(void) > return ret; > } > >+/* Caller need to hold vsock->tx_lock on vq */ >+static int virtio_transport_send_skb(struct sk_buff *skb, struct virtqueue *vq, >+ struct virtio_vsock *vsock, bool *restart_rx) >+{ >+ int ret, in_sg = 0, out_sg = 0; >+ struct scatterlist **sgs; >+ bool reply; >+ >+ reply = virtio_vsock_skb_reply(skb); >+ sgs = vsock->out_sgs; >+ sg_init_one(sgs[out_sg], virtio_vsock_hdr(skb), >+ sizeof(*virtio_vsock_hdr(skb))); >+ out_sg++; >+ >+ if (!skb_is_nonlinear(skb)) { >+ if (skb->len > 0) { >+ sg_init_one(sgs[out_sg], skb->data, skb->len); >+ out_sg++; >+ } >+ } else { >+ struct skb_shared_info *si; >+ int i; >+ >+ /* If skb is nonlinear, then its buffer must contain >+ * only header and nothing more. Data is stored in >+ * the fragged part. >+ */ >+ WARN_ON_ONCE(skb_headroom(skb) != sizeof(*virtio_vsock_hdr(skb))); >+ >+ si = skb_shinfo(skb); >+ >+ for (i = 0; i < si->nr_frags; i++) { >+ skb_frag_t *skb_frag = &si->frags[i]; >+ void *va; >+ >+ /* We will use 'page_to_virt()' for the userspace page >+ * here, because virtio or dma-mapping layers will call >+ * 'virt_to_phys()' later to fill the buffer descriptor. >+ * We don't touch memory at "virtual" address of >this page. >+ */ >+ va = page_to_virt(skb_frag_page(skb_frag)); >+ sg_init_one(sgs[out_sg], >+ va + skb_frag_off(skb_frag), >+ skb_frag_size(skb_frag)); >+ out_sg++; >+ } >+ } >+ >+ ret = virtqueue_add_sgs(vq, sgs, out_sg, in_sg, skb, GFP_KERNEL); >+ /* Usually this means that there is no more space available in >+ * the vq >+ */ >+ if (ret < 0) >+ goto out; We use the `out` label just here, so what about remove it since we just return ret? I mean: if (ret < 0) return ret; ... >+ >+ virtio_transport_deliver_tap_pkt(skb); >+ >+ if (reply) { >+ struct virtqueue *rx_vq = vsock->vqs[VSOCK_VQ_RX]; >+ int val; >+ >+ val = atomic_dec_return(&vsock->queued_replies); >+ >+ /* Do we now have resources to resume rx processing? */ >+ if (val + 1 == virtqueue_get_vring_size(rx_vq)) >+ *restart_rx = true; >+ } >+ return 0; } >+out: >+ return ret; >+} >+ > static void > virtio_transport_send_pkt_work(struct work_struct *work) > { >@@ -111,77 +183,19 @@ virtio_transport_send_pkt_work(struct work_struct *work) > vq = vsock->vqs[VSOCK_VQ_TX]; > > for (;;) { >- int ret, in_sg = 0, out_sg = 0; >- struct scatterlist **sgs; > struct sk_buff *skb; >- bool reply; >+ int ret; > > skb = virtio_vsock_skb_dequeue(&vsock->send_pkt_queue); > if (!skb) > break; > >- reply = virtio_vsock_skb_reply(skb); >- sgs = vsock->out_sgs; >- sg_init_one(sgs[out_sg], virtio_vsock_hdr(skb), >- sizeof(*virtio_vsock_hdr(skb))); >- out_sg++; >- >- if (!skb_is_nonlinear(skb)) { >- if (skb->len > 0) { >- sg_init_one(sgs[out_sg], skb->data, skb->len); >- out_sg++; >- } >- } else { >- struct skb_shared_info *si; >- int i; >+ ret = virtio_transport_send_skb(skb, vq, vsock, &restart_rx); > nit: I'd remove this new line here. >- /* If skb is nonlinear, then its buffer must contain >- * only header and nothing more. Data is stored in >- * the fragged part. >- */ >- WARN_ON_ONCE(skb_headroom(skb) != sizeof(*virtio_vsock_hdr(skb))); >- >- si = skb_shinfo(skb); >- >- for (i = 0; i < si->nr_frags; i++) { >- skb_frag_t *skb_frag = &si->frags[i]; >- void *va; >- >- /* We will use 'page_to_virt()' for the userspace page >- * here, because virtio or dma-mapping layers will call >- * 'virt_to_phys()' later to fill the buffer descriptor. >- * We don't touch memory at "virtual" address of this page. >- */ >- va = page_to_virt(skb_frag_page(skb_frag)); >- sg_init_one(sgs[out_sg], >- va + skb_frag_off(skb_frag), >- skb_frag_size(skb_frag)); >- out_sg++; >- } >- } >- >- ret = virtqueue_add_sgs(vq, sgs, out_sg, in_sg, skb, >GFP_KERNEL); >- /* Usually this means that there is no more space >available in >- * the vq >- */ > if (ret < 0) { > virtio_vsock_skb_queue_head(&vsock->send_pkt_queue, skb); > break; > } >- >- virtio_transport_deliver_tap_pkt(skb); >- >- if (reply) { >- struct virtqueue *rx_vq = vsock->vqs[VSOCK_VQ_RX]; >- int val; >- >- val = atomic_dec_return(&vsock->queued_replies); >- >- /* Do we now have resources to resume rx processing? */ >- if (val + 1 == virtqueue_get_vring_size(rx_vq)) >- restart_rx = true; >- } >- nit: maybe I'd move the empty line here. Our usual style is: ret = foo(); if (ret < 0) { //error handling } next_stuff... Thanks, Stefano > added = true; > } > >-- >2.45.2 > ^ permalink raw reply [flat|nested] 8+ messages in thread
* [PATCH net-next 2/2] vsock/virtio: avoid enqueue packets when work queue is empty [not found] <20240614135543.31515-1-luigi.leonardi@outlook.com> 2024-06-14 13:55 ` [PATCH net-next 1/2] vsock/virtio: refactor virtio_transport_send_pkt_work Luigi Leonardi @ 2024-06-14 13:55 ` Luigi Leonardi 2024-06-14 14:37 ` Stefano Garzarella 2024-06-17 13:24 ` Matias Ezequiel Vara Larsen 1 sibling, 2 replies; 8+ messages in thread From: Luigi Leonardi @ 2024-06-14 13:55 UTC (permalink / raw) To: sgarzare, edumazet, virtualization, netdev, kuba, kvm, stefanha, pabeni, davem Cc: Marco Pinna, Luigi Leonardi From: Marco Pinna <marco.pinn95@gmail.com> 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 Co-developed-by: Luigi Leonardi <luigi.leonardi@outlook.com> Signed-off-by: Luigi Leonardi <luigi.leonardi@outlook.com> Signed-off-by: Marco Pinna <marco.pinn95@gmail.com> --- 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; 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. + * Just put it on the ringbuff using virtio_transport_send_skb. + */ + + if (skb_queue_empty_lockless(&vsock->send_pkt_queue)) { + bool restart_rx = false; + struct virtqueue *vq; + + mutex_lock(&vsock->tx_lock); + + vq = vsock->vqs[VSOCK_VQ_TX]; + + ret = virtio_transport_send_skb(skb, vq, vsock, &restart_rx); + 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 ^ permalink raw reply related [flat|nested] 8+ messages in thread
* Re: [PATCH net-next 2/2] vsock/virtio: avoid enqueue packets when work queue is empty 2024-06-14 13:55 ` [PATCH net-next 2/2] vsock/virtio: avoid enqueue packets when work queue is empty Luigi Leonardi @ 2024-06-14 14:37 ` Stefano Garzarella 2024-06-17 13:24 ` Matias Ezequiel Vara Larsen 1 sibling, 0 replies; 8+ messages in thread From: Stefano Garzarella @ 2024-06-14 14:37 UTC (permalink / raw) To: Luigi Leonardi Cc: edumazet, virtualization, netdev, kuba, kvm, stefanha, pabeni, davem, Marco Pinna On Fri, Jun 14, 2024 at 03:55:43PM GMT, Luigi Leonardi wrote: >From: Marco Pinna <marco.pinn95@gmail.com> > >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? > >Co-developed-by: Luigi Leonardi <luigi.leonardi@outlook.com> >Signed-off-by: Luigi Leonardi <luigi.leonardi@outlook.com> >Signed-off-by: Marco Pinna <marco.pinn95@gmail.com> >--- > 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. > > 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 > ^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH net-next 2/2] vsock/virtio: avoid enqueue packets when work queue is empty 2024-06-14 13:55 ` [PATCH net-next 2/2] vsock/virtio: avoid enqueue packets when work queue is empty Luigi Leonardi 2024-06-14 14:37 ` Stefano Garzarella @ 2024-06-17 13:24 ` Matias Ezequiel Vara Larsen 2024-06-18 17:05 ` Luigi Leonardi 1 sibling, 1 reply; 8+ messages in thread From: Matias Ezequiel Vara Larsen @ 2024-06-17 13:24 UTC (permalink / raw) To: Luigi Leonardi Cc: sgarzare, edumazet, virtualization, netdev, kuba, kvm, stefanha, pabeni, davem, Marco Pinna 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 <marco.pinn95@gmail.com> > > 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 ^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH net-next 2/2] vsock/virtio: avoid enqueue packets when work queue is empty 2024-06-17 13:24 ` Matias Ezequiel Vara Larsen @ 2024-06-18 17:05 ` Luigi Leonardi 2024-06-21 8:58 ` Matias Ezequiel Vara Larsen 0 siblings, 1 reply; 8+ messages in thread From: Luigi Leonardi @ 2024-06-18 17:05 UTC (permalink / raw) To: mvaralar Cc: davem, edumazet, kuba, kvm, luigi.leonardi, marco.pinn95, netdev, pabeni, sgarzare, stefanha, virtualization 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. > 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! > 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? Luigi ^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH net-next 2/2] vsock/virtio: avoid enqueue packets when work queue is empty 2024-06-18 17:05 ` Luigi Leonardi @ 2024-06-21 8:58 ` Matias Ezequiel Vara Larsen 2024-06-21 9:47 ` Luigi Leonardi 0 siblings, 1 reply; 8+ messages in thread From: Matias Ezequiel Vara Larsen @ 2024-06-21 8:58 UTC (permalink / raw) To: Luigi Leonardi Cc: davem, edumazet, kuba, kvm, marco.pinn95, netdev, pabeni, sgarzare, stefanha, virtualization 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 ^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH net-next 2/2] vsock/virtio: avoid enqueue packets when work queue is empty 2024-06-21 8:58 ` Matias Ezequiel Vara Larsen @ 2024-06-21 9:47 ` Luigi Leonardi 0 siblings, 0 replies; 8+ messages in thread From: Luigi Leonardi @ 2024-06-21 9:47 UTC (permalink / raw) To: mvaralar Cc: davem, edumazet, kuba, kvm, luigi.leonardi, marco.pinn95, netdev, pabeni, sgarzare, stefanha, virtualization Hi Matias, > > > 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 ;) No worries :) > 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? Correct! Packets are enqueued in the worker-queue only if the virtqueue is full. Luigi ^ permalink raw reply [flat|nested] 8+ messages in thread
end of thread, other threads:[~2024-06-21 9:49 UTC | newest]
Thread overview: 8+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
[not found] <20240614135543.31515-1-luigi.leonardi@outlook.com>
2024-06-14 13:55 ` [PATCH net-next 1/2] vsock/virtio: refactor virtio_transport_send_pkt_work Luigi Leonardi
2024-06-14 14:37 ` Stefano Garzarella
2024-06-14 13:55 ` [PATCH net-next 2/2] vsock/virtio: avoid enqueue packets when work queue is empty Luigi Leonardi
2024-06-14 14:37 ` Stefano Garzarella
2024-06-17 13:24 ` Matias Ezequiel Vara Larsen
2024-06-18 17:05 ` Luigi Leonardi
2024-06-21 8:58 ` Matias Ezequiel Vara Larsen
2024-06-21 9:47 ` Luigi Leonardi
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for NNTP newsgroup(s).