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

[Stefano Garzarella]

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@xxxxxxxxxxxxxxxxx>
> > > ---
> > > 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@xxxxxxxxx>.
> > >
> > > 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