This patchset modifies receive logic for SOCK_SEQPACKET.
Difference between current implementation and this version is that
now reader is woken up when there is at least one RW packet in rx
queue of socket and data is copied to user's buffer, while merged
approach wake up user only when whole message is received and kept
in queue. New implementation has several advantages:
1) There is no limit for message length. Merged approach requires
that length must be smaller than 'peer_buf_alloc', otherwise
transmission will stuck.
2) There is no need to keep whole message in queue, thus no
'kmalloc()' memory will be wasted until EOR is received.
Also new approach has some feature: as fragments of message
are copied until EOR is received, it is possible that part of
message will be already in user's buffer, while rest of message
still not received. And if user will be interrupted by signal or
timeout with part of message in buffer, it will exit receive loop,
leaving rest of message in queue. To solve this problem special
callback was added to transport: it is called when user was forced
to leave exit loop and tells transport to drop any packet until
EOR met. When EOR is found, this mode is disabled and normal packet
processing started. Note, that when 'drop until EOR' mode is on,
incoming packets still inserted in queue, reader will be woken up,
tries to copy data, but nothing will be copied until EOR found.
It was possible to drain such unneeded packets it rx work without
kicking user, but implemented way is simplest. Anyway, i think
such cases are rare.
New test also added - it tries to copy to invalid user's
buffer.
Arseny Krasnov (16):
vhost/vsock: don't set 'seqpacket_has_data()' callback
vsock/loopback: don't set 'seqpacket_has_data()' callback
virtio/vsock: don't set 'seqpacket_has_data()' callback
virtio/vsock: remove 'virtio_transport_seqpacket_has_data'
af_vsock: use SOCK_STREAM function to check data
vsock/virtio: remove record size limit for SEQPACKET
virtio/vsock: don't count EORs on receive
af_vsock: change SEQPACKET receive loop
af_vsock/virtio: update dequeue callback interface
virtio/vsock: update SEQPACKET dequeue logic
afvsock: add 'seqpacket_drop()'
virtio/vsock: add 'drop until EOR' logic
vhost/vsock: enable 'seqpacket_drop' callback in transport
virtio/vsock: enable 'seqpacket_drop' callback in transport
vsock/loopback: enable 'seqpacket_drop' callback in transport
vsock_test: SEQPACKET read to broken buffer
drivers/vhost/vsock.c | 2 +-
include/linux/virtio_vsock.h | 7 +-
include/net/af_vsock.h | 4 +-
net/vmw_vsock/af_vsock.c | 44 ++++----
net/vmw_vsock/virtio_transport.c | 2 +-
net/vmw_vsock/virtio_transport_common.c | 103 +++++++-----------
net/vmw_vsock/vsock_loopback.c | 2 +-
tools/testing/vsock/vsock_test.c | 121 ++++++++++++++++++++++
8 files changed, 194 insertions(+), 91 deletions(-)
Signed-off-by: Arseny Krasnov <redacted>
--
2.25.1
As now 'rx_bytes' is used to check presence of data on socket,
this function is obsolete.
Signed-off-by: Arseny Krasnov <redacted>
---
include/linux/virtio_vsock.h | 1 -
net/vmw_vsock/virtio_transport_common.c | 13 -------------
2 files changed, 14 deletions(-)
Remove record size limit which was 'peer_buf_alloc' value.
New approach doesn't need this, because data is copied to
user's buffer in stream manner(we don't wait until whole
record is received).
Signed-off-by: Arseny Krasnov <redacted>
---
net/vmw_vsock/virtio_transport_common.c | 11 -----------
1 file changed, 11 deletions(-)
There is no sense to count EORs, because 'rx_bytes' is
used to check data presence on socket.
Signed-off-by: Arseny Krasnov <redacted>
---
net/vmw_vsock/virtio_transport_common.c | 3 ---
1 file changed, 3 deletions(-)
@@ -1005,9 +1005,6 @@ virtio_transport_recv_enqueue(struct vsock_sock *vsk,gotoout;}-if(le32_to_cpu(pkt->hdr.flags)&VIRTIO_VSOCK_SEQ_EOR)-vvs->msg_count++;-/* Try to copy small packets into the buffer of last packet queued,*toavoidwastingmemoryqueueingtheentirebufferwithasmall*payload.
Receive "loop" now really loop: it reads fragments one by
one, sleeping if queue is empty.
NOTE: 'msg_ready' pointer is not passed to 'seqpacket_dequeue()'
here - it change callback interface, so it is moved to next patch.
Signed-off-by: Arseny Krasnov <redacted>
---
net/vmw_vsock/af_vsock.c | 31 ++++++++++++++++++++++---------
1 file changed, 22 insertions(+), 9 deletions(-)
@@ -2013,23 +2014,36 @@ static int __vsock_seqpacket_recvmsg(struct sock *sk, struct msghdr *msg,transport=vsk->transport;timeout=sock_rcvtimeo(sk,flags&MSG_DONTWAIT);+msg_ready=false;+record_len=0;-err=vsock_connectible_wait_data(sk,&wait,timeout,NULL,0);-if(err<=0)-gotoout;+while(!msg_ready){+ssize_tfragment_len;+intintr_err;-record_len=transport->seqpacket_dequeue(vsk,msg,flags);+intr_err=vsock_connectible_wait_data(sk,&wait,timeout,NULL,0);+if(intr_err<=0){+err=intr_err;+break;+}-if(record_len<0){-err=-ENOMEM;-gotoout;+fragment_len=transport->seqpacket_dequeue(vsk,msg,flags);++if(fragment_len<0){+err=-ENOMEM;+break;+}++record_len+=fragment_len;}if(sk->sk_err){err=-sk->sk_err;}elseif(sk->sk_shutdown&RCV_SHUTDOWN){err=0;-}else{+}++if(msg_ready&&!err){/* User sets MSG_TRUNC, so return real length of*packet.*/
This patch adds 'msg_ready' parameter to dequeue callback, it is
set to true if EOR is found(in case of virtio transport).
This patch contains small changes for both af_vsock and virtio
transport code to avoid build fails with partly applied patchset.
Signed-off-by: Arseny Krasnov <redacted>
---
include/linux/virtio_vsock.h | 3 ++-
include/net/af_vsock.h | 2 +-
net/vmw_vsock/af_vsock.c | 2 +-
net/vmw_vsock/virtio_transport_common.c | 2 +-
4 files changed, 5 insertions(+), 4 deletions(-)
@@ -407,59 +407,48 @@ virtio_transport_stream_do_dequeue(struct vsock_sock *vsk,staticintvirtio_transport_seqpacket_do_dequeue(structvsock_sock*vsk,structmsghdr*msg,-intflags)+intflags,+bool*msg_ready){structvirtio_vsock_sock*vvs=vsk->trans;structvirtio_vsock_pkt*pkt;intdequeued_len=0;size_tuser_buf_len=msg_data_left(msg);-boolmsg_ready=false;+*msg_ready=false;spin_lock_bh(&vvs->rx_lock);-if(vvs->msg_count==0){-spin_unlock_bh(&vvs->rx_lock);-return0;-}+while(!*msg_ready&&!list_empty(&vvs->rx_queue)&&dequeued_len>=0){+size_tpkt_len;+size_tbytes_to_copy;-while(!msg_ready){pkt=list_first_entry(&vvs->rx_queue,structvirtio_vsock_pkt,list);+pkt_len=(size_t)le32_to_cpu(pkt->hdr.len);-if(dequeued_len>=0){-size_tpkt_len;-size_tbytes_to_copy;+bytes_to_copy=min(user_buf_len,pkt_len);-pkt_len=(size_t)le32_to_cpu(pkt->hdr.len);-bytes_to_copy=min(user_buf_len,pkt_len);--if(bytes_to_copy){-interr;--/* sk_lock is held by caller so no one else can dequeue.-*Unlockrx_locksincememcpy_to_msg()maysleep.-*/-spin_unlock_bh(&vvs->rx_lock);+if(bytes_to_copy){+interr;+/* sk_lock is held by caller so no one else can dequeue.+*Unlockrx_locksincememcpy_to_msg()maysleep.+*/+spin_unlock_bh(&vvs->rx_lock);-err=memcpy_to_msg(msg,pkt->buf,bytes_to_copy);-if(err){-/* Copy of message failed. Rest of-*fragmentswillbefreedwithoutcopy.-*/-dequeued_len=err;-}else{-user_buf_len-=bytes_to_copy;-}+err=memcpy_to_msg(msg,pkt->buf,bytes_to_copy);-spin_lock_bh(&vvs->rx_lock);-}+spin_lock_bh(&vvs->rx_lock);-if(dequeued_len>=0)-dequeued_len+=pkt_len;+if(err)+dequeued_len=err;+else+user_buf_len-=bytes_to_copy;}+if(dequeued_len>=0)+dequeued_len+=pkt_len;+if(le32_to_cpu(pkt->hdr.flags)&VIRTIO_VSOCK_SEQ_EOR){-msg_ready=true;-vvs->msg_count--;+*msg_ready=true;}virtio_transport_dec_rx_pkt(vvs,pkt);
Add special callback for SEQPACKET socket which is called when
we need to drop current in-progress record: part of record was
copied successfully, reader wait rest of record, but signal
interrupts it and reader leaves it's loop, leaving packets of
current record still in queue. So to avoid copy of "orphaned"
record, we tell transport to drop every packet until EOR will
be found.
Signed-off-by: Arseny Krasnov <redacted>
---
include/net/af_vsock.h | 1 +
net/vmw_vsock/af_vsock.c | 1 +
2 files changed, 2 insertions(+)
Data will copied only if 'drop until EOR' mode is disabled, also
if EOR found, 'msg_ready' is set only if we don't have current
message to drop.
Signed-off-by: Arseny Krasnov <redacted>
---
include/linux/virtio_vsock.h | 2 ++
net/vmw_vsock/virtio_transport_common.c | 23 +++++++++++++++++++----
2 files changed, 21 insertions(+), 4 deletions(-)
Add test where sender sends two message, each with own
data pattern. Reader tries to read first to broken buffer:
it has three pages size, but middle page is unmapped. Then,
reader tries to read second message to valid buffer. Test
checks, that uncopied part of first message was dropped
and thus not copied as part of second message.
Signed-off-by: Arseny Krasnov <redacted>
---
tools/testing/vsock/vsock_test.c | 121 +++++++++++++++++++++++++++++++
1 file changed, 121 insertions(+)
@@ -385,6 +386,121 @@ static void test_seqpacket_msg_trunc_server(const struct test_opts *opts)close(fd);}+#define BUF_PATTERN_1 'a'+#define BUF_PATTERN_2 'b'++staticvoidtest_seqpacket_invalid_rec_buffer_client(conststructtest_opts*opts)+{+intfd;+unsignedchar*buf1;+unsignedchar*buf2;+intbuf_size=getpagesize()*3;++fd=vsock_seqpacket_connect(opts->peer_cid,1234);+if(fd<0){+perror("connect");+exit(EXIT_FAILURE);+}++buf1=malloc(buf_size);+if(buf1==NULL){+perror("'malloc()' for 'buf1'");+exit(EXIT_FAILURE);+}++buf2=malloc(buf_size);+if(buf2==NULL){+perror("'malloc()' for 'buf2'");+exit(EXIT_FAILURE);+}++memset(buf1,BUF_PATTERN_1,buf_size);+memset(buf2,BUF_PATTERN_2,buf_size);++if(send(fd,buf1,buf_size,0)!=buf_size){+perror("send failed");+exit(EXIT_FAILURE);+}++if(send(fd,buf2,buf_size,0)!=buf_size){+perror("send failed");+exit(EXIT_FAILURE);+}++close(fd);+}++staticvoidtest_seqpacket_invalid_rec_buffer_server(conststructtest_opts*opts)+{+intfd;+unsignedchar*broken_buf;+unsignedchar*valid_buf;+intpage_size=getpagesize();+intbuf_size=page_size*3;+ssize_tres;+intprot=PROT_READ|PROT_WRITE;+intflags=MAP_PRIVATE|MAP_ANONYMOUS;+inti;++fd=vsock_seqpacket_accept(VMADDR_CID_ANY,1234,NULL);+if(fd<0){+perror("accept");+exit(EXIT_FAILURE);+}++/* Setup first buffer. */+broken_buf=mmap(NULL,buf_size,prot,flags,-1,0);+if(broken_buf==MAP_FAILED){+perror("mmap for 'broken_buf'");+exit(EXIT_FAILURE);+}++/* Unmap "hole" in buffer. */+if(munmap(broken_buf+page_size,page_size)){+perror("'broken_buf' setup");+exit(EXIT_FAILURE);+}++valid_buf=mmap(NULL,buf_size,prot,flags,-1,0);+if(valid_buf==MAP_FAILED){+perror("mmap for 'valid_buf'");+exit(EXIT_FAILURE);+}++/* Try to fill buffer with unmapped middle. */+res=read(fd,broken_buf,buf_size);+if(res!=-1){+perror("invalid read result of 'broken_buf'");+exit(EXIT_FAILURE);+}++if(errno!=ENOMEM){+perror("invalid errno of 'broken_buf'");+exit(EXIT_FAILURE);+}++/* Try to fill valid buffer. */+res=read(fd,valid_buf,buf_size);+if(res!=buf_size){+perror("invalid read result of 'valid_buf'");+exit(EXIT_FAILURE);+}++for(i=0;i<buf_size;i++){+if(valid_buf[i]!=BUF_PATTERN_2){+perror("invalid pattern for valid buf");+exit(EXIT_FAILURE);+}+}+++/* Unmap buffers. */+munmap(broken_buf,page_size);+munmap(broken_buf+page_size*2,page_size);+munmap(valid_buf,buf_size);+close(fd);+}+staticstructtest_casetest_cases[]={{.name="SOCK_STREAM connection reset",
In order to avoid issues while bisecting the kernel, we should have
commit that doesn't break the build or the runtime, so please take this
in mind also for other commits.
For example here we removed the seqpacket_has_data callbacks assignment
before to remove where we use the callback, with a potential fault at
runtime.
I think you can simply put patches from 1 to 5 together in a single
patch.
In addition, we should move these changes after we don't need
vsock_connectible_has_data() anymore, for example, where we replace the
receive loop logic.
Thanks,
Stefano
On Mon, Jun 28, 2021 at 01:03:15PM +0300, Arseny Krasnov wrote:
quoted hunk
There is no sense to count EORs, because 'rx_bytes' is
used to check data presence on socket.
Signed-off-by: Arseny Krasnov <redacted>
---
net/vmw_vsock/virtio_transport_common.c | 3 ---
1 file changed, 3 deletions(-)
Same here, please remove it when you don't need it, and also remove from
the struct virtio_vsock_sock.
Thanks,
Stefano
/* Try to copy small packets into the buffer of last packet queued,
* to avoid wasting memory queueing the entire buffer with a small
* payload.
--
2.25.1
On Mon, Jun 28, 2021 at 01:03:28PM +0300, Arseny Krasnov wrote:
Receive "loop" now really loop: it reads fragments one by
one, sleeping if queue is empty.
NOTE: 'msg_ready' pointer is not passed to 'seqpacket_dequeue()'
here - it change callback interface, so it is moved to next patch.
Signed-off-by: Arseny Krasnov <redacted>
---
net/vmw_vsock/af_vsock.c | 31 ++++++++++++++++++++++---------
1 file changed, 22 insertions(+), 9 deletions(-)
I think you can merge patches 8, 9, and 10 together since we
are touching the seqpacket_dequeue() behaviour.
Then you can remove in separate patches the unneeded parts (e.g.
seqpacket_has_data, msg_count, etc.).
Thanks,
Stefano
On Mon, Jun 28, 2021 at 01:04:12PM +0300, Arseny Krasnov wrote:
Add special callback for SEQPACKET socket which is called when
we need to drop current in-progress record: part of record was
copied successfully, reader wait rest of record, but signal
interrupts it and reader leaves it's loop, leaving packets of
current record still in queue. So to avoid copy of "orphaned"
record, we tell transport to drop every packet until EOR will
be found.
Signed-off-by: Arseny Krasnov <redacted>
---
include/net/af_vsock.h | 1 +
net/vmw_vsock/af_vsock.c | 1 +
2 files changed, 2 insertions(+)
And also for this change, I think you can merge with patches 12, 13, 14,
15, otherwise if we bisect and we build at this patch, the
seqpacket_drop pointer is not valid.
Thanks,
Stefano
On Mon, Jun 28, 2021 at 01:05:36PM +0300, Arseny Krasnov wrote:
Add test where sender sends two message, each with own
data pattern. Reader tries to read first to broken buffer:
it has three pages size, but middle page is unmapped. Then,
reader tries to read second message to valid buffer. Test
checks, that uncopied part of first message was dropped
and thus not copied as part of second message.
Signed-off-by: Arseny Krasnov <redacted>
---
tools/testing/vsock/vsock_test.c | 121 +++++++++++++++++++++++++++++++
1 file changed, 121 insertions(+)
In order to avoid issues while bisecting the kernel, we should have
commit that doesn't break the build or the runtime, so please take this
in mind also for other commits.
For example here we removed the seqpacket_has_data callbacks assignment
before to remove where we use the callback, with a potential fault at
runtime.
I think you can simply put patches from 1 to 5 together in a single
patch.
In addition, we should move these changes after we don't need
vsock_connectible_has_data() anymore, for example, where we replace the
receive loop logic.
Thanks,
Stefano
On Mon, Jun 28, 2021 at 01:03:15PM +0300, Arseny Krasnov wrote:
quoted
There is no sense to count EORs, because 'rx_bytes' is
used to check data presence on socket.
Signed-off-by: Arseny Krasnov <redacted>
---
net/vmw_vsock/virtio_transport_common.c | 3 ---
1 file changed, 3 deletions(-)
Same here, please remove it when you don't need it, and also remove from
the struct virtio_vsock_sock.
Thanks,
Stefano
Ack
quoted
/* Try to copy small packets into the buffer of last packet queued,
* to avoid wasting memory queueing the entire buffer with a small
* payload.
--
2.25.1
On Mon, Jun 28, 2021 at 01:03:28PM +0300, Arseny Krasnov wrote:
quoted
Receive "loop" now really loop: it reads fragments one by
one, sleeping if queue is empty.
NOTE: 'msg_ready' pointer is not passed to 'seqpacket_dequeue()'
here - it change callback interface, so it is moved to next patch.
Signed-off-by: Arseny Krasnov <redacted>
---
net/vmw_vsock/af_vsock.c | 31 ++++++++++++++++++++++---------
1 file changed, 22 insertions(+), 9 deletions(-)
I think you can merge patches 8, 9, and 10 together since we
are touching the seqpacket_dequeue() behaviour.
Then you can remove in separate patches the unneeded parts (e.g.
seqpacket_has_data, msg_count, etc.).
Thanks,
Stefano
On Mon, Jun 28, 2021 at 01:04:12PM +0300, Arseny Krasnov wrote:
quoted
Add special callback for SEQPACKET socket which is called when
we need to drop current in-progress record: part of record was
copied successfully, reader wait rest of record, but signal
interrupts it and reader leaves it's loop, leaving packets of
current record still in queue. So to avoid copy of "orphaned"
record, we tell transport to drop every packet until EOR will
be found.
Signed-off-by: Arseny Krasnov <redacted>
---
include/net/af_vsock.h | 1 +
net/vmw_vsock/af_vsock.c | 1 +
2 files changed, 2 insertions(+)
And also for this change, I think you can merge with patches 12, 13, 14,
15, otherwise if we bisect and we build at this patch, the
seqpacket_drop pointer is not valid.
Thanks,
Stefano