Thread (18 messages) 18 messages, 3 authors, 2025-06-27

RE: [EXTERNAL] Re: [PATCH net-next v3 1/3] vsock: Add support for SIOCINQ ioctl

From: Dexuan Cui <decui@microsoft.com>
Date: 2025-06-25 08:03:03
Also in: kvm, linux-hyperv, lkml, netdev

From: Stefano Garzarella <sgarzare@redhat.com>
Sent: Tuesday, June 17, 2025 7:39 AM
 ...
Now looks better to me, I just checked transports: vmci and virtio/vhost
returns what we want, but for hyperv we have:

	static s64 hvs_stream_has_data(struct vsock_sock *vsk)
	{
		struct hvsock *hvs = vsk->trans;
		s64 ret;

		if (hvs->recv_data_len > 0)
			return 1;

@Hyper-v maintainers: do you know why we don't return `recv_data_len`?
Sorry for the late response!  This is the complete code of the function:

static s64 hvs_stream_has_data(struct vsock_sock *vsk)
{
        struct hvsock *hvs = vsk->trans;
        s64 ret;

        if (hvs->recv_data_len > 0)
                return 1;

        switch (hvs_channel_readable_payload(hvs->chan)) {
        case 1:
                ret = 1;
                break;
        case 0:
                vsk->peer_shutdown |= SEND_SHUTDOWN;
                ret = 0;
                break;
        default: /* -1 */
                ret = 0;
                break;
        }

        return ret;
}

If (hvs->recv_data_len > 0), I think we can return hvs->recv_data_len here.

If hvs->recv_data_len is 0, and hvs_channel_readable_payload(hvs->chan)
returns 1, we should not return hvs->recv_data_len (which is 0 here), and it's
not very easy to find how many bytes of payload in total is available right now:
each host-to-guest "packet" in the VMBus channel ringbuffer has a header
(which is not part of the payload data) and a trailing padding field, and we
would have to iterate on all the "packets" (or at least the next
"packet"?) to find the exact bytes of pending payload. Please see
hvs_stream_dequeue() for details.

Ideally hvs_stream_has_data() should return the exact length of pending
readable payload, but when the hv_sock code was written in 2017, 
vsock_stream_has_data() -> ... -> hvs_stream_has_data() basically only needs
to know whether there is any data or not, i.e. it's kind of a boolean variable, so
hvs_stream_has_data() was written to return 1 or 0 for simplicity. :-)

I can post the patch below (not tested yet) to fix hvs_stream_has_data() by
returning the payload length of the next single "packet".  Does it look good
to you?
--- a/net/vmw_vsock/hyperv_transport.c
+++ b/net/vmw_vsock/hyperv_transport.c
@@ -694,15 +694,25 @@ static ssize_t hvs_stream_enqueue(struct vsock_sock *vsk, struct msghdr *msg,
 static s64 hvs_stream_has_data(struct vsock_sock *vsk)
 {
        struct hvsock *hvs = vsk->trans;
+       bool need_refill = !hvs->recv_desc;
        s64 ret;

        if (hvs->recv_data_len > 0)
-               return 1;
+               return hvs->recv_data_len;

        switch (hvs_channel_readable_payload(hvs->chan)) {
        case 1:
-               ret = 1;
-               break;
+               if (!need_refill)
+                       return -EIO;
+
+               hvs->recv_desc = hv_pkt_iter_first(hvs->chan);
+               if (!hvs->recv_desc)
+                       return -ENOBUFS;
+
+               ret = hvs_update_recv_data(hvs);
+               if (ret)
+                       return ret;
+               return hvs->recv_data_len;
        case 0:
                vsk->peer_shutdown |= SEND_SHUTDOWN;
                ret = 0;
Thanks,
Dexuan
Keyboard shortcuts
hback out one level
jnext message in thread
kprevious message in thread
ldrill in
Escclose help / fold thread tree
?toggle this help