From: Jorgen Hansen <jhansen@vmware.com> To: Arseny Krasnov <arseny.krasnov@kaspersky.com> Cc: 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>, Jakub Kicinski <kuba@kernel.org>, Colin Ian King <colin.king@canonical.com>, Andra Paraschiv <andraprs@amazon.com>, Alexander Popov <alex.popov@linux.com>, "kvm@vger.kernel.org" <kvm@vger.kernel.org>, "virtualization@lists.linux-foundation.org" <virtualization@lists.linux-foundation.org>, "netdev@vger.kernel.org" <netdev@vger.kernel.org>, "linux-kernel@vger.kernel.org" <linux-kernel@vger.kernel.org>, "stsp2@yandex.ru" <stsp2@yandex.ru>, "oxffffaa@gmail.com" <oxffffaa@gmail.com> Subject: Re: [RFC PATCH v4 02/17] af_vsock: separate wait data loop Date: Thu, 11 Feb 2021 15:11:07 +0000 [thread overview] Message-ID: <4094442C-C2A1-49D6-9901-7F3C73FABFC9@vmware.com> (raw) In-Reply-To: <20210207151451.804498-1-arseny.krasnov@kaspersky.com> > On 7 Feb 2021, at 16:14, Arseny Krasnov <arseny.krasnov@kaspersky.com> wrote: > > This moves wait loop for data to dedicated function, because later > it will be used by SEQPACKET data receive loop. > > Signed-off-by: Arseny Krasnov <arseny.krasnov@kaspersky.com> > --- > net/vmw_vsock/af_vsock.c | 158 +++++++++++++++++++++------------------ > 1 file changed, 86 insertions(+), 72 deletions(-) > > diff --git a/net/vmw_vsock/af_vsock.c b/net/vmw_vsock/af_vsock.c > index f4fabec50650..38927695786f 100644 > --- a/net/vmw_vsock/af_vsock.c > +++ b/net/vmw_vsock/af_vsock.c > @@ -1833,6 +1833,71 @@ static int vsock_connectible_sendmsg(struct socket *sock, struct msghdr *msg, > return err; > } > > +static int vsock_wait_data(struct sock *sk, struct wait_queue_entry *wait, > + long timeout, > + struct vsock_transport_recv_notify_data *recv_data, > + size_t target) > +{ > + const struct vsock_transport *transport; > + struct vsock_sock *vsk; > + s64 data; > + int err; > + > + vsk = vsock_sk(sk); > + err = 0; > + transport = vsk->transport; > + prepare_to_wait(sk_sleep(sk), wait, TASK_INTERRUPTIBLE); > + > + while ((data = vsock_stream_has_data(vsk)) == 0) { > + if (sk->sk_err != 0 || > + (sk->sk_shutdown & RCV_SHUTDOWN) || > + (vsk->peer_shutdown & SEND_SHUTDOWN)) { > + goto out; > + } > + > + /* Don't wait for non-blocking sockets. */ > + if (timeout == 0) { > + err = -EAGAIN; > + goto out; > + } > + > + if (recv_data) { > + err = transport->notify_recv_pre_block(vsk, target, recv_data); > + if (err < 0) > + goto out; > + } > + > + release_sock(sk); > + timeout = schedule_timeout(timeout); > + lock_sock(sk); > + > + if (signal_pending(current)) { > + err = sock_intr_errno(timeout); > + goto out; > + } else if (timeout == 0) { > + err = -EAGAIN; > + goto out; > + } > + } > + > + finish_wait(sk_sleep(sk), wait); > + > + /* Invalid queue pair content. XXX This should > + * be changed to a connection reset in a later > + * change. > + */ Since you are here, could you update this comment to something like: /* Internal transport error when checking for available * data. XXX This should be changed to a connection * reset in a later change. */ > + if (data < 0) > + return -ENOMEM; > + > + /* Have some data, return. */ > + if (data) > + return data; > + > +out: > + finish_wait(sk_sleep(sk), wait); > + return err; > +} I agree with Stefanos suggestion to get rid of the out: part and just have the single finish_wait(). > + > static int > vsock_connectible_recvmsg(struct socket *sock, struct msghdr *msg, size_t len, > int flags) > @@ -1912,85 +1977,34 @@ vsock_connectible_recvmsg(struct socket *sock, struct msghdr *msg, size_t len, > > > while (1) { > - s64 ready; > + ssize_t read; > > - prepare_to_wait(sk_sleep(sk), &wait, TASK_INTERRUPTIBLE); > - ready = vsock_stream_has_data(vsk); > - > - if (ready == 0) { > - if (sk->sk_err != 0 || > - (sk->sk_shutdown & RCV_SHUTDOWN) || > - (vsk->peer_shutdown & SEND_SHUTDOWN)) { > - finish_wait(sk_sleep(sk), &wait); > - break; > - } > - /* Don't wait for non-blocking sockets. */ > - if (timeout == 0) { > - err = -EAGAIN; > - finish_wait(sk_sleep(sk), &wait); > - break; > - } > - > - err = transport->notify_recv_pre_block( > - vsk, target, &recv_data); > - if (err < 0) { > - finish_wait(sk_sleep(sk), &wait); > - break; > - } > - release_sock(sk); > - timeout = schedule_timeout(timeout); > - lock_sock(sk); > - > - if (signal_pending(current)) { > - err = sock_intr_errno(timeout); > - finish_wait(sk_sleep(sk), &wait); > - break; > - } else if (timeout == 0) { > - err = -EAGAIN; > - finish_wait(sk_sleep(sk), &wait); > - break; > - } > - } else { > - ssize_t read; > + err = vsock_wait_data(sk, &wait, timeout, &recv_data, target); > + if (err <= 0) > + break; There is a small change in the behaviour here if vsock_stream_has_data(vsk) returned something < 0. Since you just do a break, the err value can be updated if there is an sk->sk_err, a receive shutdown has been performed or data has already been copied. That should be ok, though. > - finish_wait(sk_sleep(sk), &wait); > - > - if (ready < 0) { > - /* Invalid queue pair content. XXX This should > - * be changed to a connection reset in a later > - * change. > - */ > - > - err = -ENOMEM; > - goto out; > - } > - > - err = transport->notify_recv_pre_dequeue( > - vsk, target, &recv_data); > - if (err < 0) > - break; > + err = transport->notify_recv_pre_dequeue(vsk, target, > + &recv_data); > + if (err < 0) > + break; > > - read = transport->stream_dequeue( > - vsk, msg, > - len - copied, flags); > - if (read < 0) { > - err = -ENOMEM; > - break; > - } > + read = transport->stream_dequeue(vsk, msg, len - copied, flags); > + if (read < 0) { > + err = -ENOMEM; > + break; > + } > > - copied += read; > + copied += read; > > - err = transport->notify_recv_post_dequeue( > - vsk, target, read, > - !(flags & MSG_PEEK), &recv_data); > - if (err < 0) > - goto out; > + err = transport->notify_recv_post_dequeue(vsk, target, read, > + !(flags & MSG_PEEK), &recv_data); > + if (err < 0) > + goto out; > > - if (read >= target || flags & MSG_PEEK) > - break; > + if (read >= target || flags & MSG_PEEK) > + break; > > - target -= read; > - } > + target -= read; > } > > if (sk->sk_err) > -- > 2.25.1 >
WARNING: multiple messages have this Message-ID (diff)
From: Jorgen Hansen <jhansen@vmware.com> To: Arseny Krasnov <arseny.krasnov@kaspersky.com> Cc: Andra Paraschiv <andraprs@amazon.com>, "kvm@vger.kernel.org" <kvm@vger.kernel.org>, "Michael S. Tsirkin" <mst@redhat.com>, "netdev@vger.kernel.org" <netdev@vger.kernel.org>, "stsp2@yandex.ru" <stsp2@yandex.ru>, "linux-kernel@vger.kernel.org" <linux-kernel@vger.kernel.org>, "virtualization@lists.linux-foundation.org" <virtualization@lists.linux-foundation.org>, "oxffffaa@gmail.com" <oxffffaa@gmail.com>, Stefan Hajnoczi <stefanha@redhat.com>, Colin Ian King <colin.king@canonical.com>, Jakub Kicinski <kuba@kernel.org>, Alexander Popov <alex.popov@linux.com>, "David S. Miller" <davem@davemloft.net> Subject: Re: [RFC PATCH v4 02/17] af_vsock: separate wait data loop Date: Thu, 11 Feb 2021 15:11:07 +0000 [thread overview] Message-ID: <4094442C-C2A1-49D6-9901-7F3C73FABFC9@vmware.com> (raw) In-Reply-To: <20210207151451.804498-1-arseny.krasnov@kaspersky.com> > On 7 Feb 2021, at 16:14, Arseny Krasnov <arseny.krasnov@kaspersky.com> wrote: > > This moves wait loop for data to dedicated function, because later > it will be used by SEQPACKET data receive loop. > > Signed-off-by: Arseny Krasnov <arseny.krasnov@kaspersky.com> > --- > net/vmw_vsock/af_vsock.c | 158 +++++++++++++++++++++------------------ > 1 file changed, 86 insertions(+), 72 deletions(-) > > diff --git a/net/vmw_vsock/af_vsock.c b/net/vmw_vsock/af_vsock.c > index f4fabec50650..38927695786f 100644 > --- a/net/vmw_vsock/af_vsock.c > +++ b/net/vmw_vsock/af_vsock.c > @@ -1833,6 +1833,71 @@ static int vsock_connectible_sendmsg(struct socket *sock, struct msghdr *msg, > return err; > } > > +static int vsock_wait_data(struct sock *sk, struct wait_queue_entry *wait, > + long timeout, > + struct vsock_transport_recv_notify_data *recv_data, > + size_t target) > +{ > + const struct vsock_transport *transport; > + struct vsock_sock *vsk; > + s64 data; > + int err; > + > + vsk = vsock_sk(sk); > + err = 0; > + transport = vsk->transport; > + prepare_to_wait(sk_sleep(sk), wait, TASK_INTERRUPTIBLE); > + > + while ((data = vsock_stream_has_data(vsk)) == 0) { > + if (sk->sk_err != 0 || > + (sk->sk_shutdown & RCV_SHUTDOWN) || > + (vsk->peer_shutdown & SEND_SHUTDOWN)) { > + goto out; > + } > + > + /* Don't wait for non-blocking sockets. */ > + if (timeout == 0) { > + err = -EAGAIN; > + goto out; > + } > + > + if (recv_data) { > + err = transport->notify_recv_pre_block(vsk, target, recv_data); > + if (err < 0) > + goto out; > + } > + > + release_sock(sk); > + timeout = schedule_timeout(timeout); > + lock_sock(sk); > + > + if (signal_pending(current)) { > + err = sock_intr_errno(timeout); > + goto out; > + } else if (timeout == 0) { > + err = -EAGAIN; > + goto out; > + } > + } > + > + finish_wait(sk_sleep(sk), wait); > + > + /* Invalid queue pair content. XXX This should > + * be changed to a connection reset in a later > + * change. > + */ Since you are here, could you update this comment to something like: /* Internal transport error when checking for available * data. XXX This should be changed to a connection * reset in a later change. */ > + if (data < 0) > + return -ENOMEM; > + > + /* Have some data, return. */ > + if (data) > + return data; > + > +out: > + finish_wait(sk_sleep(sk), wait); > + return err; > +} I agree with Stefanos suggestion to get rid of the out: part and just have the single finish_wait(). > + > static int > vsock_connectible_recvmsg(struct socket *sock, struct msghdr *msg, size_t len, > int flags) > @@ -1912,85 +1977,34 @@ vsock_connectible_recvmsg(struct socket *sock, struct msghdr *msg, size_t len, > > > while (1) { > - s64 ready; > + ssize_t read; > > - prepare_to_wait(sk_sleep(sk), &wait, TASK_INTERRUPTIBLE); > - ready = vsock_stream_has_data(vsk); > - > - if (ready == 0) { > - if (sk->sk_err != 0 || > - (sk->sk_shutdown & RCV_SHUTDOWN) || > - (vsk->peer_shutdown & SEND_SHUTDOWN)) { > - finish_wait(sk_sleep(sk), &wait); > - break; > - } > - /* Don't wait for non-blocking sockets. */ > - if (timeout == 0) { > - err = -EAGAIN; > - finish_wait(sk_sleep(sk), &wait); > - break; > - } > - > - err = transport->notify_recv_pre_block( > - vsk, target, &recv_data); > - if (err < 0) { > - finish_wait(sk_sleep(sk), &wait); > - break; > - } > - release_sock(sk); > - timeout = schedule_timeout(timeout); > - lock_sock(sk); > - > - if (signal_pending(current)) { > - err = sock_intr_errno(timeout); > - finish_wait(sk_sleep(sk), &wait); > - break; > - } else if (timeout == 0) { > - err = -EAGAIN; > - finish_wait(sk_sleep(sk), &wait); > - break; > - } > - } else { > - ssize_t read; > + err = vsock_wait_data(sk, &wait, timeout, &recv_data, target); > + if (err <= 0) > + break; There is a small change in the behaviour here if vsock_stream_has_data(vsk) returned something < 0. Since you just do a break, the err value can be updated if there is an sk->sk_err, a receive shutdown has been performed or data has already been copied. That should be ok, though. > - finish_wait(sk_sleep(sk), &wait); > - > - if (ready < 0) { > - /* Invalid queue pair content. XXX This should > - * be changed to a connection reset in a later > - * change. > - */ > - > - err = -ENOMEM; > - goto out; > - } > - > - err = transport->notify_recv_pre_dequeue( > - vsk, target, &recv_data); > - if (err < 0) > - break; > + err = transport->notify_recv_pre_dequeue(vsk, target, > + &recv_data); > + if (err < 0) > + break; > > - read = transport->stream_dequeue( > - vsk, msg, > - len - copied, flags); > - if (read < 0) { > - err = -ENOMEM; > - break; > - } > + read = transport->stream_dequeue(vsk, msg, len - copied, flags); > + if (read < 0) { > + err = -ENOMEM; > + break; > + } > > - copied += read; > + copied += read; > > - err = transport->notify_recv_post_dequeue( > - vsk, target, read, > - !(flags & MSG_PEEK), &recv_data); > - if (err < 0) > - goto out; > + err = transport->notify_recv_post_dequeue(vsk, target, read, > + !(flags & MSG_PEEK), &recv_data); > + if (err < 0) > + goto out; > > - if (read >= target || flags & MSG_PEEK) > - break; > + if (read >= target || flags & MSG_PEEK) > + break; > > - target -= read; > - } > + target -= read; > } > > if (sk->sk_err) > -- > 2.25.1 > _______________________________________________ Virtualization mailing list Virtualization@lists.linux-foundation.org https://lists.linuxfoundation.org/mailman/listinfo/virtualization
next prev parent reply other threads:[~2021-02-11 15:35 UTC|newest] Thread overview: 61+ messages / expand[flat|nested] mbox.gz Atom feed top 2021-02-07 15:12 [RFC PATCH v4 00/17] virtio/vsock: introduce SOCK_SEQPACKET support Arseny Krasnov 2021-02-07 15:14 ` [RFC PATCH v4 01/17] af_vsock: update functions for connectible socket Arseny Krasnov 2021-02-11 10:52 ` Stefano Garzarella 2021-02-11 10:52 ` Stefano Garzarella 2021-02-07 15:14 ` [RFC PATCH v4 02/17] af_vsock: separate wait data loop Arseny Krasnov 2021-02-11 11:24 ` Stefano Garzarella 2021-02-11 11:24 ` Stefano Garzarella 2021-02-11 15:11 ` Jorgen Hansen [this message] 2021-02-11 15:11 ` Jorgen Hansen 2021-02-16 6:58 ` Arseny Krasnov 2021-02-07 15:15 ` [RFC PATCH v4 03/17] af_vsock: separate receive " Arseny Krasnov 2021-02-11 11:37 ` Stefano Garzarella 2021-02-11 11:37 ` Stefano Garzarella 2021-02-07 15:15 ` [RFC PATCH v4 04/17] af_vsock: implement SEQPACKET receive loop Arseny Krasnov 2021-02-11 11:47 ` Stefano Garzarella 2021-02-11 11:47 ` Stefano Garzarella 2021-02-07 15:15 ` [RFC PATCH v4 05/17] af_vsock: separate wait space loop Arseny Krasnov 2021-02-07 16:58 ` kernel test robot 2021-02-11 12:14 ` Stefano Garzarella 2021-02-11 12:14 ` Stefano Garzarella 2021-02-07 15:15 ` [RFC PATCH v4 06/17] af_vsock: implement send logic for SEQPACKET Arseny Krasnov 2021-02-11 12:17 ` Stefano Garzarella 2021-02-11 12:17 ` Stefano Garzarella 2021-02-07 15:16 ` [RFC PATCH v4 07/17] af_vsock: rest of SEQPACKET support Arseny Krasnov 2021-02-11 12:27 ` Stefano Garzarella 2021-02-11 12:27 ` Stefano Garzarella 2021-02-15 9:11 ` Arseny Krasnov 2021-02-07 15:16 ` [RFC PATCH v4 08/17] af_vsock: update comments for stream sockets Arseny Krasnov 2021-02-11 13:19 ` Stefano Garzarella 2021-02-11 13:19 ` Stefano Garzarella 2021-02-07 15:16 ` [RFC PATCH v4 09/17] virtio/vsock: dequeue callback for SOCK_SEQPACKET Arseny Krasnov 2021-02-11 13:54 ` Stefano Garzarella 2021-02-11 13:54 ` Stefano Garzarella 2021-02-11 14:03 ` Stefano Garzarella 2021-02-11 14:03 ` Stefano Garzarella 2021-02-07 15:17 ` [RFC PATCH v4 10/17] virtio/vsock: fetch length for SEQPACKET record Arseny Krasnov 2021-02-11 13:58 ` Stefano Garzarella 2021-02-11 13:58 ` Stefano Garzarella 2021-02-07 15:17 ` [RFC PATCH v4 11/17] virtio/vsock: add SEQPACKET receive logic Arseny Krasnov 2021-02-07 15:17 ` [RFC PATCH v4 12/17] virtio/vsock: rest of SOCK_SEQPACKET support Arseny Krasnov 2021-02-09 4:34 ` kernel test robot 2021-02-11 11:00 ` Arseny Krasnov 2021-02-11 14:29 ` Stefano Garzarella 2021-02-11 14:29 ` Stefano Garzarella 2021-02-07 15:18 ` [RFC PATCH v4 13/17] virtio/vsock: setup SEQPACKET ops for transport Arseny Krasnov 2021-02-07 15:18 ` [RFC PATCH v4 14/17] vhost/vsock: " Arseny Krasnov 2021-02-07 15:18 ` [RFC PATCH v4 15/17] vsock_test: add SOCK_SEQPACKET tests Arseny Krasnov 2021-02-07 15:18 ` [RFC PATCH v4 16/17] loopback/vsock: setup SEQPACKET ops for transport Arseny Krasnov 2021-02-11 14:31 ` Stefano Garzarella 2021-02-11 14:31 ` Stefano Garzarella 2021-02-07 15:19 ` [RFC PATCH v4 17/17] virtio/vsock: simplify credit update function API Arseny Krasnov 2021-02-11 14:39 ` Stefano Garzarella 2021-02-11 14:39 ` Stefano Garzarella 2021-02-07 16:20 ` [RFC PATCH v4 00/17] virtio/vsock: introduce SOCK_SEQPACKET support Michael S. Tsirkin 2021-02-07 16:20 ` Michael S. Tsirkin 2021-02-08 6:32 ` Arseny Krasnov 2021-02-11 14:57 ` Stefano Garzarella 2021-02-11 14:57 ` Stefano Garzarella 2021-02-12 6:11 ` Arseny Krasnov 2021-02-12 8:07 ` Stefano Garzarella 2021-02-12 8:07 ` 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=4094442C-C2A1-49D6-9901-7F3C73FABFC9@vmware.com \ --to=jhansen@vmware.com \ --cc=alex.popov@linux.com \ --cc=andraprs@amazon.com \ --cc=arseny.krasnov@kaspersky.com \ --cc=colin.king@canonical.com \ --cc=davem@davemloft.net \ --cc=jasowang@redhat.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=sgarzare@redhat.com \ --cc=stefanha@redhat.com \ --cc=stsp2@yandex.ru \ --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.