On 25.06.2024 16:46, Stefano Garzarella wrote:
> On Fri, Jun 21, 2024 at 10:25:40PM GMT, Arseniy Krasnov wrote:
>> Previous calculation of 'free_space' was wrong (but worked as expected
>> in most cases, see below), because it didn't account number of bytes in
>> rx queue. Let's rework 'free_space' calculation in the following way:
>> as this value is considered free space at rx side from tx point of view,
>> it must be equal to return value of 'virtio_transport_get_credit()' at
>> tx side. This function uses 'tx_cnt' counter and 'peer_fwd_cnt': first
>> is number of transmitted bytes (without wrap), second is last 'fwd_cnt'
>> value received from rx. So let's use same approach at rx side during
>> 'free_space' calculation: add 'rx_cnt' counter which is number of
>> received bytes (also without wrap) and subtract 'last_fwd_cnt' from it.
>> Now we have:
>> 1) 'rx_cnt' == 'tx_cnt' at both sides.
>> 2) 'last_fwd_cnt' == 'peer_fwd_cnt' - because first is last 'fwd_cnt'
>> sent to tx, while second is last 'fwd_cnt' received from rx.
>>
>> Now 'free_space' is handled correctly and also we don't need
>
> mmm, I don't know if it was wrong before, maybe we could say it was less accurate.
May be "now 'free_space' is handled in more precise way and also we ..." ?
>
> That said, could we have the same problem now if we have a lot of producers and the virtqueue becomes full?
>
I guess if virtqueue is full, we just wait by returning skb back to tx queue... e.g.
data exchange between two sockets just freezes. ?
>> 'low_rx_bytes' flag - this was more like a hack.
>>
>> Previous calculation of 'free_space' worked (in 99% cases), because if
>> we take a look on behaviour of both expressions (new and previous):
>>
>> '(rx_cnt - last_fwd_cnt)' and '(fwd_cnt - last_fwd_cnt)'
>>
>> Both of them always grows up, with almost same "speed": only difference
>> is that 'rx_cnt' is incremented earlier during packet is received,
>> while 'fwd_cnt' in incremented when packet is read by user. So if 'rx_cnt'
>> grows "faster", then resulting 'free_space' become smaller also, so we
>> send credit updates a little bit more, but:
>>
>> * 'free_space' calculation based on 'rx_cnt' gives the same value,
>> which tx sees as free space at rx side, so original idea of
>
> Ditto, what happen if the virtqueue is full?
>
>> 'free_space' is now implemented as planned.
>> * Hack with 'low_rx_bytes' now is not needed.
>
> Yeah, so this patch should also mitigate issue reported by Alex (added in CC), right?
>
> If yes, please mention that problem and add a Reported-by giving credit to Alex.
Yes, of course!
>
>>
>> Also here is some performance comparison between both versions of
>> 'free_space' calculation:
>>
>> *------*----------*----------*
>> | | 'rx_cnt' | previous |
>> *------*----------*----------*
>> |H -> G| 8.42 | 7.82 |
>> *------*----------*----------*
>> |G -> H| 11.6 | 12.1 |
>> *------*----------*----------*
>
> How many seconds did you run it? How many repetitions? There's a little discrepancy anyway, but I can't tell if it's just noise.
I run 4 times, each run for ~10 seconds... I think I can also add number of credit update messages to this report.
>
>>
>> As benchmark 'vsock-iperf' with default arguments was used. There is no
>> significant performance difference before and after this patch.
>>
>> Signed-off-by: Arseniy Krasnov <avkrasnov@xxxxxxxxxxxxxxxxx>
>> ---
>> include/linux/virtio_vsock.h | 1 +
>> net/vmw_vsock/virtio_transport_common.c | 8 +++-----
>> 2 files changed, 4 insertions(+), 5 deletions(-)
>
> Thanks for working on this, I'll do more tests but the approach LGTM.
Got it, Thanks
>
> Thanks,
> Stefano
>
>>
>> diff --git a/include/linux/virtio_vsock.h b/include/linux/virtio_vsock.h
>> index c82089dee0c8..3579491c411e 100644
>> --- a/include/linux/virtio_vsock.h
>> +++ b/include/linux/virtio_vsock.h
>> @@ -135,6 +135,7 @@ struct virtio_vsock_sock {
>> u32 peer_buf_alloc;
>>
>> /* Protected by rx_lock */
>> + u32 rx_cnt;
>> u32 fwd_cnt;
>> u32 last_fwd_cnt;
>> u32 rx_bytes;
>> diff --git a/net/vmw_vsock/virtio_transport_common.c b/net/vmw_vsock/virtio_transport_common.c
>> index 16ff976a86e3..1d4e2328e06e 100644
>> --- a/net/vmw_vsock/virtio_transport_common.c
>> +++ b/net/vmw_vsock/virtio_transport_common.c
>> @@ -441,6 +441,7 @@ static bool virtio_transport_inc_rx_pkt(struct virtio_vsock_sock *vvs,
>> return false;
>>
>> vvs->rx_bytes += len;
>> + vvs->rx_cnt += len;
>> return true;
>> }
>>
>> @@ -558,7 +559,6 @@ virtio_transport_stream_do_dequeue(struct vsock_sock *vsk,
>> size_t bytes, total = 0;
>> struct sk_buff *skb;
>> u32 fwd_cnt_delta;
>> - bool low_rx_bytes;
>> int err = -EFAULT;
>> u32 free_space;
>>
>> @@ -603,9 +603,7 @@ virtio_transport_stream_do_dequeue(struct vsock_sock *vsk,
>> }
>>
>> fwd_cnt_delta = vvs->fwd_cnt - vvs->last_fwd_cnt;
>> - free_space = vvs->buf_alloc - fwd_cnt_delta;
>> - low_rx_bytes = (vvs->rx_bytes <
>> - sock_rcvlowat(sk_vsock(vsk), 0, INT_MAX));
>> + free_space = vvs->buf_alloc - (vvs->rx_cnt - vvs->last_fwd_cnt);
>>
>> spin_unlock_bh(&vvs->rx_lock);
>>
>> @@ -619,7 +617,7 @@ virtio_transport_stream_do_dequeue(struct vsock_sock *vsk,
>> * number of bytes in rx queue is not enough to wake up reader.
>> */
>> if (fwd_cnt_delta &&
>> - (free_space < VIRTIO_VSOCK_MAX_PKT_BUF_SIZE || low_rx_bytes))
>> + (free_space < VIRTIO_VSOCK_MAX_PKT_BUF_SIZE))
>> virtio_transport_send_credit_update(vsk);
>>
>> return total;
>> --
>> 2.25.1
>>
>>
>
