diff mbox series

[net-next,v1,08/12] vsock: enable setting SO_ZEROCOPY

Message ID 20230922052428.4005676-9-avkrasnov@salutedevices.com (mailing list archive)
State Superseded
Delegated to: Netdev Maintainers
Headers show
Series vsock/virtio: continue MSG_ZEROCOPY support | expand

Checks

Context Check Description
netdev/series_format success Posting correctly formatted
netdev/tree_selection success Clearly marked for net-next, async
netdev/fixes_present success Fixes tag not required for -next series
netdev/header_inline success No static functions without inline keyword in header files
netdev/build_32bit success Errors and warnings before: 1340 this patch: 1340
netdev/cc_maintainers success CCed 7 of 7 maintainers
netdev/build_clang success Errors and warnings before: 1363 this patch: 1363
netdev/verify_signedoff success Signed-off-by tag matches author and committer
netdev/deprecated_api success None detected
netdev/check_selftest success No net selftest shell script
netdev/verify_fixes success No Fixes tag
netdev/build_allmodconfig_warn success Errors and warnings before: 1363 this patch: 1363
netdev/checkpatch warning WARNING: line length of 85 exceeds 80 columns
netdev/kdoc success Errors and warnings before: 0 this patch: 0
netdev/source_inline success Was 0 now: 0

Commit Message

Arseniy Krasnov Sept. 22, 2023, 5:24 a.m. UTC 
For AF_VSOCK, zerocopy tx mode depends on transport, so this option must
be set in AF_VSOCK implementation where transport is accessible (if
transport is not set during setting SO_ZEROCOPY: for example socket is
not connected, then SO_ZEROCOPY will be enabled, but once transport will
be assigned, support of this type of transmission will be checked).

To handle SO_ZEROCOPY, AF_VSOCK implementation uses SOCK_CUSTOM_SOCKOPT
bit, thus handling SOL_SOCKET option operations, but all of them except
SO_ZEROCOPY will be forwarded to the generic handler by calling
'sock_setsockopt()'.

Signed-off-by: Arseniy Krasnov <avkrasnov@salutedevices.com>
---
 Changelog:
 v5(big patchset) -> v1:
  * Compact 'if' conditions.
  * Rename 'zc_val' to 'zerocopy'.
  * Use 'zerocopy' value directly in 'sock_valbool_flag()', without
    ?: operator.
  * Set 'SOCK_CUSTOM_SOCKOPT' bit for connectible sockets only, as
    suggested by Bobby Eshleman <bobbyeshleman@gmail.com>.

 net/vmw_vsock/af_vsock.c | 46 ++++++++++++++++++++++++++++++++++++++--
 1 file changed, 44 insertions(+), 2 deletions(-)

Comments

Stefano Garzarella Sept. 26, 2023, 12:56 p.m. UTC | #1 
On Fri, Sep 22, 2023 at 08:24:24AM +0300, Arseniy Krasnov wrote:
>For AF_VSOCK, zerocopy tx mode depends on transport, so this option must
>be set in AF_VSOCK implementation where transport is accessible (if
>transport is not set during setting SO_ZEROCOPY: for example socket is
>not connected, then SO_ZEROCOPY will be enabled, but once transport will
>be assigned, support of this type of transmission will be checked).
>
>To handle SO_ZEROCOPY, AF_VSOCK implementation uses SOCK_CUSTOM_SOCKOPT
>bit, thus handling SOL_SOCKET option operations, but all of them except
>SO_ZEROCOPY will be forwarded to the generic handler by calling
>'sock_setsockopt()'.
>
>Signed-off-by: Arseniy Krasnov <avkrasnov@salutedevices.com>
>---
> Changelog:
> v5(big patchset) -> v1:
>  * Compact 'if' conditions.
>  * Rename 'zc_val' to 'zerocopy'.
>  * Use 'zerocopy' value directly in 'sock_valbool_flag()', without
>    ?: operator.
>  * Set 'SOCK_CUSTOM_SOCKOPT' bit for connectible sockets only, as
>    suggested by Bobby Eshleman <bobbyeshleman@gmail.com>.
>
> net/vmw_vsock/af_vsock.c | 46 ++++++++++++++++++++++++++++++++++++++--
> 1 file changed, 44 insertions(+), 2 deletions(-)
>
>diff --git a/net/vmw_vsock/af_vsock.c b/net/vmw_vsock/af_vsock.c
>index 482300eb88e0..c05a42e02a17 100644
>--- a/net/vmw_vsock/af_vsock.c
>+++ b/net/vmw_vsock/af_vsock.c
>@@ -1406,8 +1406,16 @@ static int vsock_connect(struct socket *sock, struct sockaddr *addr,
> 			goto out;
> 		}
>
>-		if (vsock_msgzerocopy_allow(transport))
>+		if (vsock_msgzerocopy_allow(transport)) {
> 			set_bit(SOCK_SUPPORT_ZC, &sk->sk_socket->flags);
>+		} else if (sock_flag(sk, SOCK_ZEROCOPY)) {
>+			/* If this option was set before 'connect()',
>+			 * when transport was unknown, check that this
>+			 * feature is supported here.
>+			 */
>+			err = -EOPNOTSUPP;
>+			goto out;
>+		}
>
> 		err = vsock_auto_bind(vsk);
> 		if (err)
>@@ -1643,7 +1651,7 @@ static int vsock_connectible_setsockopt(struct socket *sock,
> 	const struct vsock_transport *transport;
> 	u64 val;
>
>-	if (level != AF_VSOCK)
>+	if (level != AF_VSOCK && level != SOL_SOCKET)
> 		return -ENOPROTOOPT;
>
> #define COPY_IN(_v)                                       \
>@@ -1666,6 +1674,34 @@ static int vsock_connectible_setsockopt(struct socket *sock,
>
> 	transport = vsk->transport;
>
>+	if (level == SOL_SOCKET) {
>+		int zerocopy;
>+
>+		if (optname != SO_ZEROCOPY) {
>+			release_sock(sk);
>+			return sock_setsockopt(sock, level, optname, optval, optlen);
>+		}
>+
>+		/* Use 'int' type here, because variable to
>+		 * set this option usually has this type.
>+		 */
>+		COPY_IN(zerocopy);
>+
>+		if (zerocopy < 0 || zerocopy > 1) {
>+			err = -EINVAL;
>+			goto exit;
>+		}
>+
>+		if (transport && !vsock_msgzerocopy_allow(transport)) {
>+			err = -EOPNOTSUPP;
>+			goto exit;
>+		}
>+
>+		sock_valbool_flag(sk, SOCK_ZEROCOPY,
>+				  zerocopy);

it's not necessary to wrap this call.

>+		goto exit;
>+	}
>+
> 	switch (optname) {
> 	case SO_VM_SOCKETS_BUFFER_SIZE:
> 		COPY_IN(val);
>@@ -2322,6 +2358,12 @@ static int vsock_create(struct net *net, struct socket *sock,
> 		}
> 	}
>
>+	/* SOCK_DGRAM doesn't have 'setsockopt' callback set in its
>+	 * proto_ops, so there is no handler for custom logic.
>+	 */
>+	if (sock_type_connectible(sock->type))
>+		set_bit(SOCK_CUSTOM_SOCKOPT, &sk->sk_socket->flags);
>+
> 	vsock_insert_unbound(vsk);
>
> 	return 0;
>-- 
>2.25.1
>
Arseniy Krasnov Sept. 26, 2023, 7:38 p.m. UTC | #2 
On 26.09.2023 15:56, Stefano Garzarella wrote:
> On Fri, Sep 22, 2023 at 08:24:24AM +0300, Arseniy Krasnov wrote:
>> For AF_VSOCK, zerocopy tx mode depends on transport, so this option must
>> be set in AF_VSOCK implementation where transport is accessible (if
>> transport is not set during setting SO_ZEROCOPY: for example socket is
>> not connected, then SO_ZEROCOPY will be enabled, but once transport will
>> be assigned, support of this type of transmission will be checked).
>>
>> To handle SO_ZEROCOPY, AF_VSOCK implementation uses SOCK_CUSTOM_SOCKOPT
>> bit, thus handling SOL_SOCKET option operations, but all of them except
>> SO_ZEROCOPY will be forwarded to the generic handler by calling
>> 'sock_setsockopt()'.
>>
>> Signed-off-by: Arseniy Krasnov <avkrasnov@salutedevices.com>
>> ---
>> Changelog:
>> v5(big patchset) -> v1:
>>  * Compact 'if' conditions.
>>  * Rename 'zc_val' to 'zerocopy'.
>>  * Use 'zerocopy' value directly in 'sock_valbool_flag()', without
>>    ?: operator.
>>  * Set 'SOCK_CUSTOM_SOCKOPT' bit for connectible sockets only, as
>>    suggested by Bobby Eshleman <bobbyeshleman@gmail.com>.
>>
>> net/vmw_vsock/af_vsock.c | 46 ++++++++++++++++++++++++++++++++++++++--
>> 1 file changed, 44 insertions(+), 2 deletions(-)
>>
>> diff --git a/net/vmw_vsock/af_vsock.c b/net/vmw_vsock/af_vsock.c
>> index 482300eb88e0..c05a42e02a17 100644
>> --- a/net/vmw_vsock/af_vsock.c
>> +++ b/net/vmw_vsock/af_vsock.c
>> @@ -1406,8 +1406,16 @@ static int vsock_connect(struct socket *sock, struct sockaddr *addr,
>>             goto out;
>>         }
>>
>> -        if (vsock_msgzerocopy_allow(transport))
>> +        if (vsock_msgzerocopy_allow(transport)) {
>>             set_bit(SOCK_SUPPORT_ZC, &sk->sk_socket->flags);
>> +        } else if (sock_flag(sk, SOCK_ZEROCOPY)) {
>> +            /* If this option was set before 'connect()',
>> +             * when transport was unknown, check that this
>> +             * feature is supported here.
>> +             */
>> +            err = -EOPNOTSUPP;
>> +            goto out;
>> +        }
>>
>>         err = vsock_auto_bind(vsk);
>>         if (err)
>> @@ -1643,7 +1651,7 @@ static int vsock_connectible_setsockopt(struct socket *sock,
>>     const struct vsock_transport *transport;
>>     u64 val;
>>
>> -    if (level != AF_VSOCK)
>> +    if (level != AF_VSOCK && level != SOL_SOCKET)
>>         return -ENOPROTOOPT;
>>
>> #define COPY_IN(_v)                                       \
>> @@ -1666,6 +1674,34 @@ static int vsock_connectible_setsockopt(struct socket *sock,
>>
>>     transport = vsk->transport;
>>
>> +    if (level == SOL_SOCKET) {
>> +        int zerocopy;
>> +
>> +        if (optname != SO_ZEROCOPY) {
>> +            release_sock(sk);
>> +            return sock_setsockopt(sock, level, optname, optval, optlen);
>> +        }
>> +
>> +        /* Use 'int' type here, because variable to
>> +         * set this option usually has this type.
>> +         */
>> +        COPY_IN(zerocopy);
>> +
>> +        if (zerocopy < 0 || zerocopy > 1) {
>> +            err = -EINVAL;
>> +            goto exit;
>> +        }
>> +
>> +        if (transport && !vsock_msgzerocopy_allow(transport)) {
>> +            err = -EOPNOTSUPP;
>> +            goto exit;
>> +        }
>> +
>> +        sock_valbool_flag(sk, SOCK_ZEROCOPY,
>> +                  zerocopy);
> 
> it's not necessary to wrap this call.

Sorry, what do you mean ?

Thanks, Arseniy

> 
>> +        goto exit;
>> +    }
>> +
>>     switch (optname) {
>>     case SO_VM_SOCKETS_BUFFER_SIZE:
>>         COPY_IN(val);
>> @@ -2322,6 +2358,12 @@ static int vsock_create(struct net *net, struct socket *sock,
>>         }
>>     }
>>
>> +    /* SOCK_DGRAM doesn't have 'setsockopt' callback set in its
>> +     * proto_ops, so there is no handler for custom logic.
>> +     */
>> +    if (sock_type_connectible(sock->type))
>> +        set_bit(SOCK_CUSTOM_SOCKOPT, &sk->sk_socket->flags);
>> +
>>     vsock_insert_unbound(vsk);
>>
>>     return 0;
>> -- 
>> 2.25.1
>>
>
Stefano Garzarella Sept. 27, 2023, 7:35 a.m. UTC | #3 
On Tue, Sep 26, 2023 at 10:38:06PM +0300, Arseniy Krasnov wrote:
>
>
>On 26.09.2023 15:56, Stefano Garzarella wrote:
>> On Fri, Sep 22, 2023 at 08:24:24AM +0300, Arseniy Krasnov wrote:
>>> For AF_VSOCK, zerocopy tx mode depends on transport, so this option must
>>> be set in AF_VSOCK implementation where transport is accessible (if
>>> transport is not set during setting SO_ZEROCOPY: for example socket is
>>> not connected, then SO_ZEROCOPY will be enabled, but once transport will
>>> be assigned, support of this type of transmission will be checked).
>>>
>>> To handle SO_ZEROCOPY, AF_VSOCK implementation uses SOCK_CUSTOM_SOCKOPT
>>> bit, thus handling SOL_SOCKET option operations, but all of them except
>>> SO_ZEROCOPY will be forwarded to the generic handler by calling
>>> 'sock_setsockopt()'.
>>>
>>> Signed-off-by: Arseniy Krasnov <avkrasnov@salutedevices.com>
>>> ---
>>> Changelog:
>>> v5(big patchset) -> v1:
>>>  * Compact 'if' conditions.
>>>  * Rename 'zc_val' to 'zerocopy'.
>>>  * Use 'zerocopy' value directly in 'sock_valbool_flag()', without
>>>    ?: operator.
>>>  * Set 'SOCK_CUSTOM_SOCKOPT' bit for connectible sockets only, as
>>>    suggested by Bobby Eshleman <bobbyeshleman@gmail.com>.
>>>
>>> net/vmw_vsock/af_vsock.c | 46 ++++++++++++++++++++++++++++++++++++++--
>>> 1 file changed, 44 insertions(+), 2 deletions(-)
>>>
>>> diff --git a/net/vmw_vsock/af_vsock.c b/net/vmw_vsock/af_vsock.c
>>> index 482300eb88e0..c05a42e02a17 100644
>>> --- a/net/vmw_vsock/af_vsock.c
>>> +++ b/net/vmw_vsock/af_vsock.c
>>> @@ -1406,8 +1406,16 @@ static int vsock_connect(struct socket *sock, struct sockaddr *addr,
>>>             goto out;
>>>         }
>>>
>>> -        if (vsock_msgzerocopy_allow(transport))
>>> +        if (vsock_msgzerocopy_allow(transport)) {
>>>             set_bit(SOCK_SUPPORT_ZC, &sk->sk_socket->flags);
>>> +        } else if (sock_flag(sk, SOCK_ZEROCOPY)) {
>>> +            /* If this option was set before 'connect()',
>>> +             * when transport was unknown, check that this
>>> +             * feature is supported here.
>>> +             */
>>> +            err = -EOPNOTSUPP;
>>> +            goto out;
>>> +        }
>>>
>>>         err = vsock_auto_bind(vsk);
>>>         if (err)
>>> @@ -1643,7 +1651,7 @@ static int vsock_connectible_setsockopt(struct socket *sock,
>>>     const struct vsock_transport *transport;
>>>     u64 val;
>>>
>>> -    if (level != AF_VSOCK)
>>> +    if (level != AF_VSOCK && level != SOL_SOCKET)
>>>         return -ENOPROTOOPT;
>>>
>>> #define COPY_IN(_v)                                       \
>>> @@ -1666,6 +1674,34 @@ static int vsock_connectible_setsockopt(struct socket *sock,
>>>
>>>     transport = vsk->transport;
>>>
>>> +    if (level == SOL_SOCKET) {
>>> +        int zerocopy;
>>> +
>>> +        if (optname != SO_ZEROCOPY) {
>>> +            release_sock(sk);
>>> +            return sock_setsockopt(sock, level, optname, optval, optlen);
>>> +        }
>>> +
>>> +        /* Use 'int' type here, because variable to
>>> +         * set this option usually has this type.
>>> +         */
>>> +        COPY_IN(zerocopy);
>>> +
>>> +        if (zerocopy < 0 || zerocopy > 1) {
>>> +            err = -EINVAL;
>>> +            goto exit;
>>> +        }
>>> +
>>> +        if (transport && !vsock_msgzerocopy_allow(transport)) {
>>> +            err = -EOPNOTSUPP;
>>> +            goto exit;
>>> +        }
>>> +
>>> +        sock_valbool_flag(sk, SOCK_ZEROCOPY,
>>> +                  zerocopy);
>>
>> it's not necessary to wrap this call.
>
>Sorry, what do you mean ?

I mean that can be on the same line:

	sock_valbool_flag(sk, SOCK_ZEROCOPY, zerocopy);

Stefano
diff mbox series

Patch

diff --git a/net/vmw_vsock/af_vsock.c b/net/vmw_vsock/af_vsock.c
index 482300eb88e0..c05a42e02a17 100644
--- a/net/vmw_vsock/af_vsock.c
+++ b/net/vmw_vsock/af_vsock.c
@@ -1406,8 +1406,16 @@  static int vsock_connect(struct socket *sock, struct sockaddr *addr,
 			goto out;
 		}
 
-		if (vsock_msgzerocopy_allow(transport))
+		if (vsock_msgzerocopy_allow(transport)) {
 			set_bit(SOCK_SUPPORT_ZC, &sk->sk_socket->flags);
+		} else if (sock_flag(sk, SOCK_ZEROCOPY)) {
+			/* If this option was set before 'connect()',
+			 * when transport was unknown, check that this
+			 * feature is supported here.
+			 */
+			err = -EOPNOTSUPP;
+			goto out;
+		}
 
 		err = vsock_auto_bind(vsk);
 		if (err)
@@ -1643,7 +1651,7 @@  static int vsock_connectible_setsockopt(struct socket *sock,
 	const struct vsock_transport *transport;
 	u64 val;
 
-	if (level != AF_VSOCK)
+	if (level != AF_VSOCK && level != SOL_SOCKET)
 		return -ENOPROTOOPT;
 
 #define COPY_IN(_v)                                       \
@@ -1666,6 +1674,34 @@  static int vsock_connectible_setsockopt(struct socket *sock,
 
 	transport = vsk->transport;
 
+	if (level == SOL_SOCKET) {
+		int zerocopy;
+
+		if (optname != SO_ZEROCOPY) {
+			release_sock(sk);
+			return sock_setsockopt(sock, level, optname, optval, optlen);
+		}
+
+		/* Use 'int' type here, because variable to
+		 * set this option usually has this type.
+		 */
+		COPY_IN(zerocopy);
+
+		if (zerocopy < 0 || zerocopy > 1) {
+			err = -EINVAL;
+			goto exit;
+		}
+
+		if (transport && !vsock_msgzerocopy_allow(transport)) {
+			err = -EOPNOTSUPP;
+			goto exit;
+		}
+
+		sock_valbool_flag(sk, SOCK_ZEROCOPY,
+				  zerocopy);
+		goto exit;
+	}
+
 	switch (optname) {
 	case SO_VM_SOCKETS_BUFFER_SIZE:
 		COPY_IN(val);
@@ -2322,6 +2358,12 @@  static int vsock_create(struct net *net, struct socket *sock,
 		}
 	}
 
+	/* SOCK_DGRAM doesn't have 'setsockopt' callback set in its
+	 * proto_ops, so there is no handler for custom logic.
+	 */
+	if (sock_type_connectible(sock->type))
+		set_bit(SOCK_CUSTOM_SOCKOPT, &sk->sk_socket->flags);
+
 	vsock_insert_unbound(vsk);
 
 	return 0;