On Thu, Mar 09, 2023 at 07:20:20PM +0300, Arseniy Krasnov wrote:
On 09.03.2023 19:21, Stefano Garzarella wrote:
On Thu, Mar 09, 2023 at 01:10:36PM +0300, Arseniy Krasnov wrote:
Hello,
this patchset evolved from previous v2 version (see link below). It does
several updates to virtio/vsock:
1) Changes 'virtio_transport_inc/dec_rx_pkt()' interface. Now instead of
using skbuff state ('head' and 'data' pointers) to update 'fwd_cnt'
and 'rx_bytes', integer value is passed as an input argument. This
makes code more simple, because in this case we don't need to udpate
skbuff state before calling 'virtio_transport_inc/dec_rx_pkt()'. In
more common words - we don't need to change skbuff state to update
'rx_bytes' and 'fwd_cnt' correctly.
2) For SOCK_STREAM, when copying data to user fails, current skbuff is
not dropped. Next read attempt will use same skbuff and last offset.
Instead of 'skb_dequeue()', 'skb_peek()' + '__skb_unlink()' are used.
This behaviour was implemented before skbuff support.
3) For SOCK_SEQPACKET it removes unneeded 'skb_pull()' call, because for
this type of socket each skbuff is used only once: after removing it
from socket's queue, it will be freed anyway.
Test for 2) also added:
Test tries to 'recv()' data to NULL buffer, then does 'recv()' with valid
buffer. For SOCK_STREAM second 'recv()' must return data, because skbuff
must not be dropped, but for SOCK_SEQPACKET skbuff will be dropped by
kernel, and 'recv()' will return EAGAIN.
Link to v1 on lore:
https://lore.kernel.org/netdev/c2d3e204-89d9-88e9-8a15-3fe027e56b4b@xxxxxxxxxxxxxx/
Link to v2 on lore:
https://lore.kernel.org/netdev/a7ab414b-5e41-c7b6-250b-e8401f335859@xxxxxxxxxxxxxx/
Change log:
v1 -> v2:
- For SOCK_SEQPACKET call 'skb_pull()' also in case of copy failure or
dropping skbuff (when we just waiting message end).
- Handle copy failure for SOCK_STREAM in the same manner (plus free
current skbuff).
- Replace bug repdroducer with new test in vsock_test.c
v2 -> v3:
- Replace patch which removes 'skb->len' subtraction from function
'virtio_transport_dec_rx_pkt()' with patch which updates functions
'virtio_transport_inc/dec_rx_pkt()' by passing integer argument
instead of skbuff pointer.
- Replace patch which drops skbuff when copying to user fails with
patch which changes this behaviour by keeping skbuff in queue until
it has no data.
- Add patch for SOCK_SEQPACKET which removes redundant 'skb_pull()'
call on read.
- I remove "Fixes" tag from all patches, because all of them now change
code logic, not only fix something.
Yes, but they solve the problem, so we should use the tag (I think at
least in patch 1 and 3).
We usually use the tag when we are fixing a problem introduced by a
previous change. So we need to backport the patch to the stable branches
as well, and we need the tag to figure out which branches have the patch
or not.
Ahh, sorry. Ok. I see now :)
No problem at all :-)
I think also patch 2 can have the Fixes tag.
Thanks,
Stefano
Thanks, Arseniy
Thanks,
Stefano
Arseniy Krasnov (4):
virtio/vsock: don't use skbuff state to account credit
virtio/vsock: remove redundant 'skb_pull()' call
virtio/vsock: don't drop skbuff on copy failure
test/vsock: copy to user failure test
net/vmw_vsock/virtio_transport_common.c | 29 +++---
tools/testing/vsock/vsock_test.c | 118 ++++++++++++++++++++++++
2 files changed, 131 insertions(+), 16 deletions(-)
--
2.25.1
