[0/5] virtio/vsock: introduce SOCK_SEQPACKET support.

Message

Arseny Krasnov Jan. 3, 2021, 7:54 p.m. UTC

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). To send record, packet with start marker is
sent first, then all data is transmitted as '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, these marker allows to restore original record
on receiver's side. When each 'RW' packet is inserted to rx queue of
receiver, user is woken up, data is copied to user's buffer and credit
update message is sent. If there is no user waiting for data, credit
won't be updated and sender will wait. Also,  if user's buffer is full,
and record is bigger, all unneeded data will be dropped (with sending of
credit update message).
	'MSG_EOR' flag is implemented with special value of 'flags' field
in packet header. When record is received with such flags, 'MSG_EOR' is
set in 'recvmsg()' flags. 'MSG_TRUNC' flag is also supported.
	In this implementation maximum length of datagram is not limited
as in stream socket.

 drivers/vhost/vsock.c                   |   6 +-
 include/linux/virtio_vsock.h            |   7 +
 include/net/af_vsock.h                  |   4 +
 include/uapi/linux/virtio_vsock.h       |   9 +
 net/vmw_vsock/af_vsock.c                | 457 +++++++++++++++++++-----
 net/vmw_vsock/virtio_transport.c        |   3 +
 net/vmw_vsock/virtio_transport_common.c | 323 ++++++++++++++---
 7 files changed, 673 insertions(+), 136 deletions(-)

Comments

stsp Jan. 3, 2021, 9:18 p.m. UTC | #1

Hi Arseny, thanks for your work on this!

I did a small review in a hope it helps.
Also it may be cool to have the driver feature
for that (so that the host can see if its supported).
But I guess this was already said by Michael. :)

03.01.2021 22:54, Arseny Krasnov пишет:
> 	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). To send record, packet with start marker is
> sent first, then all data is transmitted as '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, these marker allows to restore original record
> on receiver's side. When each 'RW' packet is inserted to rx queue of
> receiver, user is woken up, data is copied to user's buffer and credit
> update message is sent. If there is no user waiting for data, credit
> won't be updated and sender will wait. Also,  if user's buffer is full,
> and record is bigger, all unneeded data will be dropped (with sending of
> credit update message).
> 	'MSG_EOR' flag is implemented with special value of 'flags' field
> in packet header. When record is received with such flags, 'MSG_EOR' is
> set in 'recvmsg()' flags. 'MSG_TRUNC' flag is also supported.
> 	In this implementation maximum length of datagram is not limited
> as in stream socket.
>
>   drivers/vhost/vsock.c                   |   6 +-
>   include/linux/virtio_vsock.h            |   7 +
>   include/net/af_vsock.h                  |   4 +
>   include/uapi/linux/virtio_vsock.h       |   9 +
>   net/vmw_vsock/af_vsock.c                | 457 +++++++++++++++++++-----
>   net/vmw_vsock/virtio_transport.c        |   3 +
>   net/vmw_vsock/virtio_transport_common.c | 323 ++++++++++++++---
>   7 files changed, 673 insertions(+), 136 deletions(-)
>

Arseny Krasnov Jan. 6, 2021, 7:33 p.m. UTC | #2

> Hi Arseny, thanks for your work on this!
> I did a small review in a hope it helps.
> Also it may be cool to have the driver feature
> for that (so that the host can see if its supported).
> But I guess this was already said by Michael. :)

Hello, thank You for your review, i'll prepare
v2 as soon as possible.

Stefano Garzarella Jan. 8, 2021, 10:33 a.m. UTC | #3

Hi Arseny,

On Sun, Jan 03, 2021 at 10:54:52PM +0300, Arseny Krasnov wrote:
>	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). To send record, packet with start marker is
>sent first, then all data is transmitted as '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, these marker allows to restore original record
>on receiver's side. When each 'RW' packet is inserted to rx queue of
>receiver, user is woken up, data is copied to user's buffer and credit
>update message is sent. If there is no user waiting for data, credit
>won't be updated and sender will wait. Also,  if user's buffer is full,
>and record is bigger, all unneeded data will be dropped (with sending of
>credit update message).
>	'MSG_EOR' flag is implemented with special value of 'flags' field
>in packet header. When record is received with such flags, 'MSG_EOR' is
>set in 'recvmsg()' flags. 'MSG_TRUNC' flag is also supported.
>	In this implementation maximum length of datagram is not limited
>as in stream socket.

I did a a quick review. I like the idea of adding SOCK_SEQPACKET, but 
the series needs more work.
Some patches miss the SoB, the commit messages are very minimal.
Anyway I like that you shared your patches, but please use RFC tag if 
they are not ready to be merged.

Another suggestion is to move the patches that modify the core 
(af_vsock.c) before the transport modifications to make the review 
easier.

I'd also like to see new tests in tools/testing/vsock/vsock_test.c

Thanks,
Stefano