Paolo Abeni
2023-Aug-01 13:11 UTC
[PATCH net-next v5 1/4] vsock/virtio/vhost: read data from non-linear skb
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.> Signed-off-by: Arseniy Krasnov <AVKrasnov at 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. Otherwise LGTM, thanks! Paolo