diff mbox series

[RFC,v9,19/19] af_vsock: serialize writes to shared socket

Message ID 20210508163738.3432975-1-arseny.krasnov@kaspersky.com (mailing list archive)
State New, archived
Headers show
Series virtio/vsock: introduce SOCK_SEQPACKET support | expand

Commit Message

Arseny Krasnov May 8, 2021, 4:37 p.m. UTC
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(-)

Comments

Stefano Garzarella May 13, 2021, 2:01 p.m. UTC | #1
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
>
Arseny Krasnov May 13, 2021, 2:45 p.m. UTC | #2
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
>>
>
Stefano Garzarella May 13, 2021, 2:46 p.m. UTC | #3
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
Arseny Krasnov May 13, 2021, 2:48 p.m. UTC | #4
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
>
>
Arseny Krasnov May 13, 2021, 2:51 p.m. UTC | #5
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
>>
>
Stefano Garzarella May 13, 2021, 3:41 p.m. UTC | #6
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 mbox series

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);
+
 	return err;
 }