Re: [RFC PATCH v4 02/17] af_vsock: separate wait data loop
> On 7 Feb 2021, at 16:14, Arseny Krasnov 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
> ---
> 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), , 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), );
> - break;
> - }
> - /* Don't wait for non-blocking sockets. */
> - if (timeout == 0) {
> - err = -EAGAIN;
> - finish_wait(sk_sleep(sk), );
> - break;
> - }
> -
> - err = transport->notify_recv_pre_block(
> - vsk, target, _data);
> - if (err < 0) {
> - finish_wait(sk_sleep(sk), );
> - 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), );
> - break;
> - } else if (timeout == 0) {
> - err = -EAGAIN;
> - finish_wait(sk_sleep(sk), );
> - break;
> - }
> - } else {
> -
Re: [RFC PATCH v4 02/17] af_vsock: separate wait data loop
On Sun, Feb 07, 2021 at 06:14:48PM +0300, Arseny Krasnov 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
---
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.
+*/
+ if (data < 0)
+ return -ENOMEM;
+
+ /* Have some data, return. */
+ if (data)
+ return data;
IIUC here data must be != 0 so you can simply return data in any case.
Or cleaner, you can do 'break' instead of 'goto out' in the error paths
and after the while loop you can do something like this:
finish_wait(sk_sleep(sk), wait);
if (err)
return err;
if (data < 0)
return -ENOMEM;
return data;
}
+
+out:
+ finish_wait(sk_sleep(sk), wait);
+ return err;
+}
+
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), , 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), );
- break;
- }
- /* Don't wait for non-blocking sockets. */
- if (timeout == 0) {
- err = -EAGAIN;
- finish_wait(sk_sleep(sk), );
- break;
- }
-
- err = transport->notify_recv_pre_block(
- vsk, target, _data);
- if (err < 0) {
- finish_wait(sk_sleep(sk), );
- 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), );
- break;
- } else if (timeout == 0) {
- err = -EAGAIN;
- finish_wait(sk_sleep(sk), );
- break;
- }
- } else {
- ssize_t read;
+
