On Thu, Dec 07, 2023 at 01:50:05AM +0300, Arseniy Krasnov wrote:
On 07.12.2023 01:08, Michael S. Tsirkin wrote:
On Thu, Dec 07, 2023 at 12:52:51AM +0300, Arseniy Krasnov wrote:
On 07.12.2023 00:53, Michael S. Tsirkin wrote:
On Thu, Dec 07, 2023 at 12:18:48AM +0300, Arseniy Krasnov wrote:
Add one more condition for sending credit update during dequeue from
stream socket: when number of bytes in the rx queue is smaller than
SO_RCVLOWAT value of the socket. This is actual for non-default value
of SO_RCVLOWAT (e.g. not 1) - idea is to "kick" peer to continue data
transmission, because we need at least SO_RCVLOWAT bytes in our rx
queue to wake up user for reading data (in corner case it is also
possible to stuck both tx and rx sides, this is why 'Fixes' is used).
Also handle case when 'fwd_cnt' wraps, while 'last_fwd_cnt' is still
not.
Fixes: b89d882dc9fc ("vsock/virtio: reduce credit update messages")
Signed-off-by: Arseniy Krasnov <avkrasnov@xxxxxxxxxxxxxxxxx>
---
Changelog:
v6 -> v7:
* Handle wrap of 'fwd_cnt'.
* Do to send credit update when 'fwd_cnt' == 'last_fwd_cnt'.
net/vmw_vsock/virtio_transport_common.c | 18 +++++++++++++++---
1 file changed, 15 insertions(+), 3 deletions(-)
diff --git a/net/vmw_vsock/virtio_transport_common.c b/net/vmw_vsock/virtio_transport_common.c
index e137d740804e..39f8660d825d 100644
--- a/net/vmw_vsock/virtio_transport_common.c
+++ b/net/vmw_vsock/virtio_transport_common.c
@@ -558,6 +558,8 @@ virtio_transport_stream_do_dequeue(struct vsock_sock *vsk,
struct virtio_vsock_sock *vvs = vsk->trans;
size_t bytes, total = 0;
struct sk_buff *skb;
+ u32 fwd_cnt_delta;
+ bool low_rx_bytes;
int err = -EFAULT;
u32 free_space;
@@ -601,7 +603,15 @@ virtio_transport_stream_do_dequeue(struct vsock_sock *vsk,
}
}
- free_space = vvs->buf_alloc - (vvs->fwd_cnt - vvs->last_fwd_cnt);
+ /* Handle wrap of 'fwd_cnt'. */
+ if (vvs->fwd_cnt < vvs->last_fwd_cnt)
+ fwd_cnt_delta = vvs->fwd_cnt + (U32_MAX - vvs->last_fwd_cnt);
Are you sure there's no off by one here? for example if fwd_cnt is 0
and last_fwd_cnt is 0xfffffffff then apparently delta is 0.
Seems yes, I need +1 here
And then you will get a nop, because assigning U32_MAX + 1 to u32
gives you 0. Adding () does nothing to change the result,
+ and - are commutative.
Ahh, unsigned here, yes.
Ooops, sorry I was confused here!
@Stefano, what did You mean about wrapping here?
I think Michael is right, for example
Yep, I agree!
Sorry for this wrong suggestion!
Stefano
vvs->fwd_cnt wraps and now == 5
vvs->last_fwd_cnt == 0xffffffff
now delta before this patch will be 6 - correct value
May be I didn't get your idea, so implement it very naive?
Thanks, Arseniy
+ else
+ fwd_cnt_delta = vvs->fwd_cnt - vvs->last_fwd_cnt;
I actually don't see what is wrong with just
fwd_cnt_delta = vvs->fwd_cnt - vvs->last_fwd_cnt
32 bit unsigned math will I think handle wrap around correctly.
And given buf_alloc is also u32 - I don't see where the bug is in
the original code.
I think problem is when fwd_cnt wraps, while last_fwd_cnt is not. In this
case fwd_cnt_delta will be too big, so we won't send credit update which
leads to stall for sender
Thanks, Arseniy
Care coming up with an example?
+
+ free_space = vvs->buf_alloc - fwd_cnt_delta;
+ low_rx_bytes = (vvs->rx_bytes <
+ sock_rcvlowat(sk_vsock(vsk), 0, INT_MAX));
spin_unlock_bh(&vvs->rx_lock);
@@ -611,9 +621,11 @@ virtio_transport_stream_do_dequeue(struct vsock_sock *vsk,
* too high causes extra messages. Too low causes transmitter
* stalls. As stalls are in theory more expensive than extra
* messages, we set the limit to a high value. TODO: experiment
- * with different values.
+ * with different values. Also send credit update message when
+ * number of bytes in rx queue is not enough to wake up reader.
*/
- if (free_space < VIRTIO_VSOCK_MAX_PKT_BUF_SIZE)
+ if (fwd_cnt_delta &&
+ (free_space < VIRTIO_VSOCK_MAX_PKT_BUF_SIZE || low_rx_bytes))
virtio_transport_send_credit_update(vsk);
return total;
--
2.25.1
