Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::3:2 as permitted sender) client-ip=2620:137:e000::3:2;
Message-ID: <d27b863d-8576-2c9b-c6a6-c8e55d7dad68@salutedevices.com>
Date:   Tue, 26 Sep 2023 22:38:06 +0300
MIME-Version: 1.0
User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:102.0) Gecko/20100101
 Thunderbird/102.7.1
Subject: Re: [PATCH net-next v1 08/12] vsock: enable setting SO_ZEROCOPY
Content-Language: en-US
To:     Stefano Garzarella <sgarzare@redhat.com>
CC:     Stefan Hajnoczi <stefanha@redhat.com>,
        "David S. Miller" <davem@davemloft.net>,
        Eric Dumazet <edumazet@google.com>,
        Jakub Kicinski <kuba@kernel.org>,
        Paolo Abeni <pabeni@redhat.com>,
        "Michael S. Tsirkin" <mst@redhat.com>,
        Jason Wang <jasowang@redhat.com>,
        Bobby Eshleman <bobby.eshleman@bytedance.com>,
        <kvm@vger.kernel.org>, <virtualization@lists.linux-foundation.org>,
        <netdev@vger.kernel.org>, <linux-kernel@vger.kernel.org>,
        <kernel@sberdevices.ru>, <oxffffaa@gmail.com>
References: <20230922052428.4005676-1-avkrasnov@salutedevices.com>
 <20230922052428.4005676-9-avkrasnov@salutedevices.com>
 <ynuctxau4ta4pk763ut7gfdaqzcuyve7uf2a2iltyspravs5uf@xrtqtbhuuvwq>
From:   Arseniy Krasnov <avkrasnov@salutedevices.com>
In-Reply-To: <ynuctxau4ta4pk763ut7gfdaqzcuyve7uf2a2iltyspravs5uf@xrtqtbhuuvwq>
Content-Type: text/plain; charset="UTF-8"
Content-Transfer-Encoding: 8bit
Precedence: bulk


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
>>
>