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 update
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/[email protected]/
Link to v2 on lore:
https://lore.kernel.org/netdev/[email protected]/
Link to v3 on lore:
https://lore.kernel.org/netdev/[email protected]/
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.
v3 -> v4:
- Update commit messages in all patches except test.
- Add "Fixes" tag to all patches except test.
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
'skb->len' can vary when we partially read the data, this complicates the
calculation of credit to be updated in 'virtio_transport_inc_rx_pkt()/
virtio_transport_dec_rx_pkt()'.
Also in 'virtio_transport_dec_rx_pkt()' we were miscalculating the
credit since 'skb->len' was redundant.
For these reasons, let's replace the use of skbuff state to calculate new
'rx_bytes'/'fwd_cnt' values with explicit value as input argument. This
makes code more simple, because it is not needed to change skbuff state
before each call to update 'rx_bytes'/'fwd_cnt'.
Fixes: 71dc9ec9ac7d ("virtio/vsock: replace virtio_vsock_pkt with sk_buff")
Signed-off-by: Arseniy Krasnov <[email protected]>
---
net/vmw_vsock/virtio_transport_common.c | 23 +++++++++++------------
1 file changed, 11 insertions(+), 12 deletions(-)
diff --git a/net/vmw_vsock/virtio_transport_common.c b/net/vmw_vsock/virtio_transport_common.c
index a1581c77cf84..618680fd9906 100644
--- a/net/vmw_vsock/virtio_transport_common.c
+++ b/net/vmw_vsock/virtio_transport_common.c
@@ -241,21 +241,18 @@ static int virtio_transport_send_pkt_info(struct vsock_sock *vsk,
}
static bool virtio_transport_inc_rx_pkt(struct virtio_vsock_sock *vvs,
- struct sk_buff *skb)
+ u32 len)
{
- if (vvs->rx_bytes + skb->len > vvs->buf_alloc)
+ if (vvs->rx_bytes + len > vvs->buf_alloc)
return false;
- vvs->rx_bytes += skb->len;
+ vvs->rx_bytes += len;
return true;
}
static void virtio_transport_dec_rx_pkt(struct virtio_vsock_sock *vvs,
- struct sk_buff *skb)
+ u32 len)
{
- int len;
-
- len = skb_headroom(skb) - sizeof(struct virtio_vsock_hdr) - skb->len;
vvs->rx_bytes -= len;
vvs->fwd_cnt += len;
}
@@ -388,7 +385,9 @@ virtio_transport_stream_do_dequeue(struct vsock_sock *vsk,
skb_pull(skb, bytes);
if (skb->len == 0) {
- virtio_transport_dec_rx_pkt(vvs, skb);
+ u32 pkt_len = le32_to_cpu(virtio_vsock_hdr(skb)->len);
+
+ virtio_transport_dec_rx_pkt(vvs, pkt_len);
consume_skb(skb);
} else {
__skb_queue_head(&vvs->rx_queue, skb);
@@ -437,17 +436,17 @@ static int virtio_transport_seqpacket_do_dequeue(struct vsock_sock *vsk,
while (!msg_ready) {
struct virtio_vsock_hdr *hdr;
+ size_t pkt_len;
skb = __skb_dequeue(&vvs->rx_queue);
if (!skb)
break;
hdr = virtio_vsock_hdr(skb);
+ pkt_len = (size_t)le32_to_cpu(hdr->len);
if (dequeued_len >= 0) {
- size_t pkt_len;
size_t bytes_to_copy;
- pkt_len = (size_t)le32_to_cpu(hdr->len);
bytes_to_copy = min(user_buf_len, pkt_len);
if (bytes_to_copy) {
@@ -484,7 +483,7 @@ static int virtio_transport_seqpacket_do_dequeue(struct vsock_sock *vsk,
msg->msg_flags |= MSG_EOR;
}
- virtio_transport_dec_rx_pkt(vvs, skb);
+ virtio_transport_dec_rx_pkt(vvs, pkt_len);
kfree_skb(skb);
}
@@ -1040,7 +1039,7 @@ virtio_transport_recv_enqueue(struct vsock_sock *vsk,
spin_lock_bh(&vvs->rx_lock);
- can_enqueue = virtio_transport_inc_rx_pkt(vvs, skb);
+ can_enqueue = virtio_transport_inc_rx_pkt(vvs, len);
if (!can_enqueue) {
free_pkt = true;
goto out;
--
2.25.1
Since we now no longer use 'skb->len' to update credit, there is no sense
to update skbuff state, because it is used only once after dequeue to
copy data and then will be released.
Fixes: 71dc9ec9ac7d ("virtio/vsock: replace virtio_vsock_pkt with sk_buff")
Signed-off-by: Arseniy Krasnov <[email protected]>
---
net/vmw_vsock/virtio_transport_common.c | 1 -
1 file changed, 1 deletion(-)
diff --git a/net/vmw_vsock/virtio_transport_common.c b/net/vmw_vsock/virtio_transport_common.c
index 618680fd9906..9a411475e201 100644
--- a/net/vmw_vsock/virtio_transport_common.c
+++ b/net/vmw_vsock/virtio_transport_common.c
@@ -465,7 +465,6 @@ static int virtio_transport_seqpacket_do_dequeue(struct vsock_sock *vsk,
dequeued_len = err;
} else {
user_buf_len -= bytes_to_copy;
- skb_pull(skb, bytes_to_copy);
}
spin_lock_bh(&vvs->rx_lock);
--
2.25.1
This returns behaviour of SOCK_STREAM read as before skbuff usage. When
copying to user fails current skbuff won't be dropped, but returned to
sockets's queue. Technically instead of 'skb_dequeue()', 'skb_peek()' is
called and when skbuff becomes empty, it is removed from queue by
'__skb_unlink()'.
Fixes: 71dc9ec9ac7d ("virtio/vsock: replace virtio_vsock_pkt with sk_buff")
Signed-off-by: Arseniy Krasnov <[email protected]>
---
net/vmw_vsock/virtio_transport_common.c | 5 ++---
1 file changed, 2 insertions(+), 3 deletions(-)
diff --git a/net/vmw_vsock/virtio_transport_common.c b/net/vmw_vsock/virtio_transport_common.c
index 9a411475e201..6564192e7f20 100644
--- a/net/vmw_vsock/virtio_transport_common.c
+++ b/net/vmw_vsock/virtio_transport_common.c
@@ -364,7 +364,7 @@ virtio_transport_stream_do_dequeue(struct vsock_sock *vsk,
spin_lock_bh(&vvs->rx_lock);
while (total < len && !skb_queue_empty(&vvs->rx_queue)) {
- skb = __skb_dequeue(&vvs->rx_queue);
+ skb = skb_peek(&vvs->rx_queue);
bytes = len - total;
if (bytes > skb->len)
@@ -388,9 +388,8 @@ virtio_transport_stream_do_dequeue(struct vsock_sock *vsk,
u32 pkt_len = le32_to_cpu(virtio_vsock_hdr(skb)->len);
virtio_transport_dec_rx_pkt(vvs, pkt_len);
+ __skb_unlink(skb, &vvs->rx_queue);
consume_skb(skb);
- } else {
- __skb_queue_head(&vvs->rx_queue, skb);
}
}
--
2.25.1
This adds SOCK_STREAM and SOCK_SEQPACKET tests for invalid buffer case.
It tries to read data to NULL buffer (data already presents in socket's
queue), then uses valid buffer. For SOCK_STREAM second read must return
data, because skbuff is not dropped, but for SOCK_SEQPACKET skbuff will
be dropped by kernel, and 'recv()' will return EAGAIN.
Reviewed-by: Stefano Garzarella <[email protected]>
Signed-off-by: Arseniy Krasnov <[email protected]>
---
tools/testing/vsock/vsock_test.c | 118 +++++++++++++++++++++++++++++++
1 file changed, 118 insertions(+)
diff --git a/tools/testing/vsock/vsock_test.c b/tools/testing/vsock/vsock_test.c
index 67e9f9df3a8c..3de10dbb50f5 100644
--- a/tools/testing/vsock/vsock_test.c
+++ b/tools/testing/vsock/vsock_test.c
@@ -860,6 +860,114 @@ static void test_stream_poll_rcvlowat_client(const struct test_opts *opts)
close(fd);
}
+#define INV_BUF_TEST_DATA_LEN 512
+
+static void test_inv_buf_client(const struct test_opts *opts, bool stream)
+{
+ unsigned char data[INV_BUF_TEST_DATA_LEN] = {0};
+ ssize_t ret;
+ int fd;
+
+ if (stream)
+ fd = vsock_stream_connect(opts->peer_cid, 1234);
+ else
+ fd = vsock_seqpacket_connect(opts->peer_cid, 1234);
+
+ if (fd < 0) {
+ perror("connect");
+ exit(EXIT_FAILURE);
+ }
+
+ control_expectln("SENDDONE");
+
+ /* Use invalid buffer here. */
+ ret = recv(fd, NULL, sizeof(data), 0);
+ if (ret != -1) {
+ fprintf(stderr, "expected recv(2) failure, got %zi\n", ret);
+ exit(EXIT_FAILURE);
+ }
+
+ if (errno != ENOMEM) {
+ fprintf(stderr, "unexpected recv(2) errno %d\n", errno);
+ exit(EXIT_FAILURE);
+ }
+
+ ret = recv(fd, data, sizeof(data), MSG_DONTWAIT);
+
+ if (stream) {
+ /* For SOCK_STREAM we must continue reading. */
+ if (ret != sizeof(data)) {
+ fprintf(stderr, "expected recv(2) success, got %zi\n", ret);
+ exit(EXIT_FAILURE);
+ }
+ /* Don't check errno in case of success. */
+ } else {
+ /* For SOCK_SEQPACKET socket's queue must be empty. */
+ if (ret != -1) {
+ fprintf(stderr, "expected recv(2) failure, got %zi\n", ret);
+ exit(EXIT_FAILURE);
+ }
+
+ if (errno != EAGAIN) {
+ fprintf(stderr, "unexpected recv(2) errno %d\n", errno);
+ exit(EXIT_FAILURE);
+ }
+ }
+
+ control_writeln("DONE");
+
+ close(fd);
+}
+
+static void test_inv_buf_server(const struct test_opts *opts, bool stream)
+{
+ unsigned char data[INV_BUF_TEST_DATA_LEN] = {0};
+ ssize_t res;
+ int fd;
+
+ if (stream)
+ fd = vsock_stream_accept(VMADDR_CID_ANY, 1234, NULL);
+ else
+ fd = vsock_seqpacket_accept(VMADDR_CID_ANY, 1234, NULL);
+
+ if (fd < 0) {
+ perror("accept");
+ exit(EXIT_FAILURE);
+ }
+
+ res = send(fd, data, sizeof(data), 0);
+ if (res != sizeof(data)) {
+ fprintf(stderr, "unexpected send(2) result %zi\n", res);
+ exit(EXIT_FAILURE);
+ }
+
+ control_writeln("SENDDONE");
+
+ control_expectln("DONE");
+
+ close(fd);
+}
+
+static void test_stream_inv_buf_client(const struct test_opts *opts)
+{
+ test_inv_buf_client(opts, true);
+}
+
+static void test_stream_inv_buf_server(const struct test_opts *opts)
+{
+ test_inv_buf_server(opts, true);
+}
+
+static void test_seqpacket_inv_buf_client(const struct test_opts *opts)
+{
+ test_inv_buf_client(opts, false);
+}
+
+static void test_seqpacket_inv_buf_server(const struct test_opts *opts)
+{
+ test_inv_buf_server(opts, false);
+}
+
static struct test_case test_cases[] = {
{
.name = "SOCK_STREAM connection reset",
@@ -920,6 +1028,16 @@ static struct test_case test_cases[] = {
.run_client = test_seqpacket_bigmsg_client,
.run_server = test_seqpacket_bigmsg_server,
},
+ {
+ .name = "SOCK_STREAM test invalid buffer",
+ .run_client = test_stream_inv_buf_client,
+ .run_server = test_stream_inv_buf_server,
+ },
+ {
+ .name = "SOCK_SEQPACKET test invalid buffer",
+ .run_client = test_seqpacket_inv_buf_client,
+ .run_server = test_seqpacket_inv_buf_server,
+ },
{},
};
--
2.25.1
On Thu, Mar 09, 2023 at 11:27:02PM +0300, Arseniy Krasnov wrote:
>Since we now no longer use 'skb->len' to update credit, there is no sense
>to update skbuff state, because it is used only once after dequeue to
>copy data and then will be released.
>
>Fixes: 71dc9ec9ac7d ("virtio/vsock: replace virtio_vsock_pkt with sk_buff")
>Signed-off-by: Arseniy Krasnov <[email protected]>
>---
> net/vmw_vsock/virtio_transport_common.c | 1 -
> 1 file changed, 1 deletion(-)
Reviewed-by: Stefano Garzarella <[email protected]>
>
>diff --git a/net/vmw_vsock/virtio_transport_common.c b/net/vmw_vsock/virtio_transport_common.c
>index 618680fd9906..9a411475e201 100644
>--- a/net/vmw_vsock/virtio_transport_common.c
>+++ b/net/vmw_vsock/virtio_transport_common.c
>@@ -465,7 +465,6 @@ static int virtio_transport_seqpacket_do_dequeue(struct vsock_sock *vsk,
> dequeued_len = err;
> } else {
> user_buf_len -= bytes_to_copy;
>- skb_pull(skb, bytes_to_copy);
> }
>
> spin_lock_bh(&vvs->rx_lock);
>--
>2.25.1
>
On Thu, Mar 09, 2023 at 11:28:01PM +0300, Arseniy Krasnov wrote:
>This returns behaviour of SOCK_STREAM read as before skbuff usage. When
>copying to user fails current skbuff won't be dropped, but returned to
>sockets's queue. Technically instead of 'skb_dequeue()', 'skb_peek()' is
>called and when skbuff becomes empty, it is removed from queue by
>'__skb_unlink()'.
>
>Fixes: 71dc9ec9ac7d ("virtio/vsock: replace virtio_vsock_pkt with sk_buff")
>Signed-off-by: Arseniy Krasnov <[email protected]>
>---
> net/vmw_vsock/virtio_transport_common.c | 5 ++---
> 1 file changed, 2 insertions(+), 3 deletions(-)
Reviewed-by: Stefano Garzarella <[email protected]>
>
>diff --git a/net/vmw_vsock/virtio_transport_common.c b/net/vmw_vsock/virtio_transport_common.c
>index 9a411475e201..6564192e7f20 100644
>--- a/net/vmw_vsock/virtio_transport_common.c
>+++ b/net/vmw_vsock/virtio_transport_common.c
>@@ -364,7 +364,7 @@ virtio_transport_stream_do_dequeue(struct vsock_sock *vsk,
>
> spin_lock_bh(&vvs->rx_lock);
> while (total < len && !skb_queue_empty(&vvs->rx_queue)) {
>- skb = __skb_dequeue(&vvs->rx_queue);
>+ skb = skb_peek(&vvs->rx_queue);
>
> bytes = len - total;
> if (bytes > skb->len)
>@@ -388,9 +388,8 @@ virtio_transport_stream_do_dequeue(struct vsock_sock *vsk,
> u32 pkt_len = le32_to_cpu(virtio_vsock_hdr(skb)->len);
>
> virtio_transport_dec_rx_pkt(vvs, pkt_len);
>+ __skb_unlink(skb, &vvs->rx_queue);
> consume_skb(skb);
>- } else {
>- __skb_queue_head(&vvs->rx_queue, skb);
> }
> }
>
>--
>2.25.1
>
Hi Arseniy,
On Thu, Mar 09, 2023 at 11:24:42PM +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 update
> 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.
thanks for the fixes, I would wait a few days to see if there are any
comments and then I think you can send it on net without RFC.
@Bobby if you can take a look, your ack would be appreciated :-)
Thanks,
Stefano
On Thu, Mar 09, 2023 at 11:26:00PM +0300, Arseniy Krasnov wrote:
>'skb->len' can vary when we partially read the data, this complicates the
>calculation of credit to be updated in 'virtio_transport_inc_rx_pkt()/
>virtio_transport_dec_rx_pkt()'.
>
>Also in 'virtio_transport_dec_rx_pkt()' we were miscalculating the
>credit since 'skb->len' was redundant.
>
>For these reasons, let's replace the use of skbuff state to calculate new
>'rx_bytes'/'fwd_cnt' values with explicit value as input argument. This
>makes code more simple, because it is not needed to change skbuff state
>before each call to update 'rx_bytes'/'fwd_cnt'.
>
>Fixes: 71dc9ec9ac7d ("virtio/vsock: replace virtio_vsock_pkt with sk_buff")
>Signed-off-by: Arseniy Krasnov <[email protected]>
>---
> net/vmw_vsock/virtio_transport_common.c | 23 +++++++++++------------
> 1 file changed, 11 insertions(+), 12 deletions(-)
Reviewed-by: Stefano Garzarella <[email protected]>
>
>diff --git a/net/vmw_vsock/virtio_transport_common.c b/net/vmw_vsock/virtio_transport_common.c
>index a1581c77cf84..618680fd9906 100644
>--- a/net/vmw_vsock/virtio_transport_common.c
>+++ b/net/vmw_vsock/virtio_transport_common.c
>@@ -241,21 +241,18 @@ static int virtio_transport_send_pkt_info(struct vsock_sock *vsk,
> }
>
> static bool virtio_transport_inc_rx_pkt(struct virtio_vsock_sock *vvs,
>- struct sk_buff *skb)
>+ u32 len)
> {
>- if (vvs->rx_bytes + skb->len > vvs->buf_alloc)
>+ if (vvs->rx_bytes + len > vvs->buf_alloc)
> return false;
>
>- vvs->rx_bytes += skb->len;
>+ vvs->rx_bytes += len;
> return true;
> }
>
> static void virtio_transport_dec_rx_pkt(struct virtio_vsock_sock *vvs,
>- struct sk_buff *skb)
>+ u32 len)
> {
>- int len;
>-
>- len = skb_headroom(skb) - sizeof(struct virtio_vsock_hdr) - skb->len;
> vvs->rx_bytes -= len;
> vvs->fwd_cnt += len;
> }
>@@ -388,7 +385,9 @@ virtio_transport_stream_do_dequeue(struct vsock_sock *vsk,
> skb_pull(skb, bytes);
>
> if (skb->len == 0) {
>- virtio_transport_dec_rx_pkt(vvs, skb);
>+ u32 pkt_len = le32_to_cpu(virtio_vsock_hdr(skb)->len);
>+
>+ virtio_transport_dec_rx_pkt(vvs, pkt_len);
> consume_skb(skb);
> } else {
> __skb_queue_head(&vvs->rx_queue, skb);
>@@ -437,17 +436,17 @@ static int virtio_transport_seqpacket_do_dequeue(struct vsock_sock *vsk,
>
> while (!msg_ready) {
> struct virtio_vsock_hdr *hdr;
>+ size_t pkt_len;
>
> skb = __skb_dequeue(&vvs->rx_queue);
> if (!skb)
> break;
> hdr = virtio_vsock_hdr(skb);
>+ pkt_len = (size_t)le32_to_cpu(hdr->len);
>
> if (dequeued_len >= 0) {
>- size_t pkt_len;
> size_t bytes_to_copy;
>
>- pkt_len = (size_t)le32_to_cpu(hdr->len);
> bytes_to_copy = min(user_buf_len, pkt_len);
>
> if (bytes_to_copy) {
>@@ -484,7 +483,7 @@ static int virtio_transport_seqpacket_do_dequeue(struct vsock_sock *vsk,
> msg->msg_flags |= MSG_EOR;
> }
>
>- virtio_transport_dec_rx_pkt(vvs, skb);
>+ virtio_transport_dec_rx_pkt(vvs, pkt_len);
> kfree_skb(skb);
> }
>
>@@ -1040,7 +1039,7 @@ virtio_transport_recv_enqueue(struct vsock_sock *vsk,
>
> spin_lock_bh(&vvs->rx_lock);
>
>- can_enqueue = virtio_transport_inc_rx_pkt(vvs, skb);
>+ can_enqueue = virtio_transport_inc_rx_pkt(vvs, len);
> if (!can_enqueue) {
> free_pkt = true;
> goto out;
>--
>2.25.1
>
On 10.03.2023 12:09, Stefano Garzarella wrote:
> Hi Arseniy,
>
> On Thu, Mar 09, 2023 at 11:24:42PM +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 update
>> 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.
>
> thanks for the fixes, I would wait a few days to see if there are any
> comments and then I think you can send it on net without RFC.
>
> @Bobby if you can take a look, your ack would be appreciated :-)
Ok, thanks for review. I'll wait for several days and also wait until
net-next will be opened. Then i'll resend this patchset with net-next
Thanks, Arseniy
>
> Thanks,
> Stefano
>
On Fri, Mar 10, 2023 at 12:42:13PM +0300, Arseniy Krasnov wrote:
>
>
>On 10.03.2023 12:09, Stefano Garzarella wrote:
>> Hi Arseniy,
>>
>> On Thu, Mar 09, 2023 at 11:24:42PM +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 update
>>> ? 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.
>>
>> thanks for the fixes, I would wait a few days to see if there are any
>> comments and then I think you can send it on net without RFC.
>>
>> @Bobby if you can take a look, your ack would be appreciated :-)
>Ok, thanks for review. I'll wait for several days and also wait until
>net-next will be opened. Then i'll resend this patchset with net-next
Since they are fixes, they should go with the net tree, not net-next.
Cheers,
Stefano
On 10.03.2023 14:40, Stefano Garzarella wrote:
> On Fri, Mar 10, 2023 at 12:42:13PM +0300, Arseniy Krasnov wrote:
>>
>>
>> On 10.03.2023 12:09, Stefano Garzarella wrote:
>>> Hi Arseniy,
>>>
>>> On Thu, Mar 09, 2023 at 11:24:42PM +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 update
>>>> 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.
>>>
>>> thanks for the fixes, I would wait a few days to see if there are any
>>> comments and then I think you can send it on net without RFC.
>>>
>>> @Bobby if you can take a look, your ack would be appreciated :-)
>> Ok, thanks for review. I'll wait for several days and also wait until
>> net-next will be opened. Then i'll resend this patchset with net-next
>
> Since they are fixes, they should go with the net tree, not net-next.
Ah ok, for net tree i can send it no matter that net-next is closed.
Thanks, Arseniy
>
> Cheers,
> Stefano
>