Re: [RFC PATCH v5 11/19] virtio/vsock: dequeue callback for SOCK_SEQPACKET

On Wed, Feb 24, 2021 at 08:07:48AM +0300, Arseny Krasnov wrote:

On 23.02.2021 17:17, Michael S. Tsirkin wrote:

quoted

On Thu, Feb 18, 2021 at 08:39:37AM +0300, Arseny Krasnov wrote:

quoted

This adds transport callback and it's logic for SEQPACKET dequeue.
Callback fetches RW packets from rx queue of socket until whole record
is copied(if user's buffer is full, user is not woken up). This is done
to not stall sender, because if we wake up user and it leaves syscall,
nobody will send credit update for rest of record, and sender will wait
for next enter of read syscall at receiver's side. So if user buffer is
full, we just send credit update and drop data. If during copy SEQ_BEGIN
was found(and not all data was copied), copying is restarted by reset
user's iov iterator(previous unfinished data is dropped).

Signed-off-by: Arseny Krasnov <redacted>
---
 include/linux/virtio_vsock.h            |  10 +++
 include/uapi/linux/virtio_vsock.h       |  16 ++++
 net/vmw_vsock/virtio_transport_common.c | 114 ++++++++++++++++++++++++
 3 files changed, 140 insertions(+)

diff --git a/include/linux/virtio_vsock.h b/include/linux/virtio_vsock.h
index dc636b727179..003d06ae4a85 100644
--- a/include/linux/virtio_vsock.h
+++ b/include/linux/virtio_vsock.h

@@ -36,6 +36,11 @@ struct virtio_vsock_sock {
 	u32 rx_bytes;
 	u32 buf_alloc;
 	struct list_head rx_queue;
+
+	/* For SOCK_SEQPACKET */
+	u32 user_read_seq_len;
+	u32 user_read_copied;
+	u32 curr_rx_msg_cnt;

wrap these in a struct to make it's clearer they
are related?

Ack

quoted

quoted

 };
 

66 more quoted lines

Well patching userspace to make use of a kernel feature
is par for the course, isn't it?

IMHO I'm not sure that SEQPACKET support needs feature

bit - it is just two new ops for virtio vsock protocol, and from point

of view of virtio device it is same as STREAM. May be it is needed

for cases when client tries to connect to server which doesn't support

SEQPACKET, so without bit result will be "Connection reset by peer",

and with such bit client will know that server doesn't support it and

'socket(SOCK_SEQPACKET)' will return error?

Yes, a better error handling would be one reason to do it like this.

-- 
MST

_______________________________________________
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/virtualization

On Wed, Feb 24, 2021 at 01:41:56AM -0500, Michael S. Tsirkin wrote:

On Wed, Feb 24, 2021 at 08:07:48AM +0300, Arseny Krasnov wrote:

quoted

On 23.02.2021 17:17, Michael S. Tsirkin wrote:

quoted

On Thu, Feb 18, 2021 at 08:39:37AM +0300, Arseny Krasnov wrote:

quoted

This adds transport callback and it's logic for SEQPACKET dequeue.
Callback fetches RW packets from rx queue of socket until whole record
is copied(if user's buffer is full, user is not woken up). This is done
to not stall sender, because if we wake up user and it leaves syscall,
nobody will send credit update for rest of record, and sender will wait
for next enter of read syscall at receiver's side. So if user buffer is
full, we just send credit update and drop data. If during copy SEQ_BEGIN
was found(and not all data was copied), copying is restarted by reset
user's iov iterator(previous unfinished data is dropped).

Signed-off-by: Arseny Krasnov <redacted>
---
 include/linux/virtio_vsock.h            |  10 +++
 include/uapi/linux/virtio_vsock.h       |  16 ++++
 net/vmw_vsock/virtio_transport_common.c | 114 ++++++++++++++++++++++++
 3 files changed, 140 insertions(+)

diff --git a/include/linux/virtio_vsock.h b/include/linux/virtio_vsock.h
index dc636b727179..003d06ae4a85 100644
--- a/include/linux/virtio_vsock.h
+++ b/include/linux/virtio_vsock.h

@@ -36,6 +36,11 @@ struct virtio_vsock_sock {
 	u32 rx_bytes;
 	u32 buf_alloc;
 	struct list_head rx_queue;
+
+	/* For SOCK_SEQPACKET */
+	u32 user_read_seq_len;
+	u32 user_read_copied;
+	u32 curr_rx_msg_cnt;

wrap these in a struct to make it's clearer they
are related?

Ack

quoted

quoted

 };

87 more quoted lines

Agree, in this way we could implement a 'seqpacket_allow' callback (similar to 'stream_allow'), and we can return 'true' if the feature is negotiated.

So instead of checking all the seqpacket callbacks, we can use only this callback to understand if the transport support it.
We can implement it also for other transports (vmci, hyperv) and return always false for now.

Thanks,
Stefano

_______________________________________________
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/virtualization