diff mbox series

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

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

Commit Message

Arseny Krasnov April 13, 2021, 12:47 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.

This implementation is PoC and not related to SEQPACKET close, so
i've placed it after whole patchset.

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

Arseny Krasnov April 14, 2021, 10:51 a.m. UTC | #1
On 13.04.2021 15:47, 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.
>
> This implementation is PoC and not related to SEQPACKET close, so
> i've placed it after whole patchset.
>
> 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(-)
>
> diff --git a/include/net/af_vsock.h b/include/net/af_vsock.h
> index 53d3f33dbdbf..786df80b9fc3 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 54bee7e643f4..d00f8c07a9d3 100644
> --- a/net/vmw_vsock/af_vsock.c
> +++ b/net/vmw_vsock/af_vsock.c
> @@ -1765,7 +1765,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 +1798,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 +1856,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;
>  }
>  
> @@ -2199,6 +2206,7 @@ static int vsock_create(struct net *net, struct socket *sock,
>  		return -ENOMEM;
>  
>  	vsk = vsock_sk(sk);
> +	vsk->tid_owner = -1;
This must be moved to '__vsock_create()'
>  
>  	if (sock->type == SOCK_DGRAM) {
>  		ret = vsock_assign_transport(vsk, NULL);
Stefano Garzarella April 21, 2021, 9:38 a.m. UTC | #2
On Wed, Apr 14, 2021 at 01:51:17PM +0300, Arseny Krasnov wrote:
>
>On 13.04.2021 15:47, 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.
>>
>> This implementation is PoC and not related to SEQPACKET close, so
>> i've placed it after whole patchset.
>>
>> 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(-)
>>
>> diff --git a/include/net/af_vsock.h b/include/net/af_vsock.h
>> index 53d3f33dbdbf..786df80b9fc3 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 54bee7e643f4..d00f8c07a9d3 100644
>> --- a/net/vmw_vsock/af_vsock.c
>> +++ b/net/vmw_vsock/af_vsock.c
>> @@ -1765,7 +1765,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 +1798,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 +1856,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;
>>  }
>>
>> @@ -2199,6 +2206,7 @@ static int vsock_create(struct net *net, struct socket *sock,
>>  		return -ENOMEM;
>>
>>  	vsk = vsock_sk(sk);
>> +	vsk->tid_owner = -1;
>This must be moved to '__vsock_create()'

Okay, I'll review the next version.

In order to backport this fix to stable branches I think is better to 
move at the beginning of this series or even out as a separate patch.

Thanks,
Stefano

>>
>>  	if (sock->type == SOCK_DGRAM) {
>>  		ret = vsock_assign_transport(vsk, NULL);
>
Arseny Krasnov April 21, 2021, 3:02 p.m. UTC | #3
On 21.04.2021 12:38, Stefano Garzarella wrote:
> On Wed, Apr 14, 2021 at 01:51:17PM +0300, Arseny Krasnov wrote:
>> On 13.04.2021 15:47, 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.
>>>
>>> This implementation is PoC and not related to SEQPACKET close, so
>>> i've placed it after whole patchset.
>>>
>>> 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(-)
>>>
>>> diff --git a/include/net/af_vsock.h b/include/net/af_vsock.h
>>> index 53d3f33dbdbf..786df80b9fc3 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 54bee7e643f4..d00f8c07a9d3 100644
>>> --- a/net/vmw_vsock/af_vsock.c
>>> +++ b/net/vmw_vsock/af_vsock.c
>>> @@ -1765,7 +1765,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 +1798,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 +1856,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;
>>>  }
>>>
>>> @@ -2199,6 +2206,7 @@ static int vsock_create(struct net *net, struct socket *sock,
>>>  		return -ENOMEM;
>>>
>>>  	vsk = vsock_sk(sk);
>>> +	vsk->tid_owner = -1;
>> This must be moved to '__vsock_create()'
> Okay, I'll review the next version.
>
> In order to backport this fix to stable branches I think is better to 
> move at the beginning of this series or even out as a separate patch.

Ok, i'll put it as first patch of patchset. I don't like to prepare it as

separate patch, because SEQPACKET will use this fix.

>
> Thanks,
> Stefano
>
>>>  	if (sock->type == SOCK_DGRAM) {
>>>  		ret = vsock_assign_transport(vsk, NULL);
>
diff mbox series

Patch

diff --git a/include/net/af_vsock.h b/include/net/af_vsock.h
index 53d3f33dbdbf..786df80b9fc3 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 54bee7e643f4..d00f8c07a9d3 100644
--- a/net/vmw_vsock/af_vsock.c
+++ b/net/vmw_vsock/af_vsock.c
@@ -1765,7 +1765,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 +1798,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 +1856,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;
 }
 
@@ -2199,6 +2206,7 @@  static int vsock_create(struct net *net, struct socket *sock,
 		return -ENOMEM;
 
 	vsk = vsock_sk(sk);
+	vsk->tid_owner = -1;
 
 	if (sock->type == SOCK_DGRAM) {
 		ret = vsock_assign_transport(vsk, NULL);