Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) client-ip=2620:137:e000::1:20;
Message-ID: <2b581879-f449-4ba3-8fbd-397d0f23ae15@sberdevices.ru>
Date:   Tue, 1 Aug 2023 16:28:53 +0300
MIME-Version: 1.0
User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:102.0) Gecko/20100101
 Thunderbird/102.7.1
Subject: Re: [PATCH net-next v5 1/4] vsock/virtio/vhost: read data from
 non-linear skb
Content-Language: en-US
To:     Paolo Abeni <pabeni@redhat.com>,
        Stefan Hajnoczi <stefanha@redhat.com>,
        Stefano Garzarella <sgarzare@redhat.com>,
        "David S. Miller" <davem@davemloft.net>,
        Eric Dumazet <edumazet@google.com>,
        Jakub Kicinski <kuba@kernel.org>,
        "Michael S. Tsirkin" <mst@redhat.com>,
        Jason Wang <jasowang@redhat.com>,
        Bobby Eshleman <bobby.eshleman@bytedance.com>
CC:     <kvm@vger.kernel.org>, <virtualization@lists.linux-foundation.org>,
        <netdev@vger.kernel.org>, <linux-kernel@vger.kernel.org>,
        <kernel@sberdevices.ru>, <oxffffaa@gmail.com>
References: <20230730085905.3420811-1-AVKrasnov@sberdevices.ru>
 <20230730085905.3420811-2-AVKrasnov@sberdevices.ru>
 <8972ac7df2298d47e1b2f53b7f1b5d5941999580.camel@redhat.com>
From:   Arseniy Krasnov <avkrasnov@sberdevices.ru>
In-Reply-To: <8972ac7df2298d47e1b2f53b7f1b5d5941999580.camel@redhat.com>
Content-Type: text/plain; charset="UTF-8"
Content-Transfer-Encoding: 7bit
Precedence: bulk


On 01.08.2023 16:11, Paolo Abeni wrote:
> On Sun, 2023-07-30 at 11:59 +0300, Arseniy Krasnov wrote:
>> This is preparation patch for MSG_ZEROCOPY support. It adds handling of
>> non-linear skbs by replacing direct calls of 'memcpy_to_msg()' with
>> 'skb_copy_datagram_iter()'. Main advantage of the second one is that it
>> can handle paged part of the skb by using 'kmap()' on each page, but if
>> there are no pages in the skb, it behaves like simple copying to iov
>> iterator. This patch also adds new field to the control block of skb -
>> this value shows current offset in the skb to read next portion of data
>> (it doesn't matter linear it or not). Idea behind this field is that
>> 'skb_copy_datagram_iter()' handles both types of skb internally - it
>> just needs an offset from which to copy data from the given skb. This
>> offset is incremented on each read from skb. This approach allows to
>> avoid special handling of non-linear skbs:
>> 1) We can't call 'skb_pull()' on it, because it updates 'data' pointer.
>> 2) We need to update 'data_len' also on each read from this skb.
> 
> It looks like the above sentence is a left-over from previous version
> as, as this patch does not touch data_len. And I think it contradicts
> the previous one, so it's a bit confusing.

Yes, seems I need to rephrase it in the next version. I meant that with
approach introduced in this patch we don't need to check that skb is
linear of non-linear after reading data from it. Because otherwise:
1) In case of linear skb we will need to call 'skb_pull()' after reading
   data, to update 'data' pointer.
2) In case of non-linear skb we will need to update 'data_len' field after
   reading data, as this field shows amount of data in fragged part.

> 
>> Signed-off-by: Arseniy Krasnov <AVKrasnov@sberdevices.ru>
>> ---
>>  Changelog:
>>  v5(big patchset) -> v1:
>>   * Merge 'virtio_transport_common.c' and 'vhost/vsock.c' patches into
>>     this single patch.
>>   * Commit message update: grammar fix and remark that this patch is
>>     MSG_ZEROCOPY preparation.
>>   * Use 'min_t()' instead of comparison using '<>' operators.
>>  v1 -> v2:
>>   * R-b tag added.
>>  v3 -> v4:
>>   * R-b tag removed due to rebase:
>>     * Part for 'virtio_transport_stream_do_peek()' is changed.
>>     * Part for 'virtio_transport_seqpacket_do_peek()' is added.
>>   * Comments about sleep in 'memcpy_to_msg()' now describe sleep in
>>     'skb_copy_datagram_iter()'.
>>
>>  drivers/vhost/vsock.c                   | 14 +++++++----
>>  include/linux/virtio_vsock.h            |  1 +
>>  net/vmw_vsock/virtio_transport_common.c | 32 +++++++++++++++----------
>>  3 files changed, 29 insertions(+), 18 deletions(-)
>>
>> diff --git a/drivers/vhost/vsock.c b/drivers/vhost/vsock.c
>> index 817d377a3f36..8c917be32b5d 100644
>> --- a/drivers/vhost/vsock.c
>> +++ b/drivers/vhost/vsock.c
>> @@ -114,6 +114,7 @@ vhost_transport_do_send_pkt(struct vhost_vsock *vsock,
>>  		struct sk_buff *skb;
>>  		unsigned out, in;
>>  		size_t nbytes;
>> +		u32 frag_off;
> 
> IMHO 'offset' would be a better name for both the variable and the CB
> field, as it can points both inside the skb frags, linear part or frag
> list.

Ack

> 
> Otherwise LGTM, thanks!
> 
> Paolo
> 

Thanks, Arseniy