diff mbox series

[RFC,v1,08/16] af_vsock: change SEQPACKET receive loop

Message ID 20210628100331.571056-1-arseny.krasnov@kaspersky.com
State New
Headers show
Series Improve SOCK_SEQPACKET receive logic | expand

Commit Message

Arseny Krasnov June 28, 2021, 10:03 a.m. UTC 
Receive "loop" now really loop: it reads fragments one by
one, sleeping if queue is empty.

NOTE: 'msg_ready' pointer is not passed to 'seqpacket_dequeue()'
here - it change callback interface, so it is moved to next patch.

Signed-off-by: Arseny Krasnov <arseny.krasnov@kaspersky.com>
---
 net/vmw_vsock/af_vsock.c | 31 ++++++++++++++++++++++---------
 1 file changed, 22 insertions(+), 9 deletions(-)

Comments

Stefano Garzarella June 30, 2021, 12:12 p.m. UTC | #1 
On Mon, Jun 28, 2021 at 01:03:28PM +0300, Arseny Krasnov wrote:
>Receive "loop" now really loop: it reads fragments one by

>one, sleeping if queue is empty.

>

>NOTE: 'msg_ready' pointer is not passed to 'seqpacket_dequeue()'

>here - it change callback interface, so it is moved to next patch.

>

>Signed-off-by: Arseny Krasnov <arseny.krasnov@kaspersky.com>

>---

> net/vmw_vsock/af_vsock.c | 31 ++++++++++++++++++++++---------

> 1 file changed, 22 insertions(+), 9 deletions(-)


I think you can merge patches 8, 9, and 10 together since we
are touching the seqpacket_dequeue() behaviour.

Then you can remove in separate patches the unneeded parts (e.g.
seqpacket_has_data, msg_count, etc.).

Thanks,
Stefano

>

>diff --git a/net/vmw_vsock/af_vsock.c b/net/vmw_vsock/af_vsock.c

>index 59ce35da2e5b..9552f05119f2 100644

>--- a/net/vmw_vsock/af_vsock.c

>+++ b/net/vmw_vsock/af_vsock.c

>@@ -2003,6 +2003,7 @@ static int __vsock_seqpacket_recvmsg(struct sock *sk, struct msghdr *msg,

>                                    size_t len, int flags)

> {

>       const struct vsock_transport *transport;

>+      bool msg_ready;

>       struct vsock_sock *vsk;

>       ssize_t record_len;

>       long timeout;

>@@ -2013,23 +2014,36 @@ static int __vsock_seqpacket_recvmsg(struct sock *sk, struct msghdr *msg,

>       transport = vsk->transport;

>

>       timeout = sock_rcvtimeo(sk, flags & MSG_DONTWAIT);

>+      msg_ready = false;

>+      record_len = 0;

>

>-      err = vsock_connectible_wait_data(sk, &wait, timeout, NULL, 0);

>-      if (err <= 0)

>-              goto out;

>+      while (!msg_ready) {

>+              ssize_t fragment_len;

>+              int intr_err;

>

>-      record_len = transport->seqpacket_dequeue(vsk, msg, flags);

>+              intr_err = vsock_connectible_wait_data(sk, &wait, timeout, NULL, 0);

>+              if (intr_err <= 0) {

>+                      err = intr_err;

>+                      break;

>+              }

>

>-      if (record_len < 0) {

>-              err = -ENOMEM;

>-              goto out;

>+              fragment_len = transport->seqpacket_dequeue(vsk, msg, flags);

>+

>+              if (fragment_len < 0) {

>+                      err = -ENOMEM;

>+                      break;

>+              }

>+

>+              record_len += fragment_len;

>       }

>

>       if (sk->sk_err) {

>               err = -sk->sk_err;

>       } else if (sk->sk_shutdown & RCV_SHUTDOWN) {

>               err = 0;

>-      } else {

>+      }

>+

>+      if (msg_ready && !err) {

>               /* User sets MSG_TRUNC, so return real length of

>                * packet.

>                */

>@@ -2045,7 +2059,6 @@ static int __vsock_seqpacket_recvmsg(struct sock *sk, struct msghdr *msg,

>                       msg->msg_flags |= MSG_TRUNC;

>       }

>

>-out:

>       return err;

> }

>

>--

>2.25.1

>
Arseny Krasnov June 30, 2021, 5:47 p.m. UTC | #2 
On 30.06.2021 15:12, Stefano Garzarella wrote:
> On Mon, Jun 28, 2021 at 01:03:28PM +0300, Arseny Krasnov wrote:

>> Receive "loop" now really loop: it reads fragments one by

>> one, sleeping if queue is empty.

>>

>> NOTE: 'msg_ready' pointer is not passed to 'seqpacket_dequeue()'

>> here - it change callback interface, so it is moved to next patch.

>>

>> Signed-off-by: Arseny Krasnov <arseny.krasnov@kaspersky.com>

>> ---

>> net/vmw_vsock/af_vsock.c | 31 ++++++++++++++++++++++---------

>> 1 file changed, 22 insertions(+), 9 deletions(-)

> I think you can merge patches 8, 9, and 10 together since we

> are touching the seqpacket_dequeue() behaviour.

>

> Then you can remove in separate patches the unneeded parts (e.g.

> seqpacket_has_data, msg_count, etc.).

>

> Thanks,

> Stefano

Ack
>

>> diff --git a/net/vmw_vsock/af_vsock.c b/net/vmw_vsock/af_vsock.c

>> index 59ce35da2e5b..9552f05119f2 100644

>> --- a/net/vmw_vsock/af_vsock.c

>> +++ b/net/vmw_vsock/af_vsock.c

>> @@ -2003,6 +2003,7 @@ static int __vsock_seqpacket_recvmsg(struct sock *sk, struct msghdr *msg,

>>                                    size_t len, int flags)

>> {

>>       const struct vsock_transport *transport;

>> +      bool msg_ready;

>>       struct vsock_sock *vsk;

>>       ssize_t record_len;

>>       long timeout;

>> @@ -2013,23 +2014,36 @@ static int __vsock_seqpacket_recvmsg(struct sock *sk, struct msghdr *msg,

>>       transport = vsk->transport;

>>

>>       timeout = sock_rcvtimeo(sk, flags & MSG_DONTWAIT);

>> +      msg_ready = false;

>> +      record_len = 0;

>>

>> -      err = vsock_connectible_wait_data(sk, &wait, timeout, NULL, 0);

>> -      if (err <= 0)

>> -              goto out;

>> +      while (!msg_ready) {

>> +              ssize_t fragment_len;

>> +              int intr_err;

>>

>> -      record_len = transport->seqpacket_dequeue(vsk, msg, flags);

>> +              intr_err = vsock_connectible_wait_data(sk, &wait, timeout, NULL, 0);

>> +              if (intr_err <= 0) {

>> +                      err = intr_err;

>> +                      break;

>> +              }

>>

>> -      if (record_len < 0) {

>> -              err = -ENOMEM;

>> -              goto out;

>> +              fragment_len = transport->seqpacket_dequeue(vsk, msg, flags);

>> +

>> +              if (fragment_len < 0) {

>> +                      err = -ENOMEM;

>> +                      break;

>> +              }

>> +

>> +              record_len += fragment_len;

>>       }

>>

>>       if (sk->sk_err) {

>>               err = -sk->sk_err;

>>       } else if (sk->sk_shutdown & RCV_SHUTDOWN) {

>>               err = 0;

>> -      } else {

>> +      }

>> +

>> +      if (msg_ready && !err) {

>>               /* User sets MSG_TRUNC, so return real length of

>>                * packet.

>>                */

>> @@ -2045,7 +2059,6 @@ static int __vsock_seqpacket_recvmsg(struct sock *sk, struct msghdr *msg,

>>                       msg->msg_flags |= MSG_TRUNC;

>>       }

>>

>> -out:

>>       return err;

>> }

>>

>> --

>> 2.25.1

>>

>
diff mbox series

Patch

diff --git a/net/vmw_vsock/af_vsock.c b/net/vmw_vsock/af_vsock.c
index 59ce35da2e5b..9552f05119f2 100644
--- a/net/vmw_vsock/af_vsock.c
+++ b/net/vmw_vsock/af_vsock.c
@@ -2003,6 +2003,7 @@  static int __vsock_seqpacket_recvmsg(struct sock *sk, struct msghdr *msg,
 				     size_t len, int flags)
 {
 	const struct vsock_transport *transport;
+	bool msg_ready;
 	struct vsock_sock *vsk;
 	ssize_t record_len;
 	long timeout;
@@ -2013,23 +2014,36 @@  static int __vsock_seqpacket_recvmsg(struct sock *sk, struct msghdr *msg,
 	transport = vsk->transport;
 
 	timeout = sock_rcvtimeo(sk, flags & MSG_DONTWAIT);
+	msg_ready = false;
+	record_len = 0;
 
-	err = vsock_connectible_wait_data(sk, &wait, timeout, NULL, 0);
-	if (err <= 0)
-		goto out;
+	while (!msg_ready) {
+		ssize_t fragment_len;
+		int intr_err;
 
-	record_len = transport->seqpacket_dequeue(vsk, msg, flags);
+		intr_err = vsock_connectible_wait_data(sk, &wait, timeout, NULL, 0);
+		if (intr_err <= 0) {
+			err = intr_err;
+			break;
+		}
 
-	if (record_len < 0) {
-		err = -ENOMEM;
-		goto out;
+		fragment_len = transport->seqpacket_dequeue(vsk, msg, flags);
+
+		if (fragment_len < 0) {
+			err = -ENOMEM;
+			break;
+		}
+
+		record_len += fragment_len;
 	}
 
 	if (sk->sk_err) {
 		err = -sk->sk_err;
 	} else if (sk->sk_shutdown & RCV_SHUTDOWN) {
 		err = 0;
-	} else {
+	}
+
+	if (msg_ready && !err) {
 		/* User sets MSG_TRUNC, so return real length of
 		 * packet.
 		 */
@@ -2045,7 +2059,6 @@  static int __vsock_seqpacket_recvmsg(struct sock *sk, struct msghdr *msg,
 			msg->msg_flags |= MSG_TRUNC;
 	}
 
-out:
 	return err;
 }