| Message ID | 20190510125843.95587-3-sgarzare@redhat.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | vsock/virtio: optimizations to increase the throughput | expand |
From: Stefano Garzarella <sgarzare@redhat.com> Date: Fri, 10 May 2019 14:58:37 +0200 > @@ -827,12 +827,20 @@ static bool virtio_transport_close(struct vsock_sock *vsk) > > void virtio_transport_release(struct vsock_sock *vsk) > { > + struct virtio_vsock_sock *vvs = vsk->trans; > + struct virtio_vsock_buf *buf; > struct sock *sk = &vsk->sk; > bool remove_sock = true; > > lock_sock(sk); > if (sk->sk_type == SOCK_STREAM) > remove_sock = virtio_transport_close(vsk); > + while (!list_empty(&vvs->rx_queue)) { > + buf = list_first_entry(&vvs->rx_queue, > + struct virtio_vsock_buf, list); Please use list_for_each_entry_safe().
On Fri, May 10, 2019 at 03:20:08PM -0700, David Miller wrote: > From: Stefano Garzarella <sgarzare@redhat.com> > Date: Fri, 10 May 2019 14:58:37 +0200 > > > @@ -827,12 +827,20 @@ static bool virtio_transport_close(struct vsock_sock *vsk) > > > > void virtio_transport_release(struct vsock_sock *vsk) > > { > > + struct virtio_vsock_sock *vvs = vsk->trans; > > + struct virtio_vsock_buf *buf; > > struct sock *sk = &vsk->sk; > > bool remove_sock = true; > > > > lock_sock(sk); > > if (sk->sk_type == SOCK_STREAM) > > remove_sock = virtio_transport_close(vsk); > > + while (!list_empty(&vvs->rx_queue)) { > > + buf = list_first_entry(&vvs->rx_queue, > > + struct virtio_vsock_buf, list); > > Please use list_for_each_entry_safe(). Thanks for the review, I'll change it in the v3. Cheers, Stefano
On Fri, May 10, 2019 at 02:58:37PM +0200, Stefano Garzarella wrote: > When the socket is released, we should free all packets > queued in the per-socket list in order to avoid a memory > leak. > > Signed-off-by: Stefano Garzarella <sgarzare@redhat.com> > --- > net/vmw_vsock/virtio_transport_common.c | 8 ++++++++ > 1 file changed, 8 insertions(+) Ouch, this would be nice as a separate patch that can be merged right away (with s/virtio_vsock_buf/virtio_vsock_pkt/).
On Thu, May 16, 2019 at 04:32:18PM +0100, Stefan Hajnoczi wrote: > On Fri, May 10, 2019 at 02:58:37PM +0200, Stefano Garzarella wrote: > > When the socket is released, we should free all packets > > queued in the per-socket list in order to avoid a memory > > leak. > > > > Signed-off-by: Stefano Garzarella <sgarzare@redhat.com> > > --- > > net/vmw_vsock/virtio_transport_common.c | 8 ++++++++ > > 1 file changed, 8 insertions(+) > > Ouch, this would be nice as a separate patch that can be merged right > away (with s/virtio_vsock_buf/virtio_vsock_pkt/). Okay, I'll fix this patch following the David's comment and I'll send as a separate patch using the virtio_vsock_pkt. Thanks, Stefano
diff --git a/net/vmw_vsock/virtio_transport_common.c b/net/vmw_vsock/virtio_transport_common.c index 0248d6808755..65c8b4a23f2b 100644 --- a/net/vmw_vsock/virtio_transport_common.c +++ b/net/vmw_vsock/virtio_transport_common.c @@ -827,12 +827,20 @@ static bool virtio_transport_close(struct vsock_sock *vsk) void virtio_transport_release(struct vsock_sock *vsk) { + struct virtio_vsock_sock *vvs = vsk->trans; + struct virtio_vsock_buf *buf; struct sock *sk = &vsk->sk; bool remove_sock = true; lock_sock(sk); if (sk->sk_type == SOCK_STREAM) remove_sock = virtio_transport_close(vsk); + while (!list_empty(&vvs->rx_queue)) { + buf = list_first_entry(&vvs->rx_queue, + struct virtio_vsock_buf, list); + list_del(&buf->list); + virtio_transport_free_buf(buf); + } release_sock(sk); if (remove_sock)
When the socket is released, we should free all packets queued in the per-socket list in order to avoid a memory leak. Signed-off-by: Stefano Garzarella <sgarzare@redhat.com> --- net/vmw_vsock/virtio_transport_common.c | 8 ++++++++ 1 file changed, 8 insertions(+)