| Message ID | 20231211211658.2904268-1-avkrasnov@salutedevices.com (mailing list archive) |
|---|---|
| Headers | show |
| Series | send credit update during setting SO_RCVLOWAT | expand |
On Tue, Dec 12, 2023 at 12:16:54AM +0300, Arseniy Krasnov wrote: > Hello, > > DESCRIPTION > > This patchset fixes old problem with hungup of both rx/tx sides and adds > test for it. This happens due to non-default SO_RCVLOWAT value and > deferred credit update in virtio/vsock. Link to previous old patchset: > https://lore.kernel.org/netdev/39b2e9fd-601b-189d-39a9-914e5574524c@sberdevices.ru/ Patchset: Acked-by: Michael S. Tsirkin <mst@redhat.com> But I worry whether we actually need 3/8 in net not in net-next. Thanks! > Here is what happens step by step: > > TEST > > INITIAL CONDITIONS > > 1) Vsock buffer size is 128KB. > 2) Maximum packet size is also 64KB as defined in header (yes it is > hardcoded, just to remind about that value). > 3) SO_RCVLOWAT is default, e.g. 1 byte. > > > STEPS > > SENDER RECEIVER > 1) sends 128KB + 1 byte in a > single buffer. 128KB will > be sent, but for 1 byte > sender will wait for free > space at peer. Sender goes > to sleep. > > > 2) reads 64KB, credit update not sent > 3) sets SO_RCVLOWAT to 64KB + 1 > 4) poll() -> wait forever, there is > only 64KB available to read. > > So in step 4) receiver also goes to sleep, waiting for enough data or > connection shutdown message from the sender. Idea to fix it is that rx > kicks tx side to continue transmission (and may be close connection) > when rx changes number of bytes to be woken up (e.g. SO_RCVLOWAT) and > this value is bigger than number of available bytes to read. > > I've added small test for this, but not sure as it uses hardcoded value > for maximum packet length, this value is defined in kernel header and > used to control deferred credit update. And as this is not available to > userspace, I can't control test parameters correctly (if one day this > define will be changed - test may become useless). > > Head for this patchset is: > https://git.kernel.org/pub/scm/linux/kernel/git/netdev/net-next.git/commit/?id=021b0c952f226236f2edf89c737efb9a28d1422d > > Link to v1: > https://lore.kernel.org/netdev/20231108072004.1045669-1-avkrasnov@salutedevices.com/ > Link to v2: > https://lore.kernel.org/netdev/20231119204922.2251912-1-avkrasnov@salutedevices.com/ > Link to v3: > https://lore.kernel.org/netdev/20231122180510.2297075-1-avkrasnov@salutedevices.com/ > Link to v4: > https://lore.kernel.org/netdev/20231129212519.2938875-1-avkrasnov@salutedevices.com/ > Link to v5: > https://lore.kernel.org/netdev/20231130130840.253733-1-avkrasnov@salutedevices.com/ > Link to v6: > https://lore.kernel.org/netdev/20231205064806.2851305-1-avkrasnov@salutedevices.com/ > Link to v7: > https://lore.kernel.org/netdev/20231206211849.2707151-1-avkrasnov@salutedevices.com/ > > Changelog: > v1 -> v2: > * Patchset rebased and tested on new HEAD of net-next (see hash above). > * New patch is added as 0001 - it removes return from SO_RCVLOWAT set > callback in 'af_vsock.c' when transport callback is set - with that > we can set 'sk_rcvlowat' only once in 'af_vsock.c' and in future do > not copy-paste it to every transport. It was discussed in v1. > * See per-patch changelog after ---. > v2 -> v3: > * See changelog after --- in 0003 only (0001 and 0002 still same). > v3 -> v4: > * Patchset rebased and tested on new HEAD of net-next (see hash above). > * See per-patch changelog after ---. > v4 -> v5: > * Change patchset tag 'RFC' -> 'net-next'. > * See per-patch changelog after ---. > v5 -> v6: > * New patch 0003 which sends credit update during reading bytes from > socket. > * See per-patch changelog after ---. > v6 -> v7: > * Patchset rebased and tested on new HEAD of net-next (see hash above). > * See per-patch changelog after ---. > v7 -> v8: > * See per-patch changelog after ---. > > Arseniy Krasnov (4): > vsock: update SO_RCVLOWAT setting callback > virtio/vsock: send credit update during setting SO_RCVLOWAT > virtio/vsock: fix logic which reduces credit update messages > vsock/test: two tests to check credit update logic > > drivers/vhost/vsock.c | 1 + > include/linux/virtio_vsock.h | 1 + > include/net/af_vsock.h | 2 +- > net/vmw_vsock/af_vsock.c | 9 +- > net/vmw_vsock/hyperv_transport.c | 4 +- > net/vmw_vsock/virtio_transport.c | 1 + > net/vmw_vsock/virtio_transport_common.c | 43 +++++- > net/vmw_vsock/vsock_loopback.c | 1 + > tools/testing/vsock/vsock_test.c | 175 ++++++++++++++++++++++++ > 9 files changed, 229 insertions(+), 8 deletions(-) > > -- > 2.25.1
On 12.12.2023 18:54, Michael S. Tsirkin wrote: > On Tue, Dec 12, 2023 at 12:16:54AM +0300, Arseniy Krasnov wrote: >> Hello, >> >> DESCRIPTION >> >> This patchset fixes old problem with hungup of both rx/tx sides and adds >> test for it. This happens due to non-default SO_RCVLOWAT value and >> deferred credit update in virtio/vsock. Link to previous old patchset: >> https://lore.kernel.org/netdev/39b2e9fd-601b-189d-39a9-914e5574524c@sberdevices.ru/ > > > Patchset: > > Acked-by: Michael S. Tsirkin <mst@redhat.com> Thanks! > > > But I worry whether we actually need 3/8 in net not in net-next. Because of "Fixes" tag ? I think this problem is not critical and reproducible only in special cases, but i'm not familiar with netdev process so good, so I don't have strong opinion. I guess @Stefano knows better. Thanks, Arseniy > > Thanks! > >> Here is what happens step by step: >> >> TEST >> >> INITIAL CONDITIONS >> >> 1) Vsock buffer size is 128KB. >> 2) Maximum packet size is also 64KB as defined in header (yes it is >> hardcoded, just to remind about that value). >> 3) SO_RCVLOWAT is default, e.g. 1 byte. >> >> >> STEPS >> >> SENDER RECEIVER >> 1) sends 128KB + 1 byte in a >> single buffer. 128KB will >> be sent, but for 1 byte >> sender will wait for free >> space at peer. Sender goes >> to sleep. >> >> >> 2) reads 64KB, credit update not sent >> 3) sets SO_RCVLOWAT to 64KB + 1 >> 4) poll() -> wait forever, there is >> only 64KB available to read. >> >> So in step 4) receiver also goes to sleep, waiting for enough data or >> connection shutdown message from the sender. Idea to fix it is that rx >> kicks tx side to continue transmission (and may be close connection) >> when rx changes number of bytes to be woken up (e.g. SO_RCVLOWAT) and >> this value is bigger than number of available bytes to read. >> >> I've added small test for this, but not sure as it uses hardcoded value >> for maximum packet length, this value is defined in kernel header and >> used to control deferred credit update. And as this is not available to >> userspace, I can't control test parameters correctly (if one day this >> define will be changed - test may become useless). >> >> Head for this patchset is: >> https://git.kernel.org/pub/scm/linux/kernel/git/netdev/net-next.git/commit/?id=021b0c952f226236f2edf89c737efb9a28d1422d >> >> Link to v1: >> https://lore.kernel.org/netdev/20231108072004.1045669-1-avkrasnov@salutedevices.com/ >> Link to v2: >> https://lore.kernel.org/netdev/20231119204922.2251912-1-avkrasnov@salutedevices.com/ >> Link to v3: >> https://lore.kernel.org/netdev/20231122180510.2297075-1-avkrasnov@salutedevices.com/ >> Link to v4: >> https://lore.kernel.org/netdev/20231129212519.2938875-1-avkrasnov@salutedevices.com/ >> Link to v5: >> https://lore.kernel.org/netdev/20231130130840.253733-1-avkrasnov@salutedevices.com/ >> Link to v6: >> https://lore.kernel.org/netdev/20231205064806.2851305-1-avkrasnov@salutedevices.com/ >> Link to v7: >> https://lore.kernel.org/netdev/20231206211849.2707151-1-avkrasnov@salutedevices.com/ >> >> Changelog: >> v1 -> v2: >> * Patchset rebased and tested on new HEAD of net-next (see hash above). >> * New patch is added as 0001 - it removes return from SO_RCVLOWAT set >> callback in 'af_vsock.c' when transport callback is set - with that >> we can set 'sk_rcvlowat' only once in 'af_vsock.c' and in future do >> not copy-paste it to every transport. It was discussed in v1. >> * See per-patch changelog after ---. >> v2 -> v3: >> * See changelog after --- in 0003 only (0001 and 0002 still same). >> v3 -> v4: >> * Patchset rebased and tested on new HEAD of net-next (see hash above). >> * See per-patch changelog after ---. >> v4 -> v5: >> * Change patchset tag 'RFC' -> 'net-next'. >> * See per-patch changelog after ---. >> v5 -> v6: >> * New patch 0003 which sends credit update during reading bytes from >> socket. >> * See per-patch changelog after ---. >> v6 -> v7: >> * Patchset rebased and tested on new HEAD of net-next (see hash above). >> * See per-patch changelog after ---. >> v7 -> v8: >> * See per-patch changelog after ---. >> >> Arseniy Krasnov (4): >> vsock: update SO_RCVLOWAT setting callback >> virtio/vsock: send credit update during setting SO_RCVLOWAT >> virtio/vsock: fix logic which reduces credit update messages >> vsock/test: two tests to check credit update logic >> >> drivers/vhost/vsock.c | 1 + >> include/linux/virtio_vsock.h | 1 + >> include/net/af_vsock.h | 2 +- >> net/vmw_vsock/af_vsock.c | 9 +- >> net/vmw_vsock/hyperv_transport.c | 4 +- >> net/vmw_vsock/virtio_transport.c | 1 + >> net/vmw_vsock/virtio_transport_common.c | 43 +++++- >> net/vmw_vsock/vsock_loopback.c | 1 + >> tools/testing/vsock/vsock_test.c | 175 ++++++++++++++++++++++++ >> 9 files changed, 229 insertions(+), 8 deletions(-) >> >> -- >> 2.25.1 >
On Tue, Dec 12, 2023 at 06:59:03PM +0300, Arseniy Krasnov wrote: > > > On 12.12.2023 18:54, Michael S. Tsirkin wrote: > > On Tue, Dec 12, 2023 at 12:16:54AM +0300, Arseniy Krasnov wrote: > >> Hello, > >> > >> DESCRIPTION > >> > >> This patchset fixes old problem with hungup of both rx/tx sides and adds > >> test for it. This happens due to non-default SO_RCVLOWAT value and > >> deferred credit update in virtio/vsock. Link to previous old patchset: > >> https://lore.kernel.org/netdev/39b2e9fd-601b-189d-39a9-914e5574524c@sberdevices.ru/ > > > > > > Patchset: > > > > Acked-by: Michael S. Tsirkin <mst@redhat.com> > > Thanks! > > > > > > > But I worry whether we actually need 3/8 in net not in net-next. > > Because of "Fixes" tag ? I think this problem is not critical and reproducible > only in special cases, but i'm not familiar with netdev process so good, so I don't > have strong opinion. I guess @Stefano knows better. > > Thanks, Arseniy Fixes means "if you have that other commit then you need this commit too". I think as a minimum you need to rearrange patches to make the fix go in first. We don't want a regression followed by a fix. > > > > Thanks! > > > >> Here is what happens step by step: > >> > >> TEST > >> > >> INITIAL CONDITIONS > >> > >> 1) Vsock buffer size is 128KB. > >> 2) Maximum packet size is also 64KB as defined in header (yes it is > >> hardcoded, just to remind about that value). > >> 3) SO_RCVLOWAT is default, e.g. 1 byte. > >> > >> > >> STEPS > >> > >> SENDER RECEIVER > >> 1) sends 128KB + 1 byte in a > >> single buffer. 128KB will > >> be sent, but for 1 byte > >> sender will wait for free > >> space at peer. Sender goes > >> to sleep. > >> > >> > >> 2) reads 64KB, credit update not sent > >> 3) sets SO_RCVLOWAT to 64KB + 1 > >> 4) poll() -> wait forever, there is > >> only 64KB available to read. > >> > >> So in step 4) receiver also goes to sleep, waiting for enough data or > >> connection shutdown message from the sender. Idea to fix it is that rx > >> kicks tx side to continue transmission (and may be close connection) > >> when rx changes number of bytes to be woken up (e.g. SO_RCVLOWAT) and > >> this value is bigger than number of available bytes to read. > >> > >> I've added small test for this, but not sure as it uses hardcoded value > >> for maximum packet length, this value is defined in kernel header and > >> used to control deferred credit update. And as this is not available to > >> userspace, I can't control test parameters correctly (if one day this > >> define will be changed - test may become useless). > >> > >> Head for this patchset is: > >> https://git.kernel.org/pub/scm/linux/kernel/git/netdev/net-next.git/commit/?id=021b0c952f226236f2edf89c737efb9a28d1422d > >> > >> Link to v1: > >> https://lore.kernel.org/netdev/20231108072004.1045669-1-avkrasnov@salutedevices.com/ > >> Link to v2: > >> https://lore.kernel.org/netdev/20231119204922.2251912-1-avkrasnov@salutedevices.com/ > >> Link to v3: > >> https://lore.kernel.org/netdev/20231122180510.2297075-1-avkrasnov@salutedevices.com/ > >> Link to v4: > >> https://lore.kernel.org/netdev/20231129212519.2938875-1-avkrasnov@salutedevices.com/ > >> Link to v5: > >> https://lore.kernel.org/netdev/20231130130840.253733-1-avkrasnov@salutedevices.com/ > >> Link to v6: > >> https://lore.kernel.org/netdev/20231205064806.2851305-1-avkrasnov@salutedevices.com/ > >> Link to v7: > >> https://lore.kernel.org/netdev/20231206211849.2707151-1-avkrasnov@salutedevices.com/ > >> > >> Changelog: > >> v1 -> v2: > >> * Patchset rebased and tested on new HEAD of net-next (see hash above). > >> * New patch is added as 0001 - it removes return from SO_RCVLOWAT set > >> callback in 'af_vsock.c' when transport callback is set - with that > >> we can set 'sk_rcvlowat' only once in 'af_vsock.c' and in future do > >> not copy-paste it to every transport. It was discussed in v1. > >> * See per-patch changelog after ---. > >> v2 -> v3: > >> * See changelog after --- in 0003 only (0001 and 0002 still same). > >> v3 -> v4: > >> * Patchset rebased and tested on new HEAD of net-next (see hash above). > >> * See per-patch changelog after ---. > >> v4 -> v5: > >> * Change patchset tag 'RFC' -> 'net-next'. > >> * See per-patch changelog after ---. > >> v5 -> v6: > >> * New patch 0003 which sends credit update during reading bytes from > >> socket. > >> * See per-patch changelog after ---. > >> v6 -> v7: > >> * Patchset rebased and tested on new HEAD of net-next (see hash above). > >> * See per-patch changelog after ---. > >> v7 -> v8: > >> * See per-patch changelog after ---. > >> > >> Arseniy Krasnov (4): > >> vsock: update SO_RCVLOWAT setting callback > >> virtio/vsock: send credit update during setting SO_RCVLOWAT > >> virtio/vsock: fix logic which reduces credit update messages > >> vsock/test: two tests to check credit update logic > >> > >> drivers/vhost/vsock.c | 1 + > >> include/linux/virtio_vsock.h | 1 + > >> include/net/af_vsock.h | 2 +- > >> net/vmw_vsock/af_vsock.c | 9 +- > >> net/vmw_vsock/hyperv_transport.c | 4 +- > >> net/vmw_vsock/virtio_transport.c | 1 + > >> net/vmw_vsock/virtio_transport_common.c | 43 +++++- > >> net/vmw_vsock/vsock_loopback.c | 1 + > >> tools/testing/vsock/vsock_test.c | 175 ++++++++++++++++++++++++ > >> 9 files changed, 229 insertions(+), 8 deletions(-) > >> > >> -- > >> 2.25.1 > >
On 12.12.2023 19:12, Michael S. Tsirkin wrote: > On Tue, Dec 12, 2023 at 06:59:03PM +0300, Arseniy Krasnov wrote: >> >> >> On 12.12.2023 18:54, Michael S. Tsirkin wrote: >>> On Tue, Dec 12, 2023 at 12:16:54AM +0300, Arseniy Krasnov wrote: >>>> Hello, >>>> >>>> DESCRIPTION >>>> >>>> This patchset fixes old problem with hungup of both rx/tx sides and adds >>>> test for it. This happens due to non-default SO_RCVLOWAT value and >>>> deferred credit update in virtio/vsock. Link to previous old patchset: >>>> https://lore.kernel.org/netdev/39b2e9fd-601b-189d-39a9-914e5574524c@sberdevices.ru/ >>> >>> >>> Patchset: >>> >>> Acked-by: Michael S. Tsirkin <mst@redhat.com> >> >> Thanks! >> >>> >>> >>> But I worry whether we actually need 3/8 in net not in net-next. >> >> Because of "Fixes" tag ? I think this problem is not critical and reproducible >> only in special cases, but i'm not familiar with netdev process so good, so I don't >> have strong opinion. I guess @Stefano knows better. >> >> Thanks, Arseniy > > Fixes means "if you have that other commit then you need this commit > too". I think as a minimum you need to rearrange patches to make the > fix go in first. We don't want a regression followed by a fix. I see, ok, @Stefano WDYT? I think rearrange doesn't break anything, because this patch fixes problem that is not related with the new patches from this patchset. Thanks, Arseniy > >>> >>> Thanks! >>> >>>> Here is what happens step by step: >>>> >>>> TEST >>>> >>>> INITIAL CONDITIONS >>>> >>>> 1) Vsock buffer size is 128KB. >>>> 2) Maximum packet size is also 64KB as defined in header (yes it is >>>> hardcoded, just to remind about that value). >>>> 3) SO_RCVLOWAT is default, e.g. 1 byte. >>>> >>>> >>>> STEPS >>>> >>>> SENDER RECEIVER >>>> 1) sends 128KB + 1 byte in a >>>> single buffer. 128KB will >>>> be sent, but for 1 byte >>>> sender will wait for free >>>> space at peer. Sender goes >>>> to sleep. >>>> >>>> >>>> 2) reads 64KB, credit update not sent >>>> 3) sets SO_RCVLOWAT to 64KB + 1 >>>> 4) poll() -> wait forever, there is >>>> only 64KB available to read. >>>> >>>> So in step 4) receiver also goes to sleep, waiting for enough data or >>>> connection shutdown message from the sender. Idea to fix it is that rx >>>> kicks tx side to continue transmission (and may be close connection) >>>> when rx changes number of bytes to be woken up (e.g. SO_RCVLOWAT) and >>>> this value is bigger than number of available bytes to read. >>>> >>>> I've added small test for this, but not sure as it uses hardcoded value >>>> for maximum packet length, this value is defined in kernel header and >>>> used to control deferred credit update. And as this is not available to >>>> userspace, I can't control test parameters correctly (if one day this >>>> define will be changed - test may become useless). >>>> >>>> Head for this patchset is: >>>> https://git.kernel.org/pub/scm/linux/kernel/git/netdev/net-next.git/commit/?id=021b0c952f226236f2edf89c737efb9a28d1422d >>>> >>>> Link to v1: >>>> https://lore.kernel.org/netdev/20231108072004.1045669-1-avkrasnov@salutedevices.com/ >>>> Link to v2: >>>> https://lore.kernel.org/netdev/20231119204922.2251912-1-avkrasnov@salutedevices.com/ >>>> Link to v3: >>>> https://lore.kernel.org/netdev/20231122180510.2297075-1-avkrasnov@salutedevices.com/ >>>> Link to v4: >>>> https://lore.kernel.org/netdev/20231129212519.2938875-1-avkrasnov@salutedevices.com/ >>>> Link to v5: >>>> https://lore.kernel.org/netdev/20231130130840.253733-1-avkrasnov@salutedevices.com/ >>>> Link to v6: >>>> https://lore.kernel.org/netdev/20231205064806.2851305-1-avkrasnov@salutedevices.com/ >>>> Link to v7: >>>> https://lore.kernel.org/netdev/20231206211849.2707151-1-avkrasnov@salutedevices.com/ >>>> >>>> Changelog: >>>> v1 -> v2: >>>> * Patchset rebased and tested on new HEAD of net-next (see hash above). >>>> * New patch is added as 0001 - it removes return from SO_RCVLOWAT set >>>> callback in 'af_vsock.c' when transport callback is set - with that >>>> we can set 'sk_rcvlowat' only once in 'af_vsock.c' and in future do >>>> not copy-paste it to every transport. It was discussed in v1. >>>> * See per-patch changelog after ---. >>>> v2 -> v3: >>>> * See changelog after --- in 0003 only (0001 and 0002 still same). >>>> v3 -> v4: >>>> * Patchset rebased and tested on new HEAD of net-next (see hash above). >>>> * See per-patch changelog after ---. >>>> v4 -> v5: >>>> * Change patchset tag 'RFC' -> 'net-next'. >>>> * See per-patch changelog after ---. >>>> v5 -> v6: >>>> * New patch 0003 which sends credit update during reading bytes from >>>> socket. >>>> * See per-patch changelog after ---. >>>> v6 -> v7: >>>> * Patchset rebased and tested on new HEAD of net-next (see hash above). >>>> * See per-patch changelog after ---. >>>> v7 -> v8: >>>> * See per-patch changelog after ---. >>>> >>>> Arseniy Krasnov (4): >>>> vsock: update SO_RCVLOWAT setting callback >>>> virtio/vsock: send credit update during setting SO_RCVLOWAT >>>> virtio/vsock: fix logic which reduces credit update messages >>>> vsock/test: two tests to check credit update logic >>>> >>>> drivers/vhost/vsock.c | 1 + >>>> include/linux/virtio_vsock.h | 1 + >>>> include/net/af_vsock.h | 2 +- >>>> net/vmw_vsock/af_vsock.c | 9 +- >>>> net/vmw_vsock/hyperv_transport.c | 4 +- >>>> net/vmw_vsock/virtio_transport.c | 1 + >>>> net/vmw_vsock/virtio_transport_common.c | 43 +++++- >>>> net/vmw_vsock/vsock_loopback.c | 1 + >>>> tools/testing/vsock/vsock_test.c | 175 ++++++++++++++++++++++++ >>>> 9 files changed, 229 insertions(+), 8 deletions(-) >>>> >>>> -- >>>> 2.25.1 >>> >
On Tue, Dec 12, 2023 at 08:43:07PM +0300, Arseniy Krasnov wrote: > > >On 12.12.2023 19:12, Michael S. Tsirkin wrote: >> On Tue, Dec 12, 2023 at 06:59:03PM +0300, Arseniy Krasnov wrote: >>> >>> >>> On 12.12.2023 18:54, Michael S. Tsirkin wrote: >>>> On Tue, Dec 12, 2023 at 12:16:54AM +0300, Arseniy Krasnov wrote: >>>>> Hello, >>>>> >>>>> DESCRIPTION >>>>> >>>>> This patchset fixes old problem with hungup of both rx/tx sides and adds >>>>> test for it. This happens due to non-default SO_RCVLOWAT value and >>>>> deferred credit update in virtio/vsock. Link to previous old patchset: >>>>> https://lore.kernel.org/netdev/39b2e9fd-601b-189d-39a9-914e5574524c@sberdevices.ru/ >>>> >>>> >>>> Patchset: >>>> >>>> Acked-by: Michael S. Tsirkin <mst@redhat.com> >>> >>> Thanks! >>> >>>> >>>> >>>> But I worry whether we actually need 3/8 in net not in net-next. >>> >>> Because of "Fixes" tag ? I think this problem is not critical and reproducible >>> only in special cases, but i'm not familiar with netdev process so good, so I don't >>> have strong opinion. I guess @Stefano knows better. >>> >>> Thanks, Arseniy >> >> Fixes means "if you have that other commit then you need this commit >> too". I think as a minimum you need to rearrange patches to make the >> fix go in first. We don't want a regression followed by a fix. > >I see, ok, @Stefano WDYT? I think rearrange doesn't break anything, >because this >patch fixes problem that is not related with the new patches from this patchset. I agree, patch 3 is for sure net material (I'm fine with both rearrangement or send it separately), but IMHO also patch 2 could be. I think with the same fixes tag, since before commit b89d882dc9fc ("vsock/virtio: reduce credit update messages") we sent a credit update for every bytes we read, so we should not have this problem, right? So, maybe all the series could be "net". Thanks, Stefano
On 13.12.2023 11:43, Stefano Garzarella wrote: > On Tue, Dec 12, 2023 at 08:43:07PM +0300, Arseniy Krasnov wrote: >> >> >> On 12.12.2023 19:12, Michael S. Tsirkin wrote: >>> On Tue, Dec 12, 2023 at 06:59:03PM +0300, Arseniy Krasnov wrote: >>>> >>>> >>>> On 12.12.2023 18:54, Michael S. Tsirkin wrote: >>>>> On Tue, Dec 12, 2023 at 12:16:54AM +0300, Arseniy Krasnov wrote: >>>>>> Hello, >>>>>> >>>>>> DESCRIPTION >>>>>> >>>>>> This patchset fixes old problem with hungup of both rx/tx sides and adds >>>>>> test for it. This happens due to non-default SO_RCVLOWAT value and >>>>>> deferred credit update in virtio/vsock. Link to previous old patchset: >>>>>> https://lore.kernel.org/netdev/39b2e9fd-601b-189d-39a9-914e5574524c@sberdevices.ru/ >>>>> >>>>> >>>>> Patchset: >>>>> >>>>> Acked-by: Michael S. Tsirkin <mst@redhat.com> >>>> >>>> Thanks! >>>> >>>>> >>>>> >>>>> But I worry whether we actually need 3/8 in net not in net-next. >>>> >>>> Because of "Fixes" tag ? I think this problem is not critical and reproducible >>>> only in special cases, but i'm not familiar with netdev process so good, so I don't >>>> have strong opinion. I guess @Stefano knows better. >>>> >>>> Thanks, Arseniy >>> >>> Fixes means "if you have that other commit then you need this commit >>> too". I think as a minimum you need to rearrange patches to make the >>> fix go in first. We don't want a regression followed by a fix. >> >> I see, ok, @Stefano WDYT? I think rearrange doesn't break anything, because this >> patch fixes problem that is not related with the new patches from this patchset. > > I agree, patch 3 is for sure net material (I'm fine with both rearrangement or send it separately), but IMHO also patch 2 could be. > I think with the same fixes tag, since before commit b89d882dc9fc ("vsock/virtio: reduce credit update messages") we sent a credit update > for every bytes we read, so we should not have this problem, right? Agree for 2, so I think I can rearrange: two fixes go first, then current 0001, and then tests. And send it as V9 for 'net' only ? Thanks, Arseniy > > So, maybe all the series could be "net". > > Thanks, > Stefano >
On Wed, Dec 13, 2023 at 12:08:27PM +0300, Arseniy Krasnov wrote: > > >On 13.12.2023 11:43, Stefano Garzarella wrote: >> On Tue, Dec 12, 2023 at 08:43:07PM +0300, Arseniy Krasnov wrote: >>> >>> >>> On 12.12.2023 19:12, Michael S. Tsirkin wrote: >>>> On Tue, Dec 12, 2023 at 06:59:03PM +0300, Arseniy Krasnov wrote: >>>>> >>>>> >>>>> On 12.12.2023 18:54, Michael S. Tsirkin wrote: >>>>>> On Tue, Dec 12, 2023 at 12:16:54AM +0300, Arseniy Krasnov wrote: >>>>>>> Hello, >>>>>>> >>>>>>> DESCRIPTION >>>>>>> >>>>>>> This patchset fixes old problem with hungup of both rx/tx sides and adds >>>>>>> test for it. This happens due to non-default SO_RCVLOWAT value and >>>>>>> deferred credit update in virtio/vsock. Link to previous old patchset: >>>>>>> https://lore.kernel.org/netdev/39b2e9fd-601b-189d-39a9-914e5574524c@sberdevices.ru/ >>>>>> >>>>>> >>>>>> Patchset: >>>>>> >>>>>> Acked-by: Michael S. Tsirkin <mst@redhat.com> >>>>> >>>>> Thanks! >>>>> >>>>>> >>>>>> >>>>>> But I worry whether we actually need 3/8 in net not in net-next. >>>>> >>>>> Because of "Fixes" tag ? I think this problem is not critical and reproducible >>>>> only in special cases, but i'm not familiar with netdev process so good, so I don't >>>>> have strong opinion. I guess @Stefano knows better. >>>>> >>>>> Thanks, Arseniy >>>> >>>> Fixes means "if you have that other commit then you need this commit >>>> too". I think as a minimum you need to rearrange patches to make the >>>> fix go in first. We don't want a regression followed by a fix. >>> >>> I see, ok, @Stefano WDYT? I think rearrange doesn't break anything, because this >>> patch fixes problem that is not related with the new patches from this patchset. >> >> I agree, patch 3 is for sure net material (I'm fine with both rearrangement or send it separately), but IMHO also patch 2 could be. >> I think with the same fixes tag, since before commit b89d882dc9fc ("vsock/virtio: reduce credit update messages") we sent a credit update >> for every bytes we read, so we should not have this problem, right? > >Agree for 2, so I think I can rearrange: two fixes go first, then current 0001, and then tests. And send it as V9 for 'net' only ? Maybe you can add this to patch 1 if we want it on net: Fixes: e38f22c860ed ("vsock: SO_RCVLOWAT transport set callback") Then I think that patch should go before patch 2, so we don't need to touch that code multiple times. so, IMHO the order should be the actual order or 3 - 1 - 2 - 4. Another option is to send just 2 & 3 to net, and the rest (1 & 4) to net-next. IMHO should be fine to send the entire series to net with the fixes tag also in patch 1. Net maintainers and Michael might have a different advice. Thanks, Stefano
On 13.12.2023 12:41, Stefano Garzarella wrote: > On Wed, Dec 13, 2023 at 12:08:27PM +0300, Arseniy Krasnov wrote: >> >> >> On 13.12.2023 11:43, Stefano Garzarella wrote: >>> On Tue, Dec 12, 2023 at 08:43:07PM +0300, Arseniy Krasnov wrote: >>>> >>>> >>>> On 12.12.2023 19:12, Michael S. Tsirkin wrote: >>>>> On Tue, Dec 12, 2023 at 06:59:03PM +0300, Arseniy Krasnov wrote: >>>>>> >>>>>> >>>>>> On 12.12.2023 18:54, Michael S. Tsirkin wrote: >>>>>>> On Tue, Dec 12, 2023 at 12:16:54AM +0300, Arseniy Krasnov wrote: >>>>>>>> Hello, >>>>>>>> >>>>>>>> DESCRIPTION >>>>>>>> >>>>>>>> This patchset fixes old problem with hungup of both rx/tx sides and adds >>>>>>>> test for it. This happens due to non-default SO_RCVLOWAT value and >>>>>>>> deferred credit update in virtio/vsock. Link to previous old patchset: >>>>>>>> https://lore.kernel.org/netdev/39b2e9fd-601b-189d-39a9-914e5574524c@sberdevices.ru/ >>>>>>> >>>>>>> >>>>>>> Patchset: >>>>>>> >>>>>>> Acked-by: Michael S. Tsirkin <mst@redhat.com> >>>>>> >>>>>> Thanks! >>>>>> >>>>>>> >>>>>>> >>>>>>> But I worry whether we actually need 3/8 in net not in net-next. >>>>>> >>>>>> Because of "Fixes" tag ? I think this problem is not critical and reproducible >>>>>> only in special cases, but i'm not familiar with netdev process so good, so I don't >>>>>> have strong opinion. I guess @Stefano knows better. >>>>>> >>>>>> Thanks, Arseniy >>>>> >>>>> Fixes means "if you have that other commit then you need this commit >>>>> too". I think as a minimum you need to rearrange patches to make the >>>>> fix go in first. We don't want a regression followed by a fix. >>>> >>>> I see, ok, @Stefano WDYT? I think rearrange doesn't break anything, because this >>>> patch fixes problem that is not related with the new patches from this patchset. >>> >>> I agree, patch 3 is for sure net material (I'm fine with both rearrangement or send it separately), but IMHO also patch 2 could be. >>> I think with the same fixes tag, since before commit b89d882dc9fc ("vsock/virtio: reduce credit update messages") we sent a credit update >>> for every bytes we read, so we should not have this problem, right? >> >> Agree for 2, so I think I can rearrange: two fixes go first, then current 0001, and then tests. And send it as V9 for 'net' only ? > > Maybe you can add this to patch 1 if we want it on net: > > Fixes: e38f22c860ed ("vsock: SO_RCVLOWAT transport set callback") > > Then I think that patch should go before patch 2, so we don't need to > touch that code multiple times. > > so, IMHO the order should be the actual order or 3 - 1 - 2 - 4. > > Another option is to send just 2 & 3 to net, and the rest (1 & 4) to net-next. IMHO should be fine to send the entire series to net with the fixes tag also in patch 1. Ok, agree that it is good to send whole patchset to net without splitting it. > > Net maintainers and Michael might have a different advice. Ok > > Thanks, > Stefano >
On Wed, Dec 13, 2023 at 12:08:27PM +0300, Arseniy Krasnov wrote: > > > On 13.12.2023 11:43, Stefano Garzarella wrote: > > On Tue, Dec 12, 2023 at 08:43:07PM +0300, Arseniy Krasnov wrote: > >> > >> > >> On 12.12.2023 19:12, Michael S. Tsirkin wrote: > >>> On Tue, Dec 12, 2023 at 06:59:03PM +0300, Arseniy Krasnov wrote: > >>>> > >>>> > >>>> On 12.12.2023 18:54, Michael S. Tsirkin wrote: > >>>>> On Tue, Dec 12, 2023 at 12:16:54AM +0300, Arseniy Krasnov wrote: > >>>>>> Hello, > >>>>>> > >>>>>> DESCRIPTION > >>>>>> > >>>>>> This patchset fixes old problem with hungup of both rx/tx sides and adds > >>>>>> test for it. This happens due to non-default SO_RCVLOWAT value and > >>>>>> deferred credit update in virtio/vsock. Link to previous old patchset: > >>>>>> https://lore.kernel.org/netdev/39b2e9fd-601b-189d-39a9-914e5574524c@sberdevices.ru/ > >>>>> > >>>>> > >>>>> Patchset: > >>>>> > >>>>> Acked-by: Michael S. Tsirkin <mst@redhat.com> > >>>> > >>>> Thanks! > >>>> > >>>>> > >>>>> > >>>>> But I worry whether we actually need 3/8 in net not in net-next. > >>>> > >>>> Because of "Fixes" tag ? I think this problem is not critical and reproducible > >>>> only in special cases, but i'm not familiar with netdev process so good, so I don't > >>>> have strong opinion. I guess @Stefano knows better. > >>>> > >>>> Thanks, Arseniy > >>> > >>> Fixes means "if you have that other commit then you need this commit > >>> too". I think as a minimum you need to rearrange patches to make the > >>> fix go in first. We don't want a regression followed by a fix. > >> > >> I see, ok, @Stefano WDYT? I think rearrange doesn't break anything, because this > >> patch fixes problem that is not related with the new patches from this patchset. > > > > I agree, patch 3 is for sure net material (I'm fine with both rearrangement or send it separately), but IMHO also patch 2 could be. > > I think with the same fixes tag, since before commit b89d882dc9fc ("vsock/virtio: reduce credit update messages") we sent a credit update > > for every bytes we read, so we should not have this problem, right? > > Agree for 2, so I think I can rearrange: two fixes go first, then current 0001, and then tests. And send it as V9 for 'net' only ? > > Thanks, Arseniy hmm why not net-next? > > > > So, maybe all the series could be "net". > > > > Thanks, > > Stefano > >
On Wed, Dec 13, 2023 at 10:05:44AM -0500, Michael S. Tsirkin wrote: > On Wed, Dec 13, 2023 at 12:08:27PM +0300, Arseniy Krasnov wrote: > > > > > > On 13.12.2023 11:43, Stefano Garzarella wrote: > > > On Tue, Dec 12, 2023 at 08:43:07PM +0300, Arseniy Krasnov wrote: > > >> > > >> > > >> On 12.12.2023 19:12, Michael S. Tsirkin wrote: > > >>> On Tue, Dec 12, 2023 at 06:59:03PM +0300, Arseniy Krasnov wrote: > > >>>> > > >>>> > > >>>> On 12.12.2023 18:54, Michael S. Tsirkin wrote: > > >>>>> On Tue, Dec 12, 2023 at 12:16:54AM +0300, Arseniy Krasnov wrote: > > >>>>>> Hello, > > >>>>>> > > >>>>>> DESCRIPTION > > >>>>>> > > >>>>>> This patchset fixes old problem with hungup of both rx/tx sides and adds > > >>>>>> test for it. This happens due to non-default SO_RCVLOWAT value and > > >>>>>> deferred credit update in virtio/vsock. Link to previous old patchset: > > >>>>>> https://lore.kernel.org/netdev/39b2e9fd-601b-189d-39a9-914e5574524c@sberdevices.ru/ > > >>>>> > > >>>>> > > >>>>> Patchset: > > >>>>> > > >>>>> Acked-by: Michael S. Tsirkin <mst@redhat.com> > > >>>> > > >>>> Thanks! > > >>>> > > >>>>> > > >>>>> > > >>>>> But I worry whether we actually need 3/8 in net not in net-next. > > >>>> > > >>>> Because of "Fixes" tag ? I think this problem is not critical and reproducible > > >>>> only in special cases, but i'm not familiar with netdev process so good, so I don't > > >>>> have strong opinion. I guess @Stefano knows better. > > >>>> > > >>>> Thanks, Arseniy > > >>> > > >>> Fixes means "if you have that other commit then you need this commit > > >>> too". I think as a minimum you need to rearrange patches to make the > > >>> fix go in first. We don't want a regression followed by a fix. > > >> > > >> I see, ok, @Stefano WDYT? I think rearrange doesn't break anything, because this > > >> patch fixes problem that is not related with the new patches from this patchset. > > > > > > I agree, patch 3 is for sure net material (I'm fine with both rearrangement or send it separately), but IMHO also patch 2 could be. > > > I think with the same fixes tag, since before commit b89d882dc9fc ("vsock/virtio: reduce credit update messages") we sent a credit update > > > for every bytes we read, so we should not have this problem, right? > > > > Agree for 2, so I think I can rearrange: two fixes go first, then current 0001, and then tests. And send it as V9 for 'net' only ? > > > > Thanks, Arseniy > > > hmm why not net-next? Oh I missed your previous discussion. I think everything in net-next is safer. Having said that, I won't nack it net, either. > > > > > > So, maybe all the series could be "net". > > > > > > Thanks, > > > Stefano > > >
On 13.12.2023 18:13, Michael S. Tsirkin wrote: > On Wed, Dec 13, 2023 at 10:05:44AM -0500, Michael S. Tsirkin wrote: >> On Wed, Dec 13, 2023 at 12:08:27PM +0300, Arseniy Krasnov wrote: >>> >>> >>> On 13.12.2023 11:43, Stefano Garzarella wrote: >>>> On Tue, Dec 12, 2023 at 08:43:07PM +0300, Arseniy Krasnov wrote: >>>>> >>>>> >>>>> On 12.12.2023 19:12, Michael S. Tsirkin wrote: >>>>>> On Tue, Dec 12, 2023 at 06:59:03PM +0300, Arseniy Krasnov wrote: >>>>>>> >>>>>>> >>>>>>> On 12.12.2023 18:54, Michael S. Tsirkin wrote: >>>>>>>> On Tue, Dec 12, 2023 at 12:16:54AM +0300, Arseniy Krasnov wrote: >>>>>>>>> Hello, >>>>>>>>> >>>>>>>>> DESCRIPTION >>>>>>>>> >>>>>>>>> This patchset fixes old problem with hungup of both rx/tx sides and adds >>>>>>>>> test for it. This happens due to non-default SO_RCVLOWAT value and >>>>>>>>> deferred credit update in virtio/vsock. Link to previous old patchset: >>>>>>>>> https://lore.kernel.org/netdev/39b2e9fd-601b-189d-39a9-914e5574524c@sberdevices.ru/ >>>>>>>> >>>>>>>> >>>>>>>> Patchset: >>>>>>>> >>>>>>>> Acked-by: Michael S. Tsirkin <mst@redhat.com> >>>>>>> >>>>>>> Thanks! >>>>>>> >>>>>>>> >>>>>>>> >>>>>>>> But I worry whether we actually need 3/8 in net not in net-next. >>>>>>> >>>>>>> Because of "Fixes" tag ? I think this problem is not critical and reproducible >>>>>>> only in special cases, but i'm not familiar with netdev process so good, so I don't >>>>>>> have strong opinion. I guess @Stefano knows better. >>>>>>> >>>>>>> Thanks, Arseniy >>>>>> >>>>>> Fixes means "if you have that other commit then you need this commit >>>>>> too". I think as a minimum you need to rearrange patches to make the >>>>>> fix go in first. We don't want a regression followed by a fix. >>>>> >>>>> I see, ok, @Stefano WDYT? I think rearrange doesn't break anything, because this >>>>> patch fixes problem that is not related with the new patches from this patchset. >>>> >>>> I agree, patch 3 is for sure net material (I'm fine with both rearrangement or send it separately), but IMHO also patch 2 could be. >>>> I think with the same fixes tag, since before commit b89d882dc9fc ("vsock/virtio: reduce credit update messages") we sent a credit update >>>> for every bytes we read, so we should not have this problem, right? >>> >>> Agree for 2, so I think I can rearrange: two fixes go first, then current 0001, and then tests. And send it as V9 for 'net' only ? >>> >>> Thanks, Arseniy >> >> >> hmm why not net-next? > > Oh I missed your previous discussion. I think everything in net-next is > safer. Having said that, I won't nack it net, either. So, summarizing all above: 1) This patchset entirely goes to net-next as v9 2) I reorder patches like 3 - 2 - 1 - 4, e.g. two fixes goes first with Fixes tag 3) Add Acked-by: Michael S. Tsirkin <mst@redhat.com> to each patch @Michael, @Stefano ? Thanks, Arseniy > >>>> >>>> So, maybe all the series could be "net". >>>> >>>> Thanks, >>>> Stefano >>>> >
On Wed, Dec 13, 2023 at 08:11:57PM +0300, Arseniy Krasnov wrote: > > > On 13.12.2023 18:13, Michael S. Tsirkin wrote: > > On Wed, Dec 13, 2023 at 10:05:44AM -0500, Michael S. Tsirkin wrote: > >> On Wed, Dec 13, 2023 at 12:08:27PM +0300, Arseniy Krasnov wrote: > >>> > >>> > >>> On 13.12.2023 11:43, Stefano Garzarella wrote: > >>>> On Tue, Dec 12, 2023 at 08:43:07PM +0300, Arseniy Krasnov wrote: > >>>>> > >>>>> > >>>>> On 12.12.2023 19:12, Michael S. Tsirkin wrote: > >>>>>> On Tue, Dec 12, 2023 at 06:59:03PM +0300, Arseniy Krasnov wrote: > >>>>>>> > >>>>>>> > >>>>>>> On 12.12.2023 18:54, Michael S. Tsirkin wrote: > >>>>>>>> On Tue, Dec 12, 2023 at 12:16:54AM +0300, Arseniy Krasnov wrote: > >>>>>>>>> Hello, > >>>>>>>>> > >>>>>>>>> DESCRIPTION > >>>>>>>>> > >>>>>>>>> This patchset fixes old problem with hungup of both rx/tx sides and adds > >>>>>>>>> test for it. This happens due to non-default SO_RCVLOWAT value and > >>>>>>>>> deferred credit update in virtio/vsock. Link to previous old patchset: > >>>>>>>>> https://lore.kernel.org/netdev/39b2e9fd-601b-189d-39a9-914e5574524c@sberdevices.ru/ > >>>>>>>> > >>>>>>>> > >>>>>>>> Patchset: > >>>>>>>> > >>>>>>>> Acked-by: Michael S. Tsirkin <mst@redhat.com> > >>>>>>> > >>>>>>> Thanks! > >>>>>>> > >>>>>>>> > >>>>>>>> > >>>>>>>> But I worry whether we actually need 3/8 in net not in net-next. > >>>>>>> > >>>>>>> Because of "Fixes" tag ? I think this problem is not critical and reproducible > >>>>>>> only in special cases, but i'm not familiar with netdev process so good, so I don't > >>>>>>> have strong opinion. I guess @Stefano knows better. > >>>>>>> > >>>>>>> Thanks, Arseniy > >>>>>> > >>>>>> Fixes means "if you have that other commit then you need this commit > >>>>>> too". I think as a minimum you need to rearrange patches to make the > >>>>>> fix go in first. We don't want a regression followed by a fix. > >>>>> > >>>>> I see, ok, @Stefano WDYT? I think rearrange doesn't break anything, because this > >>>>> patch fixes problem that is not related with the new patches from this patchset. > >>>> > >>>> I agree, patch 3 is for sure net material (I'm fine with both rearrangement or send it separately), but IMHO also patch 2 could be. > >>>> I think with the same fixes tag, since before commit b89d882dc9fc ("vsock/virtio: reduce credit update messages") we sent a credit update > >>>> for every bytes we read, so we should not have this problem, right? > >>> > >>> Agree for 2, so I think I can rearrange: two fixes go first, then current 0001, and then tests. And send it as V9 for 'net' only ? > >>> > >>> Thanks, Arseniy > >> > >> > >> hmm why not net-next? > > > > Oh I missed your previous discussion. I think everything in net-next is > > safer. Having said that, I won't nack it net, either. > > So, summarizing all above: > 1) This patchset entirely goes to net-next as v9 > 2) I reorder patches like 3 - 2 - 1 - 4, e.g. two fixes goes first with Fixes tag > 3) Add Acked-by: Michael S. Tsirkin <mst@redhat.com> to each patch > > @Michael, @Stefano ? > > Thanks, Arseniy Fine by me. > > > >>>> > >>>> So, maybe all the series could be "net". > >>>> > >>>> Thanks, > >>>> Stefano > >>>> > >
On Wed, Dec 13, 2023 at 08:11:57PM +0300, Arseniy Krasnov wrote: > > >On 13.12.2023 18:13, Michael S. Tsirkin wrote: >> On Wed, Dec 13, 2023 at 10:05:44AM -0500, Michael S. Tsirkin wrote: >>> On Wed, Dec 13, 2023 at 12:08:27PM +0300, Arseniy Krasnov wrote: >>>> >>>> >>>> On 13.12.2023 11:43, Stefano Garzarella wrote: >>>>> On Tue, Dec 12, 2023 at 08:43:07PM +0300, Arseniy Krasnov wrote: >>>>>> >>>>>> >>>>>> On 12.12.2023 19:12, Michael S. Tsirkin wrote: >>>>>>> On Tue, Dec 12, 2023 at 06:59:03PM +0300, Arseniy Krasnov wrote: >>>>>>>> >>>>>>>> >>>>>>>> On 12.12.2023 18:54, Michael S. Tsirkin wrote: >>>>>>>>> On Tue, Dec 12, 2023 at 12:16:54AM +0300, Arseniy Krasnov wrote: >>>>>>>>>> Hello, >>>>>>>>>> >>>>>>>>>> DESCRIPTION >>>>>>>>>> >>>>>>>>>> This patchset fixes old problem with hungup of both rx/tx sides and adds >>>>>>>>>> test for it. This happens due to non-default SO_RCVLOWAT value and >>>>>>>>>> deferred credit update in virtio/vsock. Link to previous old patchset: >>>>>>>>>> https://lore.kernel.org/netdev/39b2e9fd-601b-189d-39a9-914e5574524c@sberdevices.ru/ >>>>>>>>> >>>>>>>>> >>>>>>>>> Patchset: >>>>>>>>> >>>>>>>>> Acked-by: Michael S. Tsirkin <mst@redhat.com> >>>>>>>> >>>>>>>> Thanks! >>>>>>>> >>>>>>>>> >>>>>>>>> >>>>>>>>> But I worry whether we actually need 3/8 in net not in net-next. >>>>>>>> >>>>>>>> Because of "Fixes" tag ? I think this problem is not critical and reproducible >>>>>>>> only in special cases, but i'm not familiar with netdev process so good, so I don't >>>>>>>> have strong opinion. I guess @Stefano knows better. >>>>>>>> >>>>>>>> Thanks, Arseniy >>>>>>> >>>>>>> Fixes means "if you have that other commit then you need this commit >>>>>>> too". I think as a minimum you need to rearrange patches to make the >>>>>>> fix go in first. We don't want a regression followed by a fix. >>>>>> >>>>>> I see, ok, @Stefano WDYT? I think rearrange doesn't break anything, because this >>>>>> patch fixes problem that is not related with the new patches from this patchset. >>>>> >>>>> I agree, patch 3 is for sure net material (I'm fine with both rearrangement or send it separately), but IMHO also patch 2 could be. >>>>> I think with the same fixes tag, since before commit b89d882dc9fc ("vsock/virtio: reduce credit update messages") we sent a credit update >>>>> for every bytes we read, so we should not have this problem, right? >>>> >>>> Agree for 2, so I think I can rearrange: two fixes go first, then current 0001, and then tests. And send it as V9 for 'net' only ? >>>> >>>> Thanks, Arseniy >>> >>> >>> hmm why not net-next? >> >> Oh I missed your previous discussion. I think everything in net-next is >> safer. Having said that, I won't nack it net, either. > >So, summarizing all above: >1) This patchset entirely goes to net-next as v9 >2) I reorder patches like 3 - 2 - 1 - 4, e.g. two fixes goes first with Fixes tag >3) Add Acked-by: Michael S. Tsirkin <mst@redhat.com> to each patch > >@Michael, @Stefano ? Okay, let's do that ;-) Stefano
Hello, DESCRIPTION This patchset fixes old problem with hungup of both rx/tx sides and adds test for it. This happens due to non-default SO_RCVLOWAT value and deferred credit update in virtio/vsock. Link to previous old patchset: https://lore.kernel.org/netdev/39b2e9fd-601b-189d-39a9-914e5574524c@sberdevices.ru/ Here is what happens step by step: TEST INITIAL CONDITIONS 1) Vsock buffer size is 128KB. 2) Maximum packet size is also 64KB as defined in header (yes it is hardcoded, just to remind about that value). 3) SO_RCVLOWAT is default, e.g. 1 byte. STEPS SENDER RECEIVER 1) sends 128KB + 1 byte in a single buffer. 128KB will be sent, but for 1 byte sender will wait for free space at peer. Sender goes to sleep. 2) reads 64KB, credit update not sent 3) sets SO_RCVLOWAT to 64KB + 1 4) poll() -> wait forever, there is only 64KB available to read. So in step 4) receiver also goes to sleep, waiting for enough data or connection shutdown message from the sender. Idea to fix it is that rx kicks tx side to continue transmission (and may be close connection) when rx changes number of bytes to be woken up (e.g. SO_RCVLOWAT) and this value is bigger than number of available bytes to read. I've added small test for this, but not sure as it uses hardcoded value for maximum packet length, this value is defined in kernel header and used to control deferred credit update. And as this is not available to userspace, I can't control test parameters correctly (if one day this define will be changed - test may become useless). Head for this patchset is: https://git.kernel.org/pub/scm/linux/kernel/git/netdev/net-next.git/commit/?id=021b0c952f226236f2edf89c737efb9a28d1422d Link to v1: https://lore.kernel.org/netdev/20231108072004.1045669-1-avkrasnov@salutedevices.com/ Link to v2: https://lore.kernel.org/netdev/20231119204922.2251912-1-avkrasnov@salutedevices.com/ Link to v3: https://lore.kernel.org/netdev/20231122180510.2297075-1-avkrasnov@salutedevices.com/ Link to v4: https://lore.kernel.org/netdev/20231129212519.2938875-1-avkrasnov@salutedevices.com/ Link to v5: https://lore.kernel.org/netdev/20231130130840.253733-1-avkrasnov@salutedevices.com/ Link to v6: https://lore.kernel.org/netdev/20231205064806.2851305-1-avkrasnov@salutedevices.com/ Link to v7: https://lore.kernel.org/netdev/20231206211849.2707151-1-avkrasnov@salutedevices.com/ Changelog: v1 -> v2: * Patchset rebased and tested on new HEAD of net-next (see hash above). * New patch is added as 0001 - it removes return from SO_RCVLOWAT set callback in 'af_vsock.c' when transport callback is set - with that we can set 'sk_rcvlowat' only once in 'af_vsock.c' and in future do not copy-paste it to every transport. It was discussed in v1. * See per-patch changelog after ---. v2 -> v3: * See changelog after --- in 0003 only (0001 and 0002 still same). v3 -> v4: * Patchset rebased and tested on new HEAD of net-next (see hash above). * See per-patch changelog after ---. v4 -> v5: * Change patchset tag 'RFC' -> 'net-next'. * See per-patch changelog after ---. v5 -> v6: * New patch 0003 which sends credit update during reading bytes from socket. * See per-patch changelog after ---. v6 -> v7: * Patchset rebased and tested on new HEAD of net-next (see hash above). * See per-patch changelog after ---. v7 -> v8: * See per-patch changelog after ---. Arseniy Krasnov (4): vsock: update SO_RCVLOWAT setting callback virtio/vsock: send credit update during setting SO_RCVLOWAT virtio/vsock: fix logic which reduces credit update messages vsock/test: two tests to check credit update logic drivers/vhost/vsock.c | 1 + include/linux/virtio_vsock.h | 1 + include/net/af_vsock.h | 2 +- net/vmw_vsock/af_vsock.c | 9 +- net/vmw_vsock/hyperv_transport.c | 4 +- net/vmw_vsock/virtio_transport.c | 1 + net/vmw_vsock/virtio_transport_common.c | 43 +++++- net/vmw_vsock/vsock_loopback.c | 1 + tools/testing/vsock/vsock_test.c | 175 ++++++++++++++++++++++++ 9 files changed, 229 insertions(+), 8 deletions(-)