| lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
|
Open Source and information security mailing list archives
| ||
|
Date: Tue, 2 Jul 2024 11:49:09 +0200
From: Stefano Garzarella <sgarzare@...hat.com>
To: luigi.leonardi@...look.com, Stefan Hajnoczi <stefanha@...hat.com>
Cc: "David S. Miller" <davem@...emloft.net>,
Eric Dumazet <edumazet@...gle.com>, Jakub Kicinski <kuba@...nel.org>,
Paolo Abeni <pabeni@...hat.com>, kvm@...r.kernel.org, virtualization@...ts.linux.dev,
netdev@...r.kernel.org, linux-kernel@...r.kernel.org,
Marco Pinna <marco.pinn95@...il.com>
Subject: Re: [PATCH PATCH net-next v2 1/2] vsock/virtio: refactor
virtio_transport_send_pkt_work
On Mon, Jul 01, 2024 at 04:28:02PM GMT, Luigi Leonardi via B4 Relay wrote:
>From: Marco Pinna <marco.pinn95@...il.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@...look.com>
>Signed-off-by: Luigi Leonardi <luigi.leonardi@...look.com>
>Signed-off-by: Marco Pinna <marco.pinn95@...il.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
>
>
Powered by blists - more mailing lists