| Message ID | 20210508163738.3432975-1-arseny.krasnov@kaspersky.com (mailing list archive) |
|---|---|
| State | Superseded |
| Delegated to: | Netdev Maintainers |
| Headers | show |
| Series | virtio/vsock: introduce SOCK_SEQPACKET support | 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 | success | total: 0 errors, 0 warnings, 0 checks, 42 lines checked |
| netdev/build_allmodconfig_warn | success | Errors and warnings before: 0 this patch: 0 |
| netdev/header_inline | success | Link |
On Sat, May 08, 2021 at 07:37:35PM +0300, Arseny Krasnov wrote: >This add logic, that serializes write access to single socket >by multiple threads. It is implemented be adding field with TID >of current writer. When writer tries to send something, it checks >that field is -1(free), else it sleep in the same way as waiting >for free space at peers' side. > >Signed-off-by: Arseny Krasnov <arseny.krasnov@kaspersky.com> >--- > include/net/af_vsock.h | 1 + > net/vmw_vsock/af_vsock.c | 10 +++++++++- > 2 files changed, 10 insertions(+), 1 deletion(-) I think you forgot to move this patch at the beginning of the series. It's important because in this way we can backport to stable branches easily. About the implementation, can't we just add a mutex that we hold until we have sent all the payload? I need to check other implementations like TCP. > >diff --git a/include/net/af_vsock.h b/include/net/af_vsock.h >index 1747c0b564ef..413343f18e99 100644 >--- a/include/net/af_vsock.h >+++ b/include/net/af_vsock.h >@@ -69,6 +69,7 @@ struct vsock_sock { > u64 buffer_size; > u64 buffer_min_size; > u64 buffer_max_size; >+ pid_t tid_owner; > > /* Private to transport. */ > void *trans; >diff --git a/net/vmw_vsock/af_vsock.c b/net/vmw_vsock/af_vsock.c >index 7790728465f4..1fb4a1860f6d 100644 >--- a/net/vmw_vsock/af_vsock.c >+++ b/net/vmw_vsock/af_vsock.c >@@ -757,6 +757,7 @@ static struct sock *__vsock_create(struct net *net, > vsk->peer_shutdown = 0; > INIT_DELAYED_WORK(&vsk->connect_work, vsock_connect_timeout); > INIT_DELAYED_WORK(&vsk->pending_work, vsock_pending_work); >+ vsk->tid_owner = -1; > > psk = parent ? vsock_sk(parent) : NULL; > if (parent) { >@@ -1765,7 +1766,9 @@ static int vsock_connectible_sendmsg(struct socket *sock, struct msghdr *msg, > ssize_t written; > > add_wait_queue(sk_sleep(sk), &wait); >- while (vsock_stream_has_space(vsk) == 0 && >+ while ((vsock_stream_has_space(vsk) == 0 || >+ (vsk->tid_owner != current->pid && >+ vsk->tid_owner != -1)) && > sk->sk_err == 0 && > !(sk->sk_shutdown & SEND_SHUTDOWN) && > !(vsk->peer_shutdown & RCV_SHUTDOWN)) { >@@ -1796,6 +1799,8 @@ static int vsock_connectible_sendmsg(struct socket *sock, struct msghdr *msg, > goto out_err; > } > } >+ >+ vsk->tid_owner = current->pid; > remove_wait_queue(sk_sleep(sk), &wait); > > /* These checks occur both as part of and after the loop >@@ -1852,7 +1857,10 @@ static int vsock_connectible_sendmsg(struct socket *sock, struct msghdr *msg, > err = total_written; > } > out: >+ vsk->tid_owner = -1; > release_sock(sk); >+ sk->sk_write_space(sk); >+ Is this change related? Can you explain in the commit message why it is needed? > return err; > } > >-- >2.25.1 >
On 13.05.2021 17:01, Stefano Garzarella wrote: > On Sat, May 08, 2021 at 07:37:35PM +0300, Arseny Krasnov wrote: >> This add logic, that serializes write access to single socket >> by multiple threads. It is implemented be adding field with TID >> of current writer. When writer tries to send something, it checks >> that field is -1(free), else it sleep in the same way as waiting >> for free space at peers' side. >> >> Signed-off-by: Arseny Krasnov <arseny.krasnov@kaspersky.com> >> --- >> include/net/af_vsock.h | 1 + >> net/vmw_vsock/af_vsock.c | 10 +++++++++- >> 2 files changed, 10 insertions(+), 1 deletion(-) > I think you forgot to move this patch at the beginning of the series. > It's important because in this way we can backport to stable branches > easily. > > About the implementation, can't we just add a mutex that we hold until > we have sent all the payload? > > I need to check other implementations like TCP. Ok, i'll prepare this as separate patch > >> diff --git a/include/net/af_vsock.h b/include/net/af_vsock.h >> index 1747c0b564ef..413343f18e99 100644 >> --- a/include/net/af_vsock.h >> +++ b/include/net/af_vsock.h >> @@ -69,6 +69,7 @@ struct vsock_sock { >> u64 buffer_size; >> u64 buffer_min_size; >> u64 buffer_max_size; >> + pid_t tid_owner; >> >> /* Private to transport. */ >> void *trans; >> diff --git a/net/vmw_vsock/af_vsock.c b/net/vmw_vsock/af_vsock.c >> index 7790728465f4..1fb4a1860f6d 100644 >> --- a/net/vmw_vsock/af_vsock.c >> +++ b/net/vmw_vsock/af_vsock.c >> @@ -757,6 +757,7 @@ static struct sock *__vsock_create(struct net *net, >> vsk->peer_shutdown = 0; >> INIT_DELAYED_WORK(&vsk->connect_work, vsock_connect_timeout); >> INIT_DELAYED_WORK(&vsk->pending_work, vsock_pending_work); >> + vsk->tid_owner = -1; >> >> psk = parent ? vsock_sk(parent) : NULL; >> if (parent) { >> @@ -1765,7 +1766,9 @@ static int vsock_connectible_sendmsg(struct socket *sock, struct msghdr *msg, >> ssize_t written; >> >> add_wait_queue(sk_sleep(sk), &wait); >> - while (vsock_stream_has_space(vsk) == 0 && >> + while ((vsock_stream_has_space(vsk) == 0 || >> + (vsk->tid_owner != current->pid && >> + vsk->tid_owner != -1)) && >> sk->sk_err == 0 && >> !(sk->sk_shutdown & SEND_SHUTDOWN) && >> !(vsk->peer_shutdown & RCV_SHUTDOWN)) { >> @@ -1796,6 +1799,8 @@ static int vsock_connectible_sendmsg(struct socket *sock, struct msghdr *msg, >> goto out_err; >> } >> } >> + >> + vsk->tid_owner = current->pid; >> remove_wait_queue(sk_sleep(sk), &wait); >> >> /* These checks occur both as part of and after the loop >> @@ -1852,7 +1857,10 @@ static int vsock_connectible_sendmsg(struct socket *sock, struct msghdr *msg, >> err = total_written; >> } >> out: >> + vsk->tid_owner = -1; >> release_sock(sk); >> + sk->sk_write_space(sk); >> + > Is this change related? Can you explain in the commit message why it is > needed? > >> return err; >> } >> >> -- >> 2.25.1 >> >
On Thu, May 13, 2021 at 04:01:50PM +0200, Stefano Garzarella wrote: >On Sat, May 08, 2021 at 07:37:35PM +0300, Arseny Krasnov wrote: >>This add logic, that serializes write access to single socket >>by multiple threads. It is implemented be adding field with TID >>of current writer. When writer tries to send something, it checks >>that field is -1(free), else it sleep in the same way as waiting >>for free space at peers' side. >> >>Signed-off-by: Arseny Krasnov <arseny.krasnov@kaspersky.com> >>--- >>include/net/af_vsock.h | 1 + >>net/vmw_vsock/af_vsock.c | 10 +++++++++- >>2 files changed, 10 insertions(+), 1 deletion(-) > >I think you forgot to move this patch at the beginning of the series. >It's important because in this way we can backport to stable branches >easily. > >About the implementation, can't we just add a mutex that we hold until >we have sent all the payload? Re-thinking, I guess we can't because we have the timeout to deal with... > >I need to check other implementations like TCP. > Thanks, Stefano
On 13.05.2021 17:46, Stefano Garzarella wrote: > On Thu, May 13, 2021 at 04:01:50PM +0200, Stefano Garzarella wrote: >> On Sat, May 08, 2021 at 07:37:35PM +0300, Arseny Krasnov wrote: >>> This add logic, that serializes write access to single socket >>> by multiple threads. It is implemented be adding field with TID >>> of current writer. When writer tries to send something, it checks >>> that field is -1(free), else it sleep in the same way as waiting >>> for free space at peers' side. >>> >>> Signed-off-by: Arseny Krasnov <arseny.krasnov@kaspersky.com> >>> --- >>> include/net/af_vsock.h | 1 + >>> net/vmw_vsock/af_vsock.c | 10 +++++++++- >>> 2 files changed, 10 insertions(+), 1 deletion(-) >> I think you forgot to move this patch at the beginning of the series. >> It's important because in this way we can backport to stable branches >> easily. >> >> About the implementation, can't we just add a mutex that we hold until >> we have sent all the payload? > Re-thinking, I guess we can't because we have the timeout to deal > with... Yes, i forgot about why i've implemented it using 'tid_owner' :) > >> I need to check other implementations like TCP. >> > Thanks, > Stefano > >
On 13.05.2021 17:01, Stefano Garzarella wrote: > On Sat, May 08, 2021 at 07:37:35PM +0300, Arseny Krasnov wrote: >> This add logic, that serializes write access to single socket >> by multiple threads. It is implemented be adding field with TID >> of current writer. When writer tries to send something, it checks >> that field is -1(free), else it sleep in the same way as waiting >> for free space at peers' side. >> >> Signed-off-by: Arseny Krasnov <arseny.krasnov@kaspersky.com> >> --- >> include/net/af_vsock.h | 1 + >> net/vmw_vsock/af_vsock.c | 10 +++++++++- >> 2 files changed, 10 insertions(+), 1 deletion(-) > I think you forgot to move this patch at the beginning of the series. > It's important because in this way we can backport to stable branches > easily. > > About the implementation, can't we just add a mutex that we hold until > we have sent all the payload? > > I need to check other implementations like TCP. > >> diff --git a/include/net/af_vsock.h b/include/net/af_vsock.h >> index 1747c0b564ef..413343f18e99 100644 >> --- a/include/net/af_vsock.h >> +++ b/include/net/af_vsock.h >> @@ -69,6 +69,7 @@ struct vsock_sock { >> u64 buffer_size; >> u64 buffer_min_size; >> u64 buffer_max_size; >> + pid_t tid_owner; >> >> /* Private to transport. */ >> void *trans; >> diff --git a/net/vmw_vsock/af_vsock.c b/net/vmw_vsock/af_vsock.c >> index 7790728465f4..1fb4a1860f6d 100644 >> --- a/net/vmw_vsock/af_vsock.c >> +++ b/net/vmw_vsock/af_vsock.c >> @@ -757,6 +757,7 @@ static struct sock *__vsock_create(struct net *net, >> vsk->peer_shutdown = 0; >> INIT_DELAYED_WORK(&vsk->connect_work, vsock_connect_timeout); >> INIT_DELAYED_WORK(&vsk->pending_work, vsock_pending_work); >> + vsk->tid_owner = -1; >> >> psk = parent ? vsock_sk(parent) : NULL; >> if (parent) { >> @@ -1765,7 +1766,9 @@ static int vsock_connectible_sendmsg(struct socket *sock, struct msghdr *msg, >> ssize_t written; >> >> add_wait_queue(sk_sleep(sk), &wait); >> - while (vsock_stream_has_space(vsk) == 0 && >> + while ((vsock_stream_has_space(vsk) == 0 || >> + (vsk->tid_owner != current->pid && >> + vsk->tid_owner != -1)) && >> sk->sk_err == 0 && >> !(sk->sk_shutdown & SEND_SHUTDOWN) && >> !(vsk->peer_shutdown & RCV_SHUTDOWN)) { >> @@ -1796,6 +1799,8 @@ static int vsock_connectible_sendmsg(struct socket *sock, struct msghdr *msg, >> goto out_err; >> } >> } >> + >> + vsk->tid_owner = current->pid; >> remove_wait_queue(sk_sleep(sk), &wait); >> >> /* These checks occur both as part of and after the loop >> @@ -1852,7 +1857,10 @@ static int vsock_connectible_sendmsg(struct socket *sock, struct msghdr *msg, >> err = total_written; >> } >> out: >> + vsk->tid_owner = -1; >> release_sock(sk); >> + sk->sk_write_space(sk); >> + > Is this change related? Can you explain in the commit message why it is > needed? This is "unlocking" of socket > >> return err; >> } >> >> -- >> 2.25.1 >> >
On Thu, May 13, 2021 at 05:48:19PM +0300, Arseny Krasnov wrote: > >On 13.05.2021 17:46, Stefano Garzarella wrote: >> On Thu, May 13, 2021 at 04:01:50PM +0200, Stefano Garzarella wrote: >>> On Sat, May 08, 2021 at 07:37:35PM +0300, Arseny Krasnov wrote: >>>> This add logic, that serializes write access to single socket >>>> by multiple threads. It is implemented be adding field with TID >>>> of current writer. When writer tries to send something, it checks >>>> that field is -1(free), else it sleep in the same way as waiting >>>> for free space at peers' side. >>>> >>>> Signed-off-by: Arseny Krasnov <arseny.krasnov@kaspersky.com> >>>> --- >>>> include/net/af_vsock.h | 1 + >>>> net/vmw_vsock/af_vsock.c | 10 +++++++++- >>>> 2 files changed, 10 insertions(+), 1 deletion(-) >>> I think you forgot to move this patch at the beginning of the series. >>> It's important because in this way we can backport to stable branches >>> easily. >>> >>> About the implementation, can't we just add a mutex that we hold until >>> we have sent all the payload? >> Re-thinking, I guess we can't because we have the timeout to deal >> with... >Yes, i forgot about why i've implemented it using 'tid_owner' :) It is not clear to me if we need to do this also for stream. I think will be better to follow af_inet/af_unix, but I need to check their implementation. Thanks, Stefano
diff --git a/include/net/af_vsock.h b/include/net/af_vsock.h index 1747c0b564ef..413343f18e99 100644 --- a/include/net/af_vsock.h +++ b/include/net/af_vsock.h @@ -69,6 +69,7 @@ struct vsock_sock { u64 buffer_size; u64 buffer_min_size; u64 buffer_max_size; + pid_t tid_owner; /* Private to transport. */ void *trans; diff --git a/net/vmw_vsock/af_vsock.c b/net/vmw_vsock/af_vsock.c index 7790728465f4..1fb4a1860f6d 100644 --- a/net/vmw_vsock/af_vsock.c +++ b/net/vmw_vsock/af_vsock.c @@ -757,6 +757,7 @@ static struct sock *__vsock_create(struct net *net, vsk->peer_shutdown = 0; INIT_DELAYED_WORK(&vsk->connect_work, vsock_connect_timeout); INIT_DELAYED_WORK(&vsk->pending_work, vsock_pending_work); + vsk->tid_owner = -1; psk = parent ? vsock_sk(parent) : NULL; if (parent) { @@ -1765,7 +1766,9 @@ static int vsock_connectible_sendmsg(struct socket *sock, struct msghdr *msg, ssize_t written; add_wait_queue(sk_sleep(sk), &wait); - while (vsock_stream_has_space(vsk) == 0 && + while ((vsock_stream_has_space(vsk) == 0 || + (vsk->tid_owner != current->pid && + vsk->tid_owner != -1)) && sk->sk_err == 0 && !(sk->sk_shutdown & SEND_SHUTDOWN) && !(vsk->peer_shutdown & RCV_SHUTDOWN)) { @@ -1796,6 +1799,8 @@ static int vsock_connectible_sendmsg(struct socket *sock, struct msghdr *msg, goto out_err; } } + + vsk->tid_owner = current->pid; remove_wait_queue(sk_sleep(sk), &wait); /* These checks occur both as part of and after the loop @@ -1852,7 +1857,10 @@ static int vsock_connectible_sendmsg(struct socket *sock, struct msghdr *msg, err = total_written; } out: + vsk->tid_owner = -1; release_sock(sk); + sk->sk_write_space(sk); + return err; }
This add logic, that serializes write access to single socket by multiple threads. It is implemented be adding field with TID of current writer. When writer tries to send something, it checks that field is -1(free), else it sleep in the same way as waiting for free space at peers' side. Signed-off-by: Arseny Krasnov <arseny.krasnov@kaspersky.com> --- include/net/af_vsock.h | 1 + net/vmw_vsock/af_vsock.c | 10 +++++++++- 2 files changed, 10 insertions(+), 1 deletion(-)