Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 23.128.96.33 as permitted sender) client-ip=23.128.96.33;
Date:   Wed, 27 Sep 2023 09:35:31 +0200
From:   Stefano Garzarella <sgarzare@redhat.com>
To:     Arseniy Krasnov <avkrasnov@salutedevices.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
Subject: Re: [PATCH net-next v1 08/12] vsock: enable setting SO_ZEROCOPY
Message-ID: <n4si4yyqs2svmvhueyxxyev2v3wxugzjjb25wpyveg3ns5nv6i@cfb4fyq5kdaf>
References: <20230922052428.4005676-1-avkrasnov@salutedevices.com>
 <20230922052428.4005676-9-avkrasnov@salutedevices.com>
 <ynuctxau4ta4pk763ut7gfdaqzcuyve7uf2a2iltyspravs5uf@xrtqtbhuuvwq>
 <d27b863d-8576-2c9b-c6a6-c8e55d7dad68@salutedevices.com>
MIME-Version: 1.0
Content-Type: text/plain; charset=iso-8859-1; format=flowed
Content-Disposition: inline
Content-Transfer-Encoding: 8bit
In-Reply-To: <d27b863d-8576-2c9b-c6a6-c8e55d7dad68@salutedevices.com>
Precedence: bulk

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