Re: [RFC PATCH v3 00/13] virtio/vsock: introduce SOCK_SEQPACKET support

On Fri, Jan 29, 2021 at 06:52:23PM +0300, Arseny Krasnov wrote:

On 29.01.2021 12:26, Stefano Garzarella wrote:

quoted

On Fri, Jan 29, 2021 at 09:41:50AM +0300, Arseny Krasnov wrote:

quoted

On 28.01.2021 20:19, Stefano Garzarella wrote:

quoted

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:

quoted

	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.

quoted

    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.

quoted

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.

I'll try to take a closer look in the next few days.

Thanks,
Stefano