[net-next 0/6] Reduce code duplication of zerocopy of tun and macvtap

8 messages, 2 authors, 2013-08-07 · open the first message on its own page

[net-next 0/6] Reduce code duplication of zerocopy of tun and macvtap

From: Jason Wang <hidden>
Date: 2013-08-06 09:56:23

Hi all:

This series tries to reduce the code duplication of zerocopy of tun and macvtap
by:

- Move common helpers iov_pages() and zerocopy_sg_from_iovec() to net/core
- Use exist helpers instead of open coding the new one

With this seires, we can do further optimizations on top.

Please review.

Jason Wang (6):
  net: move iov_pages() to net/core/iovec.c
  net: move zerocopy_sg_from_iovec() to net/core/datagram.c
  net: use skb_fill_page_desc() in zerocopy_sg_from_iovec()
  net: remove the useless comment in zerocopy_sg_from_iovec()
  net: use release_pages() in zerocopy_sg_from_iovec()
  net: use skb_copy_datagram_from_iovec() in zerocopy_sg_from_iovec()

 drivers/net/macvtap.c  |  103 ------------------------------------------------
 drivers/net/tun.c      |  103 ------------------------------------------------
 include/linux/skbuff.h |    4 ++
 include/linux/socket.h |    2 +
 net/core/datagram.c    |   72 +++++++++++++++++++++++++++++++++
 net/core/iovec.c       |   24 +++++++++++
 6 files changed, 102 insertions(+), 206 deletions(-)

[net-next 1/6] net: move iov_pages() to net/core/iovec.c

From: Jason Wang <hidden>
Date: 2013-08-06 09:56:27

To let it be reused and reduce code duplication.

Signed-off-by: Jason Wang <redacted>
---
 drivers/net/macvtap.c  |   23 -----------------------
 drivers/net/tun.c      |   23 -----------------------
 include/linux/socket.h |    2 ++
 net/core/iovec.c       |   24 ++++++++++++++++++++++++
 4 files changed, 26 insertions(+), 46 deletions(-)

diff --git a/drivers/net/macvtap.c b/drivers/net/macvtap.c
index a98fb0e..dfec20d 100644
--- a/drivers/net/macvtap.c
+++ b/drivers/net/macvtap.c

@@ -698,29 +698,6 @@ static int macvtap_skb_to_vnet_hdr(const struct sk_buff *skb,
 	return 0;
 }
 
-static unsigned long iov_pages(const struct iovec *iv, int offset,
-			       unsigned long nr_segs)
-{
-	unsigned long seg, base;
-	int pages = 0, len, size;
-
-	while (nr_segs && (offset >= iv->iov_len)) {
-		offset -= iv->iov_len;
-		++iv;
-		--nr_segs;
-	}
-
-	for (seg = 0; seg < nr_segs; seg++) {
-		base = (unsigned long)iv[seg].iov_base + offset;
-		len = iv[seg].iov_len - offset;
-		size = ((base & ~PAGE_MASK) + len + ~PAGE_MASK) >> PAGE_SHIFT;
-		pages += size;
-		offset = 0;
-	}
-
-	return pages;
-}
-
 /* Get packet from user space buffer */
 static ssize_t macvtap_get_user(struct macvtap_queue *q, struct msghdr *m,
 				const struct iovec *iv, unsigned long total_len,

diff --git a/drivers/net/tun.c b/drivers/net/tun.c
index 5dce262..18527ef 100644
--- a/drivers/net/tun.c
+++ b/drivers/net/tun.c

@@ -1041,29 +1041,6 @@ static int zerocopy_sg_from_iovec(struct sk_buff *skb, const struct iovec *from,
 	return 0;
 }
 
-static unsigned long iov_pages(const struct iovec *iv, int offset,
-			       unsigned long nr_segs)
-{
-	unsigned long seg, base;
-	int pages = 0, len, size;
-
-	while (nr_segs && (offset >= iv->iov_len)) {
-		offset -= iv->iov_len;
-		++iv;
-		--nr_segs;
-	}
-
-	for (seg = 0; seg < nr_segs; seg++) {
-		base = (unsigned long)iv[seg].iov_base + offset;
-		len = iv[seg].iov_len - offset;
-		size = ((base & ~PAGE_MASK) + len + ~PAGE_MASK) >> PAGE_SHIFT;
-		pages += size;
-		offset = 0;
-	}
-
-	return pages;
-}
-
 /* Get packet from user space buffer */
 static ssize_t tun_get_user(struct tun_struct *tun, struct tun_file *tfile,
 			    void *msg_control, const struct iovec *iv,

diff --git a/include/linux/socket.h b/include/linux/socket.h
index 230c04b..445ef75 100644
--- a/include/linux/socket.h
+++ b/include/linux/socket.h

@@ -313,6 +313,8 @@ extern int csum_partial_copy_fromiovecend(unsigned char *kdata,
 					  struct iovec *iov, 
 					  int offset, 
 					  unsigned int len, __wsum *csump);
+extern unsigned long iov_pages(const struct iovec *iov, int offset,
+			       unsigned long nr_segs);
 
 extern int verify_iovec(struct msghdr *m, struct iovec *iov, struct sockaddr_storage *address, int mode);
 extern int memcpy_toiovecend(const struct iovec *v, unsigned char *kdata,

diff --git a/net/core/iovec.c b/net/core/iovec.c
index de178e4..b77eeec 100644
--- a/net/core/iovec.c
+++ b/net/core/iovec.c

@@ -212,3 +212,27 @@ out_fault:
 	goto out;
 }
 EXPORT_SYMBOL(csum_partial_copy_fromiovecend);
+
+unsigned long iov_pages(const struct iovec *iov, int offset,
+			unsigned long nr_segs)
+{
+	unsigned long seg, base;
+	int pages = 0, len, size;
+
+	while (nr_segs && (offset >= iov->iov_len)) {
+		offset -= iov->iov_len;
+		++iov;
+		--nr_segs;
+	}
+
+	for (seg = 0; seg < nr_segs; seg++) {
+		base = (unsigned long)iov[seg].iov_base + offset;
+		len = iov[seg].iov_len - offset;
+		size = ((base & ~PAGE_MASK) + len + ~PAGE_MASK) >> PAGE_SHIFT;
+		pages += size;
+		offset = 0;
+	}
+
+	return pages;
+}
+EXPORT_SYMBOL(iov_pages);

-- 
1.7.1

[net-next 2/6] net: move zerocopy_sg_from_iovec() to net/core/datagram.c

From: Jason Wang <hidden>
Date: 2013-08-06 09:56:29

To let it be reused and reduce code duplication. Also document this function.

Signed-off-by: Jason Wang <redacted>
---
 drivers/net/macvtap.c  |   80 -----------------------------------------
 drivers/net/tun.c      |   80 -----------------------------------------
 include/linux/skbuff.h |    4 ++
 net/core/datagram.c    |   93 ++++++++++++++++++++++++++++++++++++++++++++++++
 4 files changed, 97 insertions(+), 160 deletions(-)

diff --git a/drivers/net/macvtap.c b/drivers/net/macvtap.c
index dfec20d..182364a 100644
--- a/drivers/net/macvtap.c
+++ b/drivers/net/macvtap.c

@@ -536,86 +536,6 @@ static inline struct sk_buff *macvtap_alloc_skb(struct sock *sk, size_t prepad,
 	return skb;
 }
 
-/* set skb frags from iovec, this can move to core network code for reuse */
-static int zerocopy_sg_from_iovec(struct sk_buff *skb, const struct iovec *from,
-				  int offset, size_t count)
-{
-	int len = iov_length(from, count) - offset;
-	int copy = skb_headlen(skb);
-	int size, offset1 = 0;
-	int i = 0;
-
-	/* Skip over from offset */
-	while (count && (offset >= from->iov_len)) {
-		offset -= from->iov_len;
-		++from;
-		--count;
-	}
-
-	/* copy up to skb headlen */
-	while (count && (copy > 0)) {
-		size = min_t(unsigned int, copy, from->iov_len - offset);
-		if (copy_from_user(skb->data + offset1, from->iov_base + offset,
-				   size))
-			return -EFAULT;
-		if (copy > size) {
-			++from;
-			--count;
-			offset = 0;
-		} else
-			offset += size;
-		copy -= size;
-		offset1 += size;
-	}
-
-	if (len == offset1)
-		return 0;
-
-	while (count--) {
-		struct page *page[MAX_SKB_FRAGS];
-		int num_pages;
-		unsigned long base;
-		unsigned long truesize;
-
-		len = from->iov_len - offset;
-		if (!len) {
-			offset = 0;
-			++from;
-			continue;
-		}
-		base = (unsigned long)from->iov_base + offset;
-		size = ((base & ~PAGE_MASK) + len + ~PAGE_MASK) >> PAGE_SHIFT;
-		if (i + size > MAX_SKB_FRAGS)
-			return -EMSGSIZE;
-		num_pages = get_user_pages_fast(base, size, 0, &page[i]);
-		if (num_pages != size) {
-			int j;
-
-			for (j = 0; j < num_pages; j++)
-				put_page(page[i + j]);
-			return -EFAULT;
-		}
-		truesize = size * PAGE_SIZE;
-		skb->data_len += len;
-		skb->len += len;
-		skb->truesize += truesize;
-		atomic_add(truesize, &skb->sk->sk_wmem_alloc);
-		while (len) {
-			int off = base & ~PAGE_MASK;
-			int size = min_t(int, len, PAGE_SIZE - off);
-			__skb_fill_page_desc(skb, i, page[i], off, size);
-			skb_shinfo(skb)->nr_frags++;
-			/* increase sk_wmem_alloc */
-			base += size;
-			len -= size;
-			i++;
-		}
-		offset = 0;
-		++from;
-	}
-	return 0;
-}
-
 /*
  * macvtap_skb_from_vnet_hdr and macvtap_skb_to_vnet_hdr should
  * be shared with the tun/tap driver.

diff --git a/drivers/net/tun.c b/drivers/net/tun.c
index 18527ef..b163047 100644
--- a/drivers/net/tun.c
+++ b/drivers/net/tun.c

@@ -961,86 +961,6 @@ static struct sk_buff *tun_alloc_skb(struct tun_file *tfile,
 	return skb;
 }
 
-/* set skb frags from iovec, this can move to core network code for reuse */
-static int zerocopy_sg_from_iovec(struct sk_buff *skb, const struct iovec *from,
-				  int offset, size_t count)
-{
-	int len = iov_length(from, count) - offset;
-	int copy = skb_headlen(skb);
-	int size, offset1 = 0;
-	int i = 0;
-
-	/* Skip over from offset */
-	while (count && (offset >= from->iov_len)) {
-		offset -= from->iov_len;
-		++from;
-		--count;
-	}
-
-	/* copy up to skb headlen */
-	while (count && (copy > 0)) {
-		size = min_t(unsigned int, copy, from->iov_len - offset);
-		if (copy_from_user(skb->data + offset1, from->iov_base + offset,
-				   size))
-			return -EFAULT;
-		if (copy > size) {
-			++from;
-			--count;
-			offset = 0;
-		} else
-			offset += size;
-		copy -= size;
-		offset1 += size;
-	}
-
-	if (len == offset1)
-		return 0;
-
-	while (count--) {
-		struct page *page[MAX_SKB_FRAGS];
-		int num_pages;
-		unsigned long base;
-		unsigned long truesize;
-
-		len = from->iov_len - offset;
-		if (!len) {
-			offset = 0;
-			++from;
-			continue;
-		}
-		base = (unsigned long)from->iov_base + offset;
-		size = ((base & ~PAGE_MASK) + len + ~PAGE_MASK) >> PAGE_SHIFT;
-		if (i + size > MAX_SKB_FRAGS)
-			return -EMSGSIZE;
-		num_pages = get_user_pages_fast(base, size, 0, &page[i]);
-		if (num_pages != size) {
-			int j;
-
-			for (j = 0; j < num_pages; j++)
-				put_page(page[i + j]);
-			return -EFAULT;
-		}
-		truesize = size * PAGE_SIZE;
-		skb->data_len += len;
-		skb->len += len;
-		skb->truesize += truesize;
-		atomic_add(truesize, &skb->sk->sk_wmem_alloc);
-		while (len) {
-			int off = base & ~PAGE_MASK;
-			int size = min_t(int, len, PAGE_SIZE - off);
-			__skb_fill_page_desc(skb, i, page[i], off, size);
-			skb_shinfo(skb)->nr_frags++;
-			/* increase sk_wmem_alloc */
-			base += size;
-			len -= size;
-			i++;
-		}
-		offset = 0;
-		++from;
-	}
-	return 0;
-}
-
 /* Get packet from user space buffer */
 static ssize_t tun_get_user(struct tun_struct *tun, struct tun_file *tfile,
 			    void *msg_control, const struct iovec *iv,

diff --git a/include/linux/skbuff.h b/include/linux/skbuff.h
index ae46475..5ac96f3 100644
--- a/include/linux/skbuff.h
+++ b/include/linux/skbuff.h

@@ -2359,6 +2359,10 @@ extern int	       skb_copy_datagram_from_iovec(struct sk_buff *skb,
 						    const struct iovec *from,
 						    int from_offset,
 						    int len);
+extern int	       zerocopy_sg_from_iovec(struct sk_buff *skb,
+					      const struct iovec *frm,
+					      int offset,
+					      size_t count);
 extern int	       skb_copy_datagram_const_iovec(const struct sk_buff *from,
 						     int offset,
 						     const struct iovec *to,

diff --git a/net/core/datagram.c b/net/core/datagram.c
index 8ab48cd..f987fae 100644
--- a/net/core/datagram.c
+++ b/net/core/datagram.c

@@ -573,6 +573,99 @@ fault:
 }
 EXPORT_SYMBOL(skb_copy_datagram_from_iovec);
 
+/**
+ *	zerocopy_sg_from_iovec - Build a zerocopy datagram from an iovec
+ *	@skb: buffer to copy
+ *	@from: io vector to copy to
+ *	@offset: offset in the io vector to start copying from
+ *	@count: amount of vectors to copy to buffer from
+ *
+ *	The function will first copy up to headlen, and then pin the userspace
+ *	pages and build frags through them.
+ *
+ *	Returns 0, -EFAULT or -EMSGSIZE.
+ *	Note: the iovec is not modified during the copy
+ */
+int zerocopy_sg_from_iovec(struct sk_buff *skb, const struct iovec *from,
+				  int offset, size_t count)
+{
+	int len = iov_length(from, count) - offset;
+	int copy = skb_headlen(skb);
+	int size, offset1 = 0;
+	int i = 0;
+
+	/* Skip over from offset */
+	while (count && (offset >= from->iov_len)) {
+		offset -= from->iov_len;
+		++from;
+		--count;
+	}
+
+	/* copy up to skb headlen */
+	while (count && (copy > 0)) {
+		size = min_t(unsigned int, copy, from->iov_len - offset);
+		if (copy_from_user(skb->data + offset1, from->iov_base + offset,
+				   size))
+			return -EFAULT;
+		if (copy > size) {
+			++from;
+			--count;
+			offset = 0;
+		} else
+			offset += size;
+		copy -= size;
+		offset1 += size;
+	}
+
+	if (len == offset1)
+		return 0;
+
+	while (count--) {
+		struct page *page[MAX_SKB_FRAGS];
+		int num_pages;
+		unsigned long base;
+		unsigned long truesize;
+
+		len = from->iov_len - offset;
+		if (!len) {
+			offset = 0;
+			++from;
+			continue;
+		}
+		base = (unsigned long)from->iov_base + offset;
+		size = ((base & ~PAGE_MASK) + len + ~PAGE_MASK) >> PAGE_SHIFT;
+		if (i + size > MAX_SKB_FRAGS)
+			return -EMSGSIZE;
+		num_pages = get_user_pages_fast(base, size, 0, &page[i]);
+		if (num_pages != size) {
+			int j;
+
+			for (j = 0; j < num_pages; j++)
+				put_page(page[i + j]);
+			return -EFAULT;
+		}
+		truesize = size * PAGE_SIZE;
+		skb->data_len += len;
+		skb->len += len;
+		skb->truesize += truesize;
+		atomic_add(truesize, &skb->sk->sk_wmem_alloc);
+		while (len) {
+			int off = base & ~PAGE_MASK;
+			int size = min_t(int, len, PAGE_SIZE - off);
+			__skb_fill_page_desc(skb, i, page[i], off, size);
+			skb_shinfo(skb)->nr_frags++;
+			/* increase sk_wmem_alloc */
+			base += size;
+			len -= size;
+			i++;
+		}
+		offset = 0;
+		++from;
+	}
+	return 0;
+}
+EXPORT_SYMBOL(zerocopy_sg_from_iovec);
+
 static int skb_copy_and_csum_datagram(const struct sk_buff *skb, int offset,
 				      u8 __user *to, int len,
 				      __wsum *csump)

-- 
1.7.1

[net-next 3/6] net: use skb_fill_page_desc() in zerocopy_sg_from_iovec()

From: Jason Wang <hidden>
Date: 2013-08-06 09:56:32

Signed-off-by: Jason Wang <redacted>
---
 net/core/datagram.c |    3 +--
 1 files changed, 1 insertions(+), 2 deletions(-)

diff --git a/net/core/datagram.c b/net/core/datagram.c
index f987fae..bec0867 100644
--- a/net/core/datagram.c
+++ b/net/core/datagram.c

@@ -652,8 +652,7 @@ int zerocopy_sg_from_iovec(struct sk_buff *skb, const struct iovec *from,
 		while (len) {
 			int off = base & ~PAGE_MASK;
 			int size = min_t(int, len, PAGE_SIZE - off);
-			__skb_fill_page_desc(skb, i, page[i], off, size);
-			skb_shinfo(skb)->nr_frags++;
+			skb_fill_page_desc(skb, i, page[i], off, size);
 			/* increase sk_wmem_alloc */
 			base += size;
 			len -= size;

-- 
1.7.1

[net-next 4/6] net: remove the useless comment in zerocopy_sg_from_iovec()

From: Jason Wang <hidden>
Date: 2013-08-06 09:56:35

Signed-off-by: Jason Wang <redacted>
---
 net/core/datagram.c |    1 -
 1 files changed, 0 insertions(+), 1 deletions(-)

diff --git a/net/core/datagram.c b/net/core/datagram.c
index bec0867..a8a795c 100644
--- a/net/core/datagram.c
+++ b/net/core/datagram.c

@@ -653,7 +653,6 @@ int zerocopy_sg_from_iovec(struct sk_buff *skb, const struct iovec *from,
 			int off = base & ~PAGE_MASK;
 			int size = min_t(int, len, PAGE_SIZE - off);
 			skb_fill_page_desc(skb, i, page[i], off, size);
-			/* increase sk_wmem_alloc */
 			base += size;
 			len -= size;
 			i++;

-- 
1.7.1

[net-next 5/6] net: use release_pages() in zerocopy_sg_from_iovec()

From: Jason Wang <hidden>
Date: 2013-08-06 09:56:37

To reduce the duplicated codes.

Signed-off-by: Jason Wang <redacted>
---
 net/core/datagram.c |    6 ++----
 1 files changed, 2 insertions(+), 4 deletions(-)

diff --git a/net/core/datagram.c b/net/core/datagram.c
index a8a795c..badcd93 100644
--- a/net/core/datagram.c
+++ b/net/core/datagram.c

@@ -48,6 +48,7 @@
 #include <linux/highmem.h>
 #include <linux/spinlock.h>
 #include <linux/slab.h>
+#include <linux/pagemap.h>
 
 #include <net/protocol.h>
 #include <linux/skbuff.h>

@@ -638,10 +639,7 @@ int zerocopy_sg_from_iovec(struct sk_buff *skb, const struct iovec *from,
 			return -EMSGSIZE;
 		num_pages = get_user_pages_fast(base, size, 0, &page[i]);
 		if (num_pages != size) {
-			int j;
-
-			for (j = 0; j < num_pages; j++)
-				put_page(page[i + j]);
+			release_pages(&page[i], num_pages, 0);
 			return -EFAULT;
 		}
 		truesize = size * PAGE_SIZE;

-- 
1.7.1

[net-next 6/6] net: use skb_copy_datagram_from_iovec() in zerocopy_sg_from_iovec()

From: Jason Wang <hidden>
Date: 2013-08-06 09:56:47

Use skb_copy_datagram_from_iovec() to avoid code duplication and make it easy to
be read. Also we can do the skipping inside the zero-copy loop.

Signed-off-by: Jason Wang <redacted>
---
 net/core/datagram.c |   37 ++++++++++---------------------------
 1 files changed, 10 insertions(+), 27 deletions(-)

diff --git a/net/core/datagram.c b/net/core/datagram.c
index badcd93..af814e7 100644
--- a/net/core/datagram.c
+++ b/net/core/datagram.c

@@ -591,48 +591,31 @@ int zerocopy_sg_from_iovec(struct sk_buff *skb, const struct iovec *from,
 				  int offset, size_t count)
 {
 	int len = iov_length(from, count) - offset;
-	int copy = skb_headlen(skb);
-	int size, offset1 = 0;
+	int copy = min_t(int, skb_headlen(skb), len);
+	int size;
 	int i = 0;
 
-	/* Skip over from offset */
-	while (count && (offset >= from->iov_len)) {
-		offset -= from->iov_len;
-		++from;
-		--count;
-	}
-
 	/* copy up to skb headlen */
-	while (count && (copy > 0)) {
-		size = min_t(unsigned int, copy, from->iov_len - offset);
-		if (copy_from_user(skb->data + offset1, from->iov_base + offset,
-				   size))
-			return -EFAULT;
-		if (copy > size) {
-			++from;
-			--count;
-			offset = 0;
-		} else
-			offset += size;
-		copy -= size;
-		offset1 += size;
-	}
+	if (skb_copy_datagram_from_iovec(skb, 0, from, offset, copy))
+		return -EFAULT;
 
-	if (len == offset1)
+	if (len == copy)
 		return 0;
 
+	offset += copy;
 	while (count--) {
 		struct page *page[MAX_SKB_FRAGS];
 		int num_pages;
 		unsigned long base;
 		unsigned long truesize;
 
-		len = from->iov_len - offset;
-		if (!len) {
-			offset = 0;
+		/* Skip over from offset and copied */
+		if (offset >= from->iov_len) {
+			offset -= from->iov_len;
 			++from;
 			continue;
 		}
+		len = from->iov_len - offset;
 		base = (unsigned long)from->iov_base + offset;
 		size = ((base & ~PAGE_MASK) + len + ~PAGE_MASK) >> PAGE_SHIFT;
 		if (i + size > MAX_SKB_FRAGS)

-- 
1.7.1

Re: [net-next 0/6] Reduce code duplication of zerocopy of tun and macvtap

From: David Miller <davem@davemloft.net>
Date: 2013-08-07 23:47:44

From: Jason Wang <redacted>
Date: Tue,  6 Aug 2013 17:45:02 +0800

This series tries to reduce the code duplication of zerocopy of tun and macvtap
by:

- Move common helpers iov_pages() and zerocopy_sg_from_iovec() to net/core
- Use exist helpers instead of open coding the new one

With this seires, we can do further optimizations on top.

Looks great, applied!

`h`	back out one level
`j`	next message in thread
`k`	previous message in thread
`l`	drill in
`Esc`	close help / fold thread tree
`?`	toggle this help