Re: [PATCH net v3 1/2] vsock/bpf: release sock lock while waiting for data in recvmsg

[jrmmhm . kernel]

On Fri, Oct 30, 2026 at 03:18:10PM +0100, Stefano Garzarella wrote:
> Ditto about commit message, please put more effort on writing something
> readable by a human.

Agreed, I'll rewrite both messages myself for v4, much shorter.

> >+            if (sk->sk_err) {
> >+                    ret = -sk->sk_err;
>
> Why?

Same as vsock_connectible_wait_data(): an error that arrives during the
wait is returned to recv(). Without the check the loop still ends, since
every path that sets sk_err also closes the socket or sets peer_shutdown,
but recv() then returns 0 or -ENOTCONN instead of the error.

> >+            if (sk->sk_state != TCP_ESTABLISHED) {
> >+                    ret = sock_flag(sk, SOCK_DONE) ? 0 : -ENOTCONN;
> >+                    break;
> >+            }
>
> I'm thinking if we should split this in multiple patches.

>From reading the code, it can be split without breaking bisect: first
loop on all exit conditions with the lock still held, then drop the lock
around wait_woken(). The order is fixed, since dropping the lock alone
brings back the early -EAGAIN, and the loop alone does not fix the
deadlock, so both would carry the Fixes tag. That is why I kept it as one
patch, but I'm fine either way. Split it like that for v4, with the
current 2/2 on top, or keep it as one?

> >+            if (!timeo) {
> >+                    ret = -EAGAIN;
> >+                    break;
> >+            }
>
> Is the move of this check changing some user visible error?
> I mean before this patch it was one of the first checks, is this okay?

Yes, for non-blocking reads, intentionally: -sk_err, 0 on peer shutdown
and the not-connected result now come before -EAGAIN, as on the native
recvmsg path. I'll say so in the commit message.

> >+            if (signal_pending(current)) {
> >+                    ret = sock_intr_errno(timeo);
> >+                    break;
> >+            }
>
> Why? Should be a separate patch?

No, the loop needs it: with a pending signal wait_woken() returns at
once, so without the check the loop spins until SO_RCVTIMEO expires, or
forever if none is set. I checked it by removing the check: a signalled
reader without SO_RCVTIMEO never returned and used a full CPU. The
native path does the same.

> >+            if (data < 0) {
>
> Why not data <= 0  and void the other if?

Right, will do in v4.

Thanks,
Jerome