Date:   Thu, 9 Mar 2023 17:21:50 +0100
From:   Stefano Garzarella <sgarzare@redhat.com>
To:     Arseniy Krasnov <avkrasnov@sberdevices.ru>
Cc:     Stefan Hajnoczi <stefanha@redhat.com>,
        "David S. Miller" <davem@davemloft.net>,
        Eric Dumazet <edumazet@google.com>,
        Jakub Kicinski <kuba@kernel.org>,
        Paolo Abeni <pabeni@redhat.com>,
        Bobby Eshleman <bobby.eshleman@bytedance.com>,
        kvm@vger.kernel.org, virtualization@lists.linux-foundation.org,
        netdev@vger.kernel.org, linux-kernel@vger.kernel.org,
        kernel@sberdevices.ru, oxffffaa@gmail.com
Subject: Re: [RFC PATCH v3 0/4] several updates to virtio/vsock
Message-ID: <20230309162150.qqrlqmqghi5muucx@sgarzare-redhat>
References: <0abeec42-a11d-3a51-453b-6acf76604f2e@sberdevices.ru>
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii; format=flowed
Content-Disposition: inline
In-Reply-To: <0abeec42-a11d-3a51-453b-6acf76604f2e@sberdevices.ru>
Precedence: bulk

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@sberdevices.ru/
>
>Link to v2 on lore:
>https://lore.kernel.org/netdev/a7ab414b-5e41-c7b6-250b-e8401f335859@sberdevices.ru/
>
>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.

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
>