On 4/11/25 15:43, Stefano Garzarella wrote:
> On Mon, Apr 07, 2025 at 08:41:43PM +0200, Michal Luczaj wrote:
>> Change the behaviour of a lingering close(): instead of waiting for all
>> data to be consumed, block until data is considered sent, i.e. until worker
>> picks the packets and decrements virtio_vsock_sock::bytes_unsent down to 0.
>>
>> Do linger on shutdown() just as well.
>
> I think this should go in a separate patch.
As discussed, dropping linger on shutdown() for now.
>> diff --git a/net/vmw_vsock/af_vsock.c b/net/vmw_vsock/af_vsock.c
>> index
>> fc6afbc8d6806a4d98c66abc3af4bd139c583b08..383c6644d047589035c0439c47d1440273e67ea9
>> 100644
>> --- a/net/vmw_vsock/af_vsock.c
>> +++ b/net/vmw_vsock/af_vsock.c
>> @@ -1013,6 +1013,29 @@ static int vsock_getname(struct socket *sock,
>> return err;
>> }
>>
>> +void vsock_linger(struct sock *sk, long timeout)
>> +{
>> + if (timeout) {
>
> I would prefer to avoid a whole nested block and return immediately
> in such a case. (It's pre-existing, but since we are moving this code
> I'd fix it).
>
> if (!timeout)
> return;
In v2 no code is moved (since no shutdown() lingering), so I'll reduce the
indentation in a separate patch. Let me know if it's not worth the churn
anymore.
>> + DEFINE_WAIT_FUNC(wait, woken_wake_function);
>> + ssize_t (*unsent)(struct vsock_sock *vsk);
>> + struct vsock_sock *vsk = vsock_sk(sk);
>> +
>
> transport->unsent_bytes can be NULL, this will panic with hyperv or
> vmci transport, especially because we now call this function in
> vsock_shutdown().
>
> I'd skip that call if transports don't implement it, but please add
> a comment on top of this function about that.
I'm not sure I understand. I am calling it only conditionally, see below.
Nevertheless, I'll add a comment.
>> + unsent = vsk->transport->unsent_bytes;
>> + if (!unsent)
>> + return;
>> +
>> + add_wait_queue(sk_sleep(sk), &wait);
>> +
>> + do {
>> + if (sk_wait_event(sk, &timeout, unsent(vsk) == 0,
>> &wait))
>> + break;
>> + } while (!signal_pending(current) && timeout);
>> +
>> + remove_wait_queue(sk_sleep(sk), &wait);
>> + }
>> +}
...
>> - if (sock_flag(sk, SOCK_DONE)) {
>> + if (sock_flag(sk, SOCK_DONE))
>> return true;
>> - }
>
> Please avoid this unrelated changes, if you really want to fix them,
> add another patch in the series where to fix them.
>
>>
>> sock_hold(sk);
>> - INIT_DELAYED_WORK(&vsk->close_work,
>> - virtio_transport_close_timeout);
>> + INIT_DELAYED_WORK(&vsk->close_work, virtio_transport_close_timeout);
>
> Ditto.
>
> These 2 could go together in a single `cleanup` patch, although I
> usually avoid it so that `git blame` makes sense. But if we want to
> make checkpatch happy, that's fine.
All right, dropping these. I've thought, since I was touching this function
and this wasn't a bug fix (and wouldn't be backported), it'd be okay to do
some trivial cleanups along the way.
Here's v2:
https://lore.kernel.org/netdev/20250421-vsock-linger-v2-0-fe9febd64...@rbox.co/
Thanks,
Michal
