From: Paolo Abeni <pabeni@redhat.com> To: Bobby Eshleman <bobby.eshleman@bytedance.com> Cc: Bobby Eshleman <bobbyeshleman@gmail.com>, Cong Wang <cong.wang@bytedance.com>, Krasnov Arseniy <oxffffaa@gmail.com>, Jiang Wang <jiang.wang@bytedance.com>, "Michael S. Tsirkin" <mst@redhat.com>, netdev@vger.kernel.org, linux-kernel@vger.kernel.org, virtualization@lists.linux-foundation.org, Eric Dumazet <edumazet@google.com>, Stefan Hajnoczi <stefanha@redhat.com>, kvm@vger.kernel.org, Jakub Kicinski <kuba@kernel.org>, "David S. Miller" <davem@davemloft.net> Subject: Re: [PATCH net-next v7] virtio/vsock: replace virtio_vsock_pkt with sk_buff Date: Wed, 14 Dec 2022 11:58:47 +0100 [thread overview] Message-ID: <4b66f91f23a3eb91c3232bc68f45bdb799217c40.camel@redhat.com> (raw) In-Reply-To: <20221213192843.421032-1-bobby.eshleman@bytedance.com> On Tue, 2022-12-13 at 19:28 +0000, Bobby Eshleman wrote: > diff --git a/drivers/vhost/vsock.c b/drivers/vhost/vsock.c > index 5703775af129..2a5994b029b2 100644 > --- a/drivers/vhost/vsock.c > +++ b/drivers/vhost/vsock.c > @@ -51,8 +51,7 @@ struct vhost_vsock { > struct hlist_node hash; > > struct vhost_work send_pkt_work; > - spinlock_t send_pkt_list_lock; > - struct list_head send_pkt_list; /* host->guest pending packets */ > + struct sk_buff_head send_pkt_queue; /* host->guest pending packets */ > > atomic_t queued_replies; > > @@ -108,40 +107,33 @@ vhost_transport_do_send_pkt(struct vhost_vsock *vsock, > vhost_disable_notify(&vsock->dev, vq); > > do { > - struct virtio_vsock_pkt *pkt; > + struct virtio_vsock_hdr *hdr; > + size_t iov_len, payload_len; > struct iov_iter iov_iter; > + u32 flags_to_restore = 0; > + struct sk_buff *skb; > unsigned out, in; > size_t nbytes; > - size_t iov_len, payload_len; > int head; > - u32 flags_to_restore = 0; > > - spin_lock_bh(&vsock->send_pkt_list_lock); > - if (list_empty(&vsock->send_pkt_list)) { > - spin_unlock_bh(&vsock->send_pkt_list_lock); > + spin_lock(&vsock->send_pkt_queue.lock); > + skb = __skb_dequeue(&vsock->send_pkt_queue); > + spin_unlock(&vsock->send_pkt_queue.lock); Here you use a plain spin_lock(), but every other lock has the _bh() variant. A few lines above this functions acquires a mutex, so this is process context (and not BH context): I guess you should use _bh() here, too. [...] > diff --git a/include/linux/virtio_vsock.h b/include/linux/virtio_vsock.h > index 35d7eedb5e8e..0385df976d41 100644 > --- a/include/linux/virtio_vsock.h > +++ b/include/linux/virtio_vsock.h > @@ -3,10 +3,116 @@ > #define _LINUX_VIRTIO_VSOCK_H > > #include <uapi/linux/virtio_vsock.h> > +#include <linux/bits.h> > #include <linux/socket.h> > #include <net/sock.h> > #include <net/af_vsock.h> > > +#define VIRTIO_VSOCK_SKB_HEADROOM (sizeof(struct virtio_vsock_hdr)) > + > +enum virtio_vsock_skb_flags { > + VIRTIO_VSOCK_SKB_FLAGS_REPLY = BIT(0), > + VIRTIO_VSOCK_SKB_FLAGS_TAP_DELIVERED = BIT(1), > +}; It looks like the above enum is not used anymore, you can drop it. [...] > @@ -121,20 +108,18 @@ static void vsock_loopback_work(struct work_struct *work) > { > struct vsock_loopback *vsock = > container_of(work, struct vsock_loopback, pkt_work); > - LIST_HEAD(pkts); > + struct sk_buff_head pkts; > + struct sk_buff *skb; > + > + skb_queue_head_init(&pkts); > > spin_lock_bh(&vsock->pkt_list_lock); > - list_splice_init(&vsock->pkt_list, &pkts); > + skb_queue_splice_init(&vsock->pkt_queue, &pkts); > spin_unlock_bh(&vsock->pkt_list_lock); > > - while (!list_empty(&pkts)) { > - struct virtio_vsock_pkt *pkt; > - > - pkt = list_first_entry(&pkts, struct virtio_vsock_pkt, list); > - list_del_init(&pkt->list); > - > - virtio_transport_deliver_tap_pkt(pkt); > - virtio_transport_recv_pkt(&loopback_transport, pkt); > + while ((skb = skb_dequeue(&pkts))) { Minor nit: since this code has complete ownership of the pkts queue, you can use the lockless dequeue variant here: while ((skb = __skb_dequeue(&pkts))) { > + virtio_transport_deliver_tap_pkt(skb); > + virtio_transport_recv_pkt(&loopback_transport, skb); > } > } > Other then that LGTM. @Michael: feel free to take this via your tree, once that the above feedback has been addressed, thanks! Paolo _______________________________________________ Virtualization mailing list Virtualization@lists.linux-foundation.org https://lists.linuxfoundation.org/mailman/listinfo/virtualization
WARNING: multiple messages have this Message-ID (diff)
From: Paolo Abeni <pabeni@redhat.com> To: Bobby Eshleman <bobby.eshleman@bytedance.com> Cc: Bobby Eshleman <bobbyeshleman@gmail.com>, Cong Wang <cong.wang@bytedance.com>, Jiang Wang <jiang.wang@bytedance.com>, Krasnov Arseniy <oxffffaa@gmail.com>, Stefan Hajnoczi <stefanha@redhat.com>, Stefano Garzarella <sgarzare@redhat.com>, "Michael S. Tsirkin" <mst@redhat.com>, Jason Wang <jasowang@redhat.com>, "David S. Miller" <davem@davemloft.net>, Eric Dumazet <edumazet@google.com>, Jakub Kicinski <kuba@kernel.org>, kvm@vger.kernel.org, virtualization@lists.linux-foundation.org, netdev@vger.kernel.org, linux-kernel@vger.kernel.org Subject: Re: [PATCH net-next v7] virtio/vsock: replace virtio_vsock_pkt with sk_buff Date: Wed, 14 Dec 2022 11:58:47 +0100 [thread overview] Message-ID: <4b66f91f23a3eb91c3232bc68f45bdb799217c40.camel@redhat.com> (raw) In-Reply-To: <20221213192843.421032-1-bobby.eshleman@bytedance.com> On Tue, 2022-12-13 at 19:28 +0000, Bobby Eshleman wrote: > diff --git a/drivers/vhost/vsock.c b/drivers/vhost/vsock.c > index 5703775af129..2a5994b029b2 100644 > --- a/drivers/vhost/vsock.c > +++ b/drivers/vhost/vsock.c > @@ -51,8 +51,7 @@ struct vhost_vsock { > struct hlist_node hash; > > struct vhost_work send_pkt_work; > - spinlock_t send_pkt_list_lock; > - struct list_head send_pkt_list; /* host->guest pending packets */ > + struct sk_buff_head send_pkt_queue; /* host->guest pending packets */ > > atomic_t queued_replies; > > @@ -108,40 +107,33 @@ vhost_transport_do_send_pkt(struct vhost_vsock *vsock, > vhost_disable_notify(&vsock->dev, vq); > > do { > - struct virtio_vsock_pkt *pkt; > + struct virtio_vsock_hdr *hdr; > + size_t iov_len, payload_len; > struct iov_iter iov_iter; > + u32 flags_to_restore = 0; > + struct sk_buff *skb; > unsigned out, in; > size_t nbytes; > - size_t iov_len, payload_len; > int head; > - u32 flags_to_restore = 0; > > - spin_lock_bh(&vsock->send_pkt_list_lock); > - if (list_empty(&vsock->send_pkt_list)) { > - spin_unlock_bh(&vsock->send_pkt_list_lock); > + spin_lock(&vsock->send_pkt_queue.lock); > + skb = __skb_dequeue(&vsock->send_pkt_queue); > + spin_unlock(&vsock->send_pkt_queue.lock); Here you use a plain spin_lock(), but every other lock has the _bh() variant. A few lines above this functions acquires a mutex, so this is process context (and not BH context): I guess you should use _bh() here, too. [...] > diff --git a/include/linux/virtio_vsock.h b/include/linux/virtio_vsock.h > index 35d7eedb5e8e..0385df976d41 100644 > --- a/include/linux/virtio_vsock.h > +++ b/include/linux/virtio_vsock.h > @@ -3,10 +3,116 @@ > #define _LINUX_VIRTIO_VSOCK_H > > #include <uapi/linux/virtio_vsock.h> > +#include <linux/bits.h> > #include <linux/socket.h> > #include <net/sock.h> > #include <net/af_vsock.h> > > +#define VIRTIO_VSOCK_SKB_HEADROOM (sizeof(struct virtio_vsock_hdr)) > + > +enum virtio_vsock_skb_flags { > + VIRTIO_VSOCK_SKB_FLAGS_REPLY = BIT(0), > + VIRTIO_VSOCK_SKB_FLAGS_TAP_DELIVERED = BIT(1), > +}; It looks like the above enum is not used anymore, you can drop it. [...] > @@ -121,20 +108,18 @@ static void vsock_loopback_work(struct work_struct *work) > { > struct vsock_loopback *vsock = > container_of(work, struct vsock_loopback, pkt_work); > - LIST_HEAD(pkts); > + struct sk_buff_head pkts; > + struct sk_buff *skb; > + > + skb_queue_head_init(&pkts); > > spin_lock_bh(&vsock->pkt_list_lock); > - list_splice_init(&vsock->pkt_list, &pkts); > + skb_queue_splice_init(&vsock->pkt_queue, &pkts); > spin_unlock_bh(&vsock->pkt_list_lock); > > - while (!list_empty(&pkts)) { > - struct virtio_vsock_pkt *pkt; > - > - pkt = list_first_entry(&pkts, struct virtio_vsock_pkt, list); > - list_del_init(&pkt->list); > - > - virtio_transport_deliver_tap_pkt(pkt); > - virtio_transport_recv_pkt(&loopback_transport, pkt); > + while ((skb = skb_dequeue(&pkts))) { Minor nit: since this code has complete ownership of the pkts queue, you can use the lockless dequeue variant here: while ((skb = __skb_dequeue(&pkts))) { > + virtio_transport_deliver_tap_pkt(skb); > + virtio_transport_recv_pkt(&loopback_transport, skb); > } > } > Other then that LGTM. @Michael: feel free to take this via your tree, once that the above feedback has been addressed, thanks! Paolo
next prev parent reply other threads:[~2022-12-14 10:58 UTC|newest] Thread overview: 7+ messages / expand[flat|nested] mbox.gz Atom feed top 2022-12-13 19:28 [PATCH net-next v7] virtio/vsock: replace virtio_vsock_pkt with sk_buff Bobby Eshleman 2022-12-14 8:57 ` Stefano Garzarella 2022-12-14 8:57 ` Stefano Garzarella 2022-12-14 10:58 ` Paolo Abeni [this message] 2022-12-14 10:58 ` Paolo Abeni 2022-12-15 8:30 ` Stefano Garzarella 2022-12-15 8:30 ` Stefano Garzarella
Reply instructions: You may reply publicly to this message via plain-text email using any one of the following methods: * Save the following mbox file, import it into your mail client, and reply-to-all from there: mbox Avoid top-posting and favor interleaved quoting: https://en.wikipedia.org/wiki/Posting_style#Interleaved_style * Reply using the --to, --cc, and --in-reply-to switches of git-send-email(1): git send-email \ --in-reply-to=4b66f91f23a3eb91c3232bc68f45bdb799217c40.camel@redhat.com \ --to=pabeni@redhat.com \ --cc=bobby.eshleman@bytedance.com \ --cc=bobbyeshleman@gmail.com \ --cc=cong.wang@bytedance.com \ --cc=davem@davemloft.net \ --cc=edumazet@google.com \ --cc=jiang.wang@bytedance.com \ --cc=kuba@kernel.org \ --cc=kvm@vger.kernel.org \ --cc=linux-kernel@vger.kernel.org \ --cc=mst@redhat.com \ --cc=netdev@vger.kernel.org \ --cc=oxffffaa@gmail.com \ --cc=stefanha@redhat.com \ --cc=virtualization@lists.linux-foundation.org \ /path/to/YOUR_REPLY https://kernel.org/pub/software/scm/git/docs/git-send-email.html * If your mail client supports setting the In-Reply-To header via mailto: links, try the mailto: linkBe sure your reply has a Subject: header at the top and a blank line before the message body.
This is an external index of several public inboxes, see mirroring instructions on how to clone and mirror all data and code used by this external index.