| Message ID | 20210115053553.1454517-1-arseny.krasnov@kaspersky.com (mailing list archive) |
|---|---|
| Headers | show |
| Series | virtio/vsock: introduce SOCK_SEQPACKET support. | expand |
15.01.2021 08:35, Arseny Krasnov пишет: > This patchset impelements support of SOCK_SEQPACKET for virtio > transport. > As SOCK_SEQPACKET guarantees to save record boundaries, so to > do it, new packet operation was added: it marks start of record (with > record length in header), such packet doesn't carry any data. To send > record, packet with start marker is sent first, then all data is sent > as usual 'RW' packets. On receiver's side, length of record is known > from packet with start record marker. Now as packets of one socket > are not reordered neither on vsock nor on vhost transport layers, such > marker allows to restore original record on receiver's side. If user's > buffer is smaller that than > record length, when then > v1 -> v2: > - patches reordered: af_vsock.c changes now before virtio vsock > - patches reorganized: more small patches, where +/- are not mixed If you did this because I asked, then this is not what I asked. :) You can't just add some static func in a separate patch, as it will just produce the compilation warning of an unused function. I only asked to separate the refactoring from the new code. I.e. if you move some code block to a separate function, you shouldn't split that into 2 patches, one that adds a code block and another one that removes it. It should be in one patch, so that it is clear what was moved, and no new warnings are introduced. What I asked to separate, is the old code moves with the new code additions. Such things can definitely go in a separate patches. NB: just trying to help, as I already played with your code a bit. I am neither a maintainer nor a contributor here, but it would be cool to have the vsock SEQPACKET support.
On Fri, Jan 15, 2021 at 12:59:30PM +0300, stsp wrote: >15.01.2021 08:35, Arseny Krasnov пишет: >> This patchset impelements support of SOCK_SEQPACKET for virtio >>transport. >> As SOCK_SEQPACKET guarantees to save record boundaries, so to >>do it, new packet operation was added: it marks start of record (with >>record length in header), such packet doesn't carry any data. To send >>record, packet with start marker is sent first, then all data is sent >>as usual 'RW' packets. On receiver's side, length of record is known >>from packet with start record marker. Now as packets of one socket >>are not reordered neither on vsock nor on vhost transport layers, such >>marker allows to restore original record on receiver's side. If user's >>buffer is smaller that > >than > > >> record length, when > >then > > >> v1 -> v2: >> - patches reordered: af_vsock.c changes now before virtio vsock >> - patches reorganized: more small patches, where +/- are not mixed > >If you did this because I asked, then this >is not what I asked. :) >You can't just add some static func in a >separate patch, as it will just produce the >compilation warning of an unused function. >I only asked to separate the refactoring from >the new code. I.e. if you move some code >block to a separate function, you shouldn't >split that into 2 patches, one that adds a >code block and another one that removes it. >It should be in one patch, so that it is clear >what was moved, and no new warnings are >introduced. >What I asked to separate, is the old code >moves with the new code additions. Such >things can definitely go in a separate patches. Arseny, thanks for the v2. I appreciated that you moved the af_vsock changes before the transport and also the test, but I agree with stsp about split patches. As stsp suggested, you can have some "preparation" patches that touch the already existing code (e.g. rename vsock_stream_sendmsg in vsock_connectible_sendmsg() and call it inside the new vsock_stream_sendmsg, etc.), then a patch that adds seqpacket stuff in af_vsock. Also for virtio/vhost transports, you can have some patches that add support in virtio_transport_common, then a patch that enable it in virtio_transport and a patch for vhost_vsock, as you rightly did in patch 12. So, I'd suggest moving out the code that touches virtio_transport.c from patch 11. These changes should simplify the review. In addition, you can also remove the . from the commit titles. I left other comments in the single patches. Thanks, Stefano
This patchset impelements support of SOCK_SEQPACKET for virtio transport. As SOCK_SEQPACKET guarantees to save record boundaries, so to do it, new packet operation was added: it marks start of record (with record length in header), such packet doesn't carry any data. To send record, packet with start marker is sent first, then all data is sent as usual 'RW' packets. On receiver's side, length of record is known from packet with start record marker. Now as packets of one socket are not reordered neither on vsock nor on vhost transport layers, such marker allows to restore original record on receiver's side. If user's buffer is smaller that record length, when all out of size data is dropped. Maximum length of datagram is not limited as in stream socket, because same credit logic is used. Difference with stream socket is that user is not woken up until whole record is received or error occurred. Implementation also supports 'MSG_EOR' and 'MSG_TRUNC' flags. Tests also implemented. Arseny Krasnov (13): af_vsock: implement 'vsock_wait_data()'. af_vsock: separate rx loops for STREAM/SEQPACKET. af_vsock: implement rx loops entry point af_vsock: replace previous stream rx loop. af_vsock: implement send logic for SOCK_SEQPACKET af_vsock: general support of SOCK_SEQPACKET type. af_vsock: update comments for stream sockets. virtio/vsock: dequeue callback for SOCK_SEQPACKET. virtio/vsock: implement fetch of record length virtio/vsock: update receive logic virtio/vsock: rest of SOCK_SEQPACKET support vhost/vsock: support for SOCK_SEQPACKET socket. vsock_test: add SOCK_SEQPACKET tests. drivers/vhost/vsock.c | 7 +- include/linux/virtio_vsock.h | 12 + include/net/af_vsock.h | 6 + include/uapi/linux/virtio_vsock.h | 9 + net/vmw_vsock/af_vsock.c | 483 ++++++++++++++++------ net/vmw_vsock/virtio_transport.c | 4 + net/vmw_vsock/virtio_transport_common.c | 294 +++++++++++-- tools/testing/vsock/util.c | 32 +- tools/testing/vsock/util.h | 3 + tools/testing/vsock/vsock_test.c | 126 ++++++ 10 files changed, 824 insertions(+), 152 deletions(-) v1 -> v2: - patches reordered: af_vsock.c changes now before virtio vsock - patches reorganized: more small patches, where +/- are not mixed - tests for SOCK_SEQPACKET added - all commit messages updated - af_vsock.c: 'vsock_pre_recv_check()' inlined to 'vsock_connectible_recvmsg()' - af_vsock.c: 'vsock_assign_transport()' returns ENODEV if transport was not found - virtio_transport_common.c: transport callback for seqpacket dequeue - virtio_transport_common.c: simplified 'virtio_transport_recv_connected()' - virtio_transport_common.c: send reset on socket and packet type mismatch. Signed-off-by: Arseny Krasnov <arseny.krasnov@kaspersky.com>