| Message ID | 20240701-pinna-v2-1-ac396d181f59@outlook.com (mailing list archive) |
|---|---|
| State | Changes Requested |
| Delegated to: | Netdev Maintainers |
| Headers | show |
| Series | vsock: avoid queuing on workqueue if possible | expand |
On Mon, Jul 01, 2024 at 04:28:02PM GMT, Luigi Leonardi via B4 Relay wrote: >From: Marco Pinna <marco.pinn95@gmail.com> > >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 | 133 +++++++++++++++++++++------------------ > 1 file changed, 73 insertions(+), 60 deletions(-) > >diff --git a/net/vmw_vsock/virtio_transport.c b/net/vmw_vsock/virtio_transport.c >index 43d405298857..a74083d28120 100644 >--- a/net/vmw_vsock/virtio_transport.c >+++ b/net/vmw_vsock/virtio_transport.c >@@ -94,6 +94,77 @@ 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) >+ 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; >+ } Looking more closely at this patch, perhaps we can leave reply handling out of this refactoring, as it is only needed in the worker. IIUC, this is to prevent the RX worker from leaving room for the TX worker by handling too many replies. So when we have a large enough number of replies (equal to the size of the RX queue) in the queue of the TX worker ready to be queued in the virtqueue, we stop the RX worker and restart it only when the TX worker has had a chance to send replies. @Stefan can you confirm this since you were involved in the original implementation? If we skip the worker, we don't need this. Moreover, we know well that the worker has no queued elements, so we will only go to increment `queued_replies` and then decrement it immediately afterwards. Thanks, Stefano >+ >+ return 0; >+} >+ > static void > virtio_transport_send_pkt_work(struct work_struct *work) > { >@@ -111,77 +182,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; >- >- /* 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 >- */ >+ ret = virtio_transport_send_skb(skb, vq, vsock, &restart_rx); > 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 > >
diff --git a/net/vmw_vsock/virtio_transport.c b/net/vmw_vsock/virtio_transport.c index 43d405298857..a74083d28120 100644 --- a/net/vmw_vsock/virtio_transport.c +++ b/net/vmw_vsock/virtio_transport.c @@ -94,6 +94,77 @@ 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) + 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; +} + static void virtio_transport_send_pkt_work(struct work_struct *work) { @@ -111,77 +182,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; - - /* 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 - */ + ret = virtio_transport_send_skb(skb, vq, vsock, &restart_rx); 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; }