Re: [RFC PATCH v1 07/12] vsock/virtio: MGS_ZEROCOPY flag support

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



On Mon, Feb 06, 2023 at 07:00:35AM +0000, Arseniy Krasnov wrote:
This adds main logic of MSG_ZEROCOPY flag processing for packet
creation. When this flag is set and user's iov iterator fits for
zerocopy transmission, call 'get_user_pages()' and add returned
pages to the newly created skb.

Signed-off-by: Arseniy Krasnov <AVKrasnov@xxxxxxxxxxxxxx>
---
net/vmw_vsock/virtio_transport_common.c | 212 ++++++++++++++++++++++--
1 file changed, 195 insertions(+), 17 deletions(-)

diff --git a/net/vmw_vsock/virtio_transport_common.c b/net/vmw_vsock/virtio_transport_common.c
index 05ce97b967ad..69e37f8a68a6 100644
--- a/net/vmw_vsock/virtio_transport_common.c
+++ b/net/vmw_vsock/virtio_transport_common.c
@@ -37,6 +37,169 @@ virtio_transport_get_ops(struct vsock_sock *vsk)
	return container_of(t, struct virtio_transport, transport);
}


I'd use bool if we don't need to return an error value in the following
new functions.

+static int virtio_transport_can_zcopy(struct iov_iter *iov_iter,
+				      size_t free_space)
+{
+	size_t pages;
+	int i;
+
+	if (!iter_is_iovec(iov_iter))
+		return -1;
+
+	if (iov_iter->iov_offset)
+		return -1;
+
+	/* We can't send whole iov. */
+	if (free_space < iov_iter->count)
+		return -1;
+
+	for (pages = 0, i = 0; i < iov_iter->nr_segs; i++) {
+		const struct iovec *iovec;
+		int pages_in_elem;
+
+		iovec = &iov_iter->iov[i];
+
+		/* Base must be page aligned. */
+		if (offset_in_page(iovec->iov_base))
+			return -1;
+
+		/* Only last element could have not page aligned size.  */
+		if (i != (iov_iter->nr_segs - 1)) {
+			if (offset_in_page(iovec->iov_len))
+				return -1;
+
+			pages_in_elem = iovec->iov_len >> PAGE_SHIFT;
+		} else {
+			pages_in_elem = round_up(iovec->iov_len, PAGE_SIZE);
+			pages_in_elem >>= PAGE_SHIFT;
+		}
+
+		/* In case of user's pages - one page is one frag. */
+		if (pages + pages_in_elem > MAX_SKB_FRAGS)
+			return -1;
+
+		pages += pages_in_elem;
+	}
+
+	return 0;
+}
+
+static int virtio_transport_init_zcopy_skb(struct vsock_sock *vsk,
+					   struct sk_buff *skb,
+					   struct iov_iter *iter,
+					   bool zerocopy)
+{
+	struct ubuf_info_msgzc *uarg_zc;
+	struct ubuf_info *uarg;
+
+	uarg = msg_zerocopy_realloc(sk_vsock(vsk),
+				    iov_length(iter->iov, iter->nr_segs),
+				    NULL);
+
+	if (!uarg)
+		return -1;
+
+	uarg_zc = uarg_to_msgzc(uarg);
+	uarg_zc->zerocopy = zerocopy ? 1 : 0;
+
+	skb_zcopy_init(skb, uarg);
+
+	return 0;
+}
+
+static int virtio_transport_fill_nonlinear_skb(struct sk_buff *skb,
+					       struct vsock_sock *vsk,
+					       struct virtio_vsock_pkt_info *info)
+{
+	struct iov_iter *iter;
+	int frag_idx;
+	int seg_idx;
+
+	iter = &info->msg->msg_iter;
+	frag_idx = 0;
+	VIRTIO_VSOCK_SKB_CB(skb)->curr_frag = 0;
+	VIRTIO_VSOCK_SKB_CB(skb)->frag_off = 0;
+
+	/* At this moment:
+	 * 1) 'iov_offset' is zero.
+	 * 2) Every 'iov_base' and 'iov_len' are also page aligned
+	 *    (except length of the last element).
+	 * 3) Number of pages in this iov <= MAX_SKB_FRAGS.
+	 * 4) Length of the data fits in current credit space.
+	 */
+	for (seg_idx = 0; seg_idx < iter->nr_segs; seg_idx++) {
+		struct page *user_pages[MAX_SKB_FRAGS];
+		const struct iovec *iovec;
+		size_t last_frag_len;
+		size_t pages_in_seg;
+		int page_idx;
+
+		iovec = &iter->iov[seg_idx];
+		pages_in_seg = iovec->iov_len >> PAGE_SHIFT;
+
+		if (iovec->iov_len % PAGE_SIZE) {
+			last_frag_len = iovec->iov_len % PAGE_SIZE;
+			pages_in_seg++;
+		} else {
+			last_frag_len = PAGE_SIZE;
+		}
+
+		if (get_user_pages((unsigned long)iovec->iov_base,
+				   pages_in_seg, FOLL_GET, user_pages,
+				   NULL) != pages_in_seg)
+			return -1;

Reading the get_user_pages() documentation, this should pin the user
pages, so we should be fine if we then expose them in the virtqueue.

But reading Documentation/core-api/pin_user_pages.rst it seems that
drivers should use "pin_user_pages*() for DMA-pinned pages", so I'm not
sure what we should do.

Additional advice would be great!

Anyway, when we are done using the pages, we should call put_page() or
unpin_user_page() depending on how we pin them.

+
+		for (page_idx = 0; page_idx < pages_in_seg; page_idx++) {
+			int frag_len = PAGE_SIZE;
+
+			if (page_idx == (pages_in_seg - 1))
+				frag_len = last_frag_len;
+
+			skb_fill_page_desc(skb, frag_idx++,
+					   user_pages[page_idx], 0,
+					   frag_len);
+			skb_len_add(skb, frag_len);
+		}
+	}
+
+	return virtio_transport_init_zcopy_skb(vsk, skb, iter, true);
+}
+
+static int virtio_transport_copy_payload(struct sk_buff *skb,
+					 struct vsock_sock *vsk,
+					 struct virtio_vsock_pkt_info *info,
+					 size_t len)
+{
+	void *payload;
+	int err;
+
+	payload = skb_put(skb, len);
+	err = memcpy_from_msg(payload, info->msg, len);
+	if (err)
+		return -1;
+
+	if (msg_data_left(info->msg))
+		return 0;
+
+	if (info->type == VIRTIO_VSOCK_TYPE_SEQPACKET) {
+		struct virtio_vsock_hdr *hdr;
+
+		hdr = virtio_vsock_hdr(skb);
+
+		hdr->flags |= cpu_to_le32(VIRTIO_VSOCK_SEQ_EOM);
+
+		if (info->msg->msg_flags & MSG_EOR)
+			hdr->flags |= cpu_to_le32(VIRTIO_VSOCK_SEQ_EOR);
+	}
+

A comment here explaining why this is necessary would be helpful.

+	if (info->flags & MSG_ZEROCOPY)
+		return virtio_transport_init_zcopy_skb(vsk, skb,
+						       &info->msg->msg_iter,
+						       false);
+
+	return 0;
+}
+
/* Returns a new packet on success, otherwise returns NULL.
 *
 * If NULL is returned, errp is set to a negative errno.
@@ -47,15 +210,31 @@ virtio_transport_alloc_skb(struct virtio_vsock_pkt_info *info,
			   u32 src_cid,
			   u32 src_port,
			   u32 dst_cid,
-			   u32 dst_port)
+			   u32 dst_port,
+			   struct vsock_sock *vsk)
{
-	const size_t skb_len = VIRTIO_VSOCK_SKB_HEADROOM + len;
+	const size_t skb_len = VIRTIO_VSOCK_SKB_HEADROOM;
	struct virtio_vsock_hdr *hdr;
	struct sk_buff *skb;
-	void *payload;
-	int err;
+	bool use_zcopy = false;
+
+	if (info->msg) {
+		/* If SOCK_ZEROCOPY is not enabled, ignore MSG_ZEROCOPY
+		 * flag later and continue in classic way(e.g. without
+		 * completion).
+		 */
+		if (!sock_flag(sk_vsock(vsk), SOCK_ZEROCOPY)) {

`vsk` can be null, should we check it?
Otherwise, what about passing only a flag?
So the caller will check it.

+			info->flags &= ~MSG_ZEROCOPY;
+		} else {
+			if ((info->flags & MSG_ZEROCOPY) &&
+			    !virtio_transport_can_zcopy(&info->msg->msg_iter, len)) {

This part is not very clear, I think virtio_transport_can_zcopy()
should return `true` if "can_zcopy".

+				use_zcopy = true;
+			}
+		}
+	}

-	skb = virtio_vsock_alloc_skb(skb_len, GFP_KERNEL);
+	/* For MSG_ZEROCOPY length will be added later. */
+	skb = virtio_vsock_alloc_skb(skb_len + (use_zcopy ? 0 : len), GFP_KERNEL);

I think is better to adsjut `skb_len` in the previous block, when we set
`use_zcopy = true`, we can do `skb_len -= len` (with the comment);

	if (!skb)
		return NULL;

@@ -70,18 +249,15 @@ virtio_transport_alloc_skb(struct virtio_vsock_pkt_info *info,
	hdr->len	= cpu_to_le32(len);

	if (info->msg && len > 0) {
-		payload = skb_put(skb, len);
-		err = memcpy_from_msg(payload, info->msg, len);
-		if (err)
-			goto out;
+		int err;

-		if (msg_data_left(info->msg) == 0 &&
-		    info->type == VIRTIO_VSOCK_TYPE_SEQPACKET) {
-			hdr->flags |= cpu_to_le32(VIRTIO_VSOCK_SEQ_EOM);
+		if (use_zcopy)
+			err = virtio_transport_fill_nonlinear_skb(skb, vsk, info);
+		else
+			err = virtio_transport_copy_payload(skb, vsk, info, len);

-			if (info->msg->msg_flags & MSG_EOR)
-				hdr->flags |= cpu_to_le32(VIRTIO_VSOCK_SEQ_EOR);
-		}
+		if (err)
+			goto out;
	}

	if (info->reply)
@@ -266,7 +442,8 @@ static int virtio_transport_send_pkt_info(struct vsock_sock *vsk,

	skb = virtio_transport_alloc_skb(info, pkt_len,
					 src_cid, src_port,
-					 dst_cid, dst_port);
+					 dst_cid, dst_port,
+					 vsk);
	if (!skb) {
		virtio_transport_put_credit(vvs, pkt_len);
		return -ENOMEM;
@@ -842,6 +1019,7 @@ virtio_transport_stream_enqueue(struct vsock_sock *vsk,
		.msg = msg,
		.pkt_len = len,
		.vsk = vsk,
+		.flags = msg->msg_flags,
	};

	return virtio_transport_send_pkt_info(vsk, &info);
@@ -894,7 +1072,7 @@ static int virtio_transport_reset_no_sock(const struct virtio_transport *t,
					   le64_to_cpu(hdr->dst_cid),
					   le32_to_cpu(hdr->dst_port),
					   le64_to_cpu(hdr->src_cid),
-					   le32_to_cpu(hdr->src_port));
+					   le32_to_cpu(hdr->src_port), NULL);
	if (!reply)
		return -ENOMEM;

--
2.25.1




[Index of Archives]     [KVM ARM]     [KVM ia64]     [KVM ppc]     [Virtualization Tools]     [Spice Development]     [Libvirt]     [Libvirt Users]     [Linux USB Devel]     [Linux Audio Users]     [Yosemite Questions]     [Linux Kernel]     [Linux SCSI]     [XFree86]

  Powered by Linux