Stefano Garzarella
2021-Feb-01 14:23 UTC
[RFC PATCH v3 00/13] virtio/vsock: introduce SOCK_SEQPACKET support
On Mon, Feb 01, 2021 at 04:57:18PM +0300, Arseny Krasnov wrote:> >On 01.02.2021 14:02, Stefano Garzarella wrote: >> On Fri, Jan 29, 2021 at 06:52:23PM +0300, Arseny Krasnov wrote: >>> On 29.01.2021 12:26, Stefano Garzarella wrote: >>>> On Fri, Jan 29, 2021 at 09:41:50AM +0300, Arseny Krasnov wrote: >>>>> On 28.01.2021 20:19, Stefano Garzarella wrote: >>>>>> Hi Arseny, >>>>>> I reviewed a part, tomorrow I hope to finish the other patches. >>>>>> >>>>>> Just a couple of comments in the TODOs below. >>>>>> >>>>>> On Mon, Jan 25, 2021 at 02:09:00PM +0300, Arseny Krasnov wrote: >>>>>>> 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: prepare for SOCK_SEQPACKET support >>>>>>> af_vsock: prepare 'vsock_connectible_recvmsg()' >>>>>>> af_vsock: implement SEQPACKET rx loop >>>>>>> af_vsock: implement send logic for SOCK_SEQPACKET >>>>>>> af_vsock: rest of SEQPACKET support >>>>>>> af_vsock: update comments for stream sockets >>>>>>> virtio/vsock: dequeue callback for SOCK_SEQPACKET >>>>>>> virtio/vsock: fetch length for SEQPACKET record >>>>>>> virtio/vsock: add SEQPACKET receive logic >>>>>>> virtio/vsock: rest of SOCK_SEQPACKET support >>>>>>> virtio/vsock: setup SEQPACKET ops for transport >>>>>>> vhost/vsock: setup SEQPACKET ops for transport >>>>>>> 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 | 543 ++++++++++++++++------ >>>>>>> net/vmw_vsock/virtio_transport.c | 4 + >>>>>>> net/vmw_vsock/virtio_transport_common.c | 295 ++++++++++-- >>>>>>> tools/testing/vsock/util.c | 32 +- >>>>>>> tools/testing/vsock/util.h | 3 + >>>>>>> tools/testing/vsock/vsock_test.c | 126 +++++ >>>>>>> 10 files changed, 862 insertions(+), 175 deletions(-) >>>>>>> >>>>>>> TODO: >>>>>>> - Support for record integrity control. As transport could drop some >>>>>>> packets, something like "record-id" and record end marker need to >>>>>>> be implemented. Idea is that SEQ_BEGIN packet carries both record >>>>>>> length and record id, end marker(let it be SEQ_END) carries only >>>>>>> record id. To be sure that no one packet was lost, receiver checks >>>>>>> length of data between SEQ_BEGIN and SEQ_END(it must be same with >>>>>>> value in SEQ_BEGIN) and record ids of SEQ_BEGIN and SEQ_END(this >>>>>>> means that both markers were not dropped. I think that easiest way >>>>>>> to implement record id for SEQ_BEGIN is to reuse another field of >>>>>>> packet header(SEQ_BEGIN already uses 'flags' as record length).For >>>>>>> SEQ_END record id could be stored in 'flags'. >>>>>> I don't really like the idea of reusing the 'flags' field for this >>>>>> purpose. >>>>>> >>>>>>> Another way to implement it, is to move metadata of both SEQ_END >>>>>>> and SEQ_BEGIN to payload. But this approach has problem, because >>>>>>> if we move something to payload, such payload is accounted by >>>>>>> credit logic, which fragments payload, while payload with record >>>>>>> length and id couldn't be fragmented. One way to overcome it is to >>>>>>> ignore credit update for SEQ_BEGIN/SEQ_END packet.Another solution >>>>>>> is to update 'stream_has_space()' function: current implementation >>>>>>> return non-zero when at least 1 byte is allowed to use,but updated >>>>>>> version will have extra argument, which is needed length. For 'RW' >>>>>>> packet this argument is 1, for SEQ_BEGIN it is sizeof(record len + >>>>>>> record id) and for SEQ_END it is sizeof(record id). >>>>>> Is the payload accounted by credit logic also if hdr.op is not >>>>>> VIRTIO_VSOCK_OP_RW? >>>>> Yes, on send any packet with payload could be fragmented if >>>>> >>>>> there is not enough space at receiver. On receive 'fwd_cnt' and >>>>> >>>>> 'buf_alloc' are updated with header of every packet. Of course, >>>>> >>>>> to every such case i've described i can add check for 'RW' >>>>> >>>>> packet, to exclude payload from credit accounting, but this is >>>>> >>>>> bunch of dumb checks. >>>>> >>>>>> I think that we can define a specific header to put after the >>>>>> virtio_vsock_hdr when hdr.op is SEQ_BEGIN or SEQ_END, and in this header >>>>>> we can store the id and the length of the message. >>>>> I think it is better than use payload and touch credit logic >>>>> >>>> Cool, so let's try this option, hoping there aren't a lot of issues. >>> If i understand, current implementation has 'struct >>> virtio_vsock_hdr', >>> >>> then i'll add 'struct virtio_vsock_hdr_seq' with message length and id. >>> >>> After that, in 'struct virtio_vsock_pkt' which describes packet, field for >>> >>> header(which is 'struct virtio_vsock_hdr') must be replaced with new >>> >>> structure which? contains both 'struct virtio_vsock_hdr' and 'struct >>> >>> virtio_vsock_hdr_seq', because header field of 'struct virtio_vsock_pkt' >>> >>> is buffer for virtio layer. After it all accesses to header(for example to >>> >>> 'buf_alloc' field will go accross new? structure with both headers: >>> >>> pkt->hdr.buf_alloc?? ->?? pkt->extended_hdr.classic_hdr.buf_alloc >>> >>> May be to avoid this, packet's header could be allocated dynamically >>> >>> in the same manner as packet's buffer? Size of allocation is always >>> >>> sizeof(classic header) + sizeof(seq header). In 'struct virtio_vsock_pkt' >>> >>> such header will be implemented as union of two pointers: class header >>> >>> and extended header containing classic and seq header. Which pointer >>> >>> to use is depends on packet's op. >> I think that the 'classic header' can stay as is, and the extended >> header can be dynamically allocated, as we do for the payload. >> >> But we have to be careful what happens if the other peer doesn't support >> SEQPACKET and if it counts this extra header as a payload for the credit >> mechanism. > >You mean put extra header to payload(buffer of second virtio desc), > >in this way on send/receive auxiliary 'if's are needed to avoid credit > >logic(or set length field in header of such packets to 0). But what > >about placing extra header after classic header in buffer of first virtio > >desc? In this case extra header is not payload and credit works as is. > >Or it is critical, that size of first buffer will be not same as size of > >classic header?We need to think about compatibility with old drivers. What would happen in this case? I think it's easier to use the second buffer, usually used for the payload, to carry the extra header. Also, we can leave hdr.len = 0, so we are sure that it is not counted in credit mechanism. If the driver supports SEQPACKET, it knows it must fetch extra header when it must handle SEQ_BEGIN/SEQ_END. If it is not clear, I'll try to provide a simple PoC of a patch. Thanks, Stefano