Message-ID: <1ea271c1-d492-d7f7-5016-7650a72b6139@sberdevices.ru>
Date:   Tue, 9 Aug 2022 09:37:31 +0000
From:   Arseniy Krasnov <AVKrasnov@...rdevices.ru>
To:     Stefano Garzarella <sgarzare@...hat.com>
CC:     "David S. Miller" <davem@...emloft.net>,
        "edumazet@...gle.com" <edumazet@...gle.com>,
        Jakub Kicinski <kuba@...nel.org>,
        Paolo Abeni <pabeni@...hat.com>,
        "kys@...rosoft.com" <kys@...rosoft.com>,
        "haiyangz@...rosoft.com" <haiyangz@...rosoft.com>,
        "sthemmin@...rosoft.com" <sthemmin@...rosoft.com>,
        "wei.liu@...nel.org" <wei.liu@...nel.org>,
        "Dexuan Cui" <decui@...rosoft.com>,
        Stefan Hajnoczi <stefanha@...hat.com>,
        Bryan Tan <bryantan@...are.com>,
        Vishnu Dasa <vdasa@...are.com>,
        "VMware PV-Drivers Reviewers" <pv-drivers@...are.com>,
        Krasnov Arseniy <oxffffaa@...il.com>,
        "virtualization@...ts.linux-foundation.org" 
        <virtualization@...ts.linux-foundation.org>,
        "netdev@...r.kernel.org" <netdev@...r.kernel.org>,
        "linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>,
        "linux-hyperv@...r.kernel.org" <linux-hyperv@...r.kernel.org>,
        "kvm@...r.kernel.org" <kvm@...r.kernel.org>,
        kernel <kernel@...rdevices.ru>
Subject: Re: [RFC PATCH v3 1/9] vsock: SO_RCVLOWAT transport set callback

On 08.08.2022 13:30, Stefano Garzarella wrote:
> On Mon, Aug 8, 2022 at 12:23 PM Stefano Garzarella <sgarzare@...hat.com> wrote:
>>
>> On Wed, Aug 03, 2022 at 01:51:05PM +0000, Arseniy Krasnov wrote:
>>> This adds transport specific callback for SO_RCVLOWAT, because in some
>>> transports it may be difficult to know current available number of bytes
>>> ready to read. Thus, when SO_RCVLOWAT is set, transport may reject it.
>>>
>>> Signed-off-by: Arseniy Krasnov <AVKrasnov@...rdevices.ru>
>>> ---
>>> include/net/af_vsock.h   |  1 +
>>> net/vmw_vsock/af_vsock.c | 25 +++++++++++++++++++++++++
>>> 2 files changed, 26 insertions(+)
>>>
>>> diff --git a/include/net/af_vsock.h b/include/net/af_vsock.h
>>> index f742e50207fb..eae5874bae35 100644
>>> --- a/include/net/af_vsock.h
>>> +++ b/include/net/af_vsock.h
>>> @@ -134,6 +134,7 @@ struct vsock_transport {
>>>       u64 (*stream_rcvhiwat)(struct vsock_sock *);
>>>       bool (*stream_is_active)(struct vsock_sock *);
>>>       bool (*stream_allow)(u32 cid, u32 port);
>>> +      int (*set_rcvlowat)(struct vsock_sock *, int);
>>
>> checkpatch suggests to add identifier names. For some we put them in,
>> for others we didn't, but I suggest putting them in for the new ones
>> because I think it's clearer too.
>>
>> WARNING: function definition argument 'struct vsock_sock *' should also
>> have an identifier name
>> #25: FILE: include/net/af_vsock.h:137:
>> +       int (*set_rcvlowat)(struct vsock_sock *, int);
>>
>> WARNING: function definition argument 'int' should also have an identifier name
>> #25: FILE: include/net/af_vsock.h:137:
>> +       int (*set_rcvlowat)(struct vsock_sock *, int);
>>
>> total: 0 errors, 2 warnings, 0 checks, 44 lines checked
>>
>>>
>>>       /* SEQ_PACKET. */
>>>       ssize_t (*seqpacket_dequeue)(struct vsock_sock *vsk, struct msghdr *msg,
>>> diff --git a/net/vmw_vsock/af_vsock.c b/net/vmw_vsock/af_vsock.c
>>> index f04abf662ec6..016ad5ff78b7 100644
>>> --- a/net/vmw_vsock/af_vsock.c
>>> +++ b/net/vmw_vsock/af_vsock.c
>>> @@ -2129,6 +2129,30 @@ vsock_connectible_recvmsg(struct socket *sock, struct msghdr *msg, size_t len,
>>>       return err;
>>> }
>>>
>>> +static int vsock_set_rcvlowat(struct sock *sk, int val)
>>> +{
>>> +      const struct vsock_transport *transport;
>>> +      struct vsock_sock *vsk;
>>> +      int err = 0;
>>> +
>>> +      vsk = vsock_sk(sk);
>>> +
>>> +      if (val > vsk->buffer_size)
>>> +              return -EINVAL;
>>> +
>>> +      transport = vsk->transport;
>>> +
>>> +      if (!transport)
>>> +              return -EOPNOTSUPP;
>>
>> I don't know whether it is better in this case to write it in
>> sk->sk_rcvlowat, maybe we can return EOPNOTSUPP only when the trasport
>> is assigned and set_rcvlowat is not defined. This is because usually the
>> options are set just after creation, when the transport is practically
>> unassigned.
>>
>> I mean something like this:
>>
>>          if (transport) {
>>                  if (transport->set_rcvlowat)
>>                          return transport->set_rcvlowat(vsk, val);
>>                  else
>>                          return -EOPNOTSUPP;
>>          }
>>
>>          WRITE_ONCE(sk->sk_rcvlowat, val ? : 1);
>>
>>          return 0;
> 
> Since hv_sock implements `set_rcvlowat` to return EOPNOTSUPP. maybe we 
> can just do the following:
> 
>         if (transport && transport->set_rcvlowat)
>                 return transport->set_rcvlowat(vsk, val);
> 
>         WRITE_ONCE(sk->sk_rcvlowat, val ? : 1);
>         return 0;
> 
> That is, the default behavior is to set sk->sk_rcvlowat, but for 
> transports that want a different behavior, they need to define 
> set_rcvlowat() (like hv_sock).
Hm ok, i see. I've implemented logic when non-empty transport is required, because hyperv transport
forbids to set SO_RCVLOWAT, so user needs to call this setsockopt AFTER transport is assigned(to check
that transport allows it. Not after socket creation as You mentioned above). Otherwise there is no sense
in such callback - it will be never used. Also in code above - for hyperv we will have different behavior
depends on when set_rcvlowat is called: before or after transport assignment. Is it ok?
> 
> Thanks,
> Stefano
> 

Powered by blists - more mailing lists