| Message ID | 20231206211849.2707151-4-avkrasnov@salutedevices.com (mailing list archive) |
|---|---|
| State | Superseded |
| Delegated to: | Netdev Maintainers |
| Headers | show |
| Series | send credit update during setting SO_RCVLOWAT | expand |
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@salutedevices.com> >> --- >> 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 > > >> + 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 > > >> + >> + 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 >
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@salutedevices.com> > --- > 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. > + 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. > + > + 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
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@salutedevices.com> > >> --- > >> 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. > > > > > >> + 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 > >
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@salutedevices.com> >>>> --- >>>> 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. @Stefano, what did You mean about wrapping here? I think Michael is right, for example 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 >>> >
On 11.12.2023 15:01, Stefano Garzarella wrote: > 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@salutedevices.com> >>>>>> --- >>>>>> 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! Got it! I'll remove it, no problem Thanks, Arseniy > > 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 >>>>> >>> >> >
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@salutedevices.com> >>>>> --- >>>>> 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 >>>> >> >
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); + else + 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)); 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;
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@salutedevices.com> --- 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(-)