Re: [RESEND] [PATCH] tcp: fix for splice receive when used with software LRO

[Octavian Purdila <opurdila@ixiacom.com>]

[-- Attachment #1: Type: text/plain, Size: 862 bytes --]

On Friday 20 June 2008, Jarek Poplawski wrote:
> >
> > Let me try to wrap my brain around this again :) and see if we don't miss
> > anything. In the process, maybe I can even find a way to make this code
> > easier to understand.
>
> Sure, no need to hurry, and after all, I'm OK with the current patch too.
>

OK, here is my attempt at making it a bit more readable:

- move all of the details on offsets, lengths and buffers into a 
single function instead of doing these operation from multiple places

- use a bottom up approach - try to avoid details in the high level functions, 
introduce them gradually as you go deeper in the function call stack

I've attached both the patch and a full excerpt - it might be easier to 
comment on it  this way.

I've minimally tested the patch, no obvious functional or performance problems 
were found.

Thanks,
tavi

[-- Attachment #2: y --]
[-- Type: text/plain, Size: 3161 bytes --]


static inline void __segment_seek(struct page **page, int *poff, int *plen,
				  int off)
{
	*poff += off;
	*page += *poff / PAGE_SIZE;
	*poff = *poff % PAGE_SIZE;
	*plen -= off;
}

static inline int __splice_segment(struct page *page, unsigned int poff,
				   unsigned int plen, unsigned int *off,
				   unsigned int *len, struct sk_buff *skb,
				   struct splice_pipe_desc *spd)
{
	/* skip this segment if already processed */
	if (*off >= plen) {
		*off -= plen;
		return 0;
	}

	/* ignore any bits we already processed */
	if (*off) {
		__segment_seek(&page, &poff, &plen, *off);
		*off = 0;
	}

	do {
		unsigned int flen = min(*len, plen);

		if (spd_fill_page(spd, page, flen, poff, skb))
			return 1;

		__segment_seek(&page, &poff, &plen, flen);
		*len -= flen;

	} while (*len && plen);

	return 0;
}

int __skb_splice_bits(struct sk_buff *skb, unsigned int *offset,
			     unsigned int *len,
			     struct splice_pipe_desc *spd)
{
	int seg;

	/*
	 * map the linear part
	 */
	if (__splice_segment(virt_to_page(skb->data),
			     (unsigned long) skb->data & (PAGE_SIZE - 1),
			     skb_headlen(skb),
			     offset, len, skb, spd))
		return 1;

	/*
	 * then map the fragments
	 */
	for (seg = 0; seg < skb_shinfo(skb)->nr_frags; seg++) {
		const skb_frag_t *f = &skb_shinfo(skb)->frags[seg];

		if (__splice_segment(f->page, f->page_offset, f->size,
				     offset, len, skb, spd))
			return 1;
	}

	/*
	 * now see if we have a frag_list to map
	 */
	if (skb_shinfo(skb)->frag_list) {
		struct sk_buff *list = skb_shinfo(skb)->frag_list;

		for (; list && *len; list = list->next)
			if (__skb_splice_bits(list, offset, len, spd))
				return 1;
	}

	return 0;
}

/*
 * Map data from the skb to a pipe. Should handle both the linear part,
 * the fragments, and the frag list.
 */
int skb_splice_bits(struct sk_buff *__skb, unsigned int offset,
		    struct pipe_inode_info *pipe, unsigned int tlen,
		    unsigned int flags)
{
	struct partial_page partial[PIPE_BUFFERS];
	struct page *pages[PIPE_BUFFERS];
	struct splice_pipe_desc spd = {
		.pages = pages,
		.partial = partial,
		.flags = flags,
		.ops = &sock_pipe_buf_ops,
		.spd_release = sock_spd_release,
	};
	struct sk_buff *skb;

	/*
	 * I'd love to avoid the clone here, but tcp_read_sock()
	 * ignores reference counts and unconditonally kills the sk_buff
	 * on return from the actor.
	 */
	skb = skb_clone(__skb, GFP_KERNEL);
	if (unlikely(!skb))
		return -ENOMEM;


	__skb_splice_bits(skb, &offset, &tlen, &spd);

	/*
	 * drop our reference to the clone, the pipe consumption will
	 * drop the rest.
	 */
	kfree_skb(skb);

	if (spd.nr_pages) {
		int ret;
		struct sock *sk = __skb->sk;

		/*
		 * Drop the socket lock, otherwise we have reverse
		 * locking dependencies between sk_lock and i_mutex
		 * here as compared to sendfile(). We enter here
		 * with the socket lock held, and splice_to_pipe() will
		 * grab the pipe inode lock. For sendfile() emulation,
		 * we call into ->sendpage() with the i_mutex lock held
		 * and networking will grab the socket lock.
		 */
		release_sock(sk);
		ret = splice_to_pipe(pipe, &spd);
		lock_sock(sk);
		return ret;
	}

	return 0;
}

[-- Attachment #3: x --]
[-- Type: text/plain, Size: 4985 bytes --]

diff --git a/net/core/skbuff.c b/net/core/skbuff.c
index 25fa74d..017daef 100644
--- a/net/core/skbuff.c
+++ b/net/core/skbuff.c
@@ -1188,114 +1188,89 @@ static inline int spd_fill_page(struct splice_pipe_desc *spd, struct page *page,
 	return 0;
 }
 
-/*
- * Map linear and fragment data from the skb to spd. Returns number of
- * pages mapped.
- */
-static int __skb_splice_bits(struct sk_buff *skb, unsigned int *offset,
-			     unsigned int *total_len,
-			     struct splice_pipe_desc *spd)
+static inline void __segment_seek(struct page **page, int *poff, int *plen,
+				  int off)
 {
-	unsigned int nr_pages = spd->nr_pages;
-	unsigned int poff, plen, len, toff, tlen;
-	int headlen, seg;
+	*poff += off;
+	*page += *poff / PAGE_SIZE;
+	*poff = *poff % PAGE_SIZE;
+	*plen -= off;
+}
 
-	toff = *offset;
-	tlen = *total_len;
-	if (!tlen)
-		goto err;
+static inline int __splice_segment(struct page *page, unsigned int poff,
+				   unsigned int plen, unsigned int *off,
+				   unsigned int *len, struct sk_buff *skb,
+				   struct splice_pipe_desc *spd)
+{
+	/* skip this segment if already processed */
+	if (*off >= plen) {
+		*off -= plen;
+		return 0;
+	}
 
-	/*
-	 * if the offset is greater than the linear part, go directly to
-	 * the fragments.
-	 */
-	headlen = skb_headlen(skb);
-	if (toff >= headlen) {
-		toff -= headlen;
-		goto map_frag;
+	/* ignore any bits we already processed */
+	if (*off) {
+		__segment_seek(&page, &poff, &plen, *off);
+		*off = 0;
 	}
 
-	/*
-	 * first map the linear region into the pages/partial map, skipping
-	 * any potential initial offset.
-	 */
-	len = 0;
-	while (len < headlen) {
-		void *p = skb->data + len;
-
-		poff = (unsigned long) p & (PAGE_SIZE - 1);
-		plen = min_t(unsigned int, headlen - len, PAGE_SIZE - poff);
-		len += plen;
-
-		if (toff) {
-			if (plen <= toff) {
-				toff -= plen;
-				continue;
-			}
-			plen -= toff;
-			poff += toff;
-			toff = 0;
-		}
+	do {
+		unsigned int flen = min(*len, plen);
 
-		plen = min(plen, tlen);
-		if (!plen)
-			break;
+		if (spd_fill_page(spd, page, flen, poff, skb))
+			return 1;
 
-		/*
-		 * just jump directly to update and return, no point
-		 * in going over fragments when the output is full.
-		 */
-		if (spd_fill_page(spd, virt_to_page(p), plen, poff, skb))
-			goto done;
+		__segment_seek(&page, &poff, &plen, flen);
+		*len -= flen;
 
-		tlen -= plen;
-	}
+	} while (*len && plen);
+
+	return 0;
+}
+
+int __skb_splice_bits(struct sk_buff *skb, unsigned int *offset,
+			     unsigned int *len,
+			     struct splice_pipe_desc *spd)
+{
+	int seg;
+
+	/*
+	 * map the linear part
+	 */
+	if (__splice_segment(virt_to_page(skb->data),
+			     (unsigned long) skb->data & (PAGE_SIZE - 1),
+			     skb_headlen(skb),
+			     offset, len, skb, spd))
+		return 1;
 
 	/*
 	 * then map the fragments
 	 */
-map_frag:
 	for (seg = 0; seg < skb_shinfo(skb)->nr_frags; seg++) {
 		const skb_frag_t *f = &skb_shinfo(skb)->frags[seg];
 
-		plen = f->size;
-		poff = f->page_offset;
-
-		if (toff) {
-			if (plen <= toff) {
-				toff -= plen;
-				continue;
-			}
-			plen -= toff;
-			poff += toff;
-			toff = 0;
-		}
-
-		plen = min(plen, tlen);
-		if (!plen)
-			break;
+		if (__splice_segment(f->page, f->page_offset, f->size,
+				     offset, len, skb, spd))
+			return 1;
+	}
 
-		if (spd_fill_page(spd, f->page, plen, poff, skb))
-			break;
+	/*
+	 * now see if we have a frag_list to map
+	 */
+	if (skb_shinfo(skb)->frag_list) {
+		struct sk_buff *list = skb_shinfo(skb)->frag_list;
 
-		tlen -= plen;
+		for (; list && *len; list = list->next)
+			if (__skb_splice_bits(list, offset, len, spd))
+				return 1;
 	}
 
-done:
-	if (spd->nr_pages - nr_pages) {
-		*offset = 0;
-		*total_len = tlen;
-		return 0;
-	}
-err:
-	return 1;
+	return 0;
 }
 
 /*
  * Map data from the skb to a pipe. Should handle both the linear part,
- * the fragments, and the frag list. It does NOT handle frag lists within
- * the frag list, if such a thing exists. We'd probably need to recurse to
- * handle that cleanly.
+ * the fragments, and the frag list.
  */
 int skb_splice_bits(struct sk_buff *__skb, unsigned int offset,
 		    struct pipe_inode_info *pipe, unsigned int tlen,
@@ -1321,28 +1296,9 @@ int skb_splice_bits(struct sk_buff *__skb, unsigned int offset,
 	if (unlikely(!skb))
 		return -ENOMEM;
 
-	/*
-	 * __skb_splice_bits() only fails if the output has no room left,
-	 * so no point in going over the frag_list for the error case.
-	 */
-	if (__skb_splice_bits(skb, &offset, &tlen, &spd))
-		goto done;
-	else if (!tlen)
-		goto done;
-
-	/*
-	 * now see if we have a frag_list to map
-	 */
-	if (skb_shinfo(skb)->frag_list) {
-		struct sk_buff *list = skb_shinfo(skb)->frag_list;
 
-		for (; list && tlen; list = list->next) {
-			if (__skb_splice_bits(list, &offset, &tlen, &spd))
-				break;
-		}
-	}
+	__skb_splice_bits(skb, &offset, &tlen, &spd);
 
-done:
 	/*
 	 * drop our reference to the clone, the pipe consumption will
 	 * drop the rest.