| Message ID | 20210628100331.571056-1-arseny.krasnov@kaspersky.com (mailing list archive) |
|---|---|
| State | RFC |
| Delegated to: | Netdev Maintainers |
| Headers | show |
| Series | Improve SOCK_SEQPACKET receive logic | expand |
| Context | Check | Description |
|---|---|---|
| netdev/cover_letter | success | Link |
| netdev/fixes_present | success | Link |
| netdev/patch_count | fail | Series longer than 15 patches |
| netdev/tree_selection | success | Guessed tree name to be net-next |
| netdev/subject_prefix | success | Link |
| netdev/cc_maintainers | success | CCed 8 of 8 maintainers |
| netdev/source_inline | success | Was 0 now: 0 |
| netdev/verify_signedoff | success | Link |
| netdev/module_param | success | Was 0 now: 0 |
| netdev/build_32bit | success | Errors and warnings before: 0 this patch: 0 |
| netdev/kdoc | success | Errors and warnings before: 0 this patch: 0 |
| netdev/verify_fixes | success | Link |
| netdev/checkpatch | warning | WARNING: line length of 84 exceeds 80 columns |
| netdev/build_allmodconfig_warn | success | Errors and warnings before: 0 this patch: 0 |
| netdev/header_inline | success | Link |
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 >
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 --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; }
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(-)