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@xxxxxxxxxxxxxx> >> --- >> 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
