Re: [PATCH net-next v7 03/16] net: Add a function to splice pages into an skbuff for MSG_SPLICE_PAGES

[David Howells <dhowells@redhat.com>]

Paolo Abeni <pabeni@redhat.com> wrote:

> Minor nit: please respect the reverse x-mas tree order (there are a few
> other occurrences around)

I hadn't come across that.  Normally I only apply that to the types so that
the names aren't all over the place.  But whatever.

> > +		if (space == 0 &&
> > +		    !skb_can_coalesce(skb, skb_shinfo(skb)->nr_frags,
> > +				      pages[0], off)) {
> > +			iov_iter_revert(iter, len);
> > +			break;
> > +		}
> 
> It looks like the above condition/checks duplicate what the later
> skb_append_pagefrags() will perform below. I guess the above chunk
> could be removed?

Good point.  There used to be an allocation between in the case sendpage_ok()
failed and we wanted to copy the data.  I've removed that for the moment.

> > +			ret = -EIO;
> > +			if (!sendpage_ok(page))
> > +				goto out;
> 
> My (limited) understanding is that the current sendpage code assumes
> that the caller provides/uses pages suitable for such use. The existing
> sendpage_ok() check is in place as way to try to catch possible code
> bug - via the WARN_ONCE().
>
> I think the same could be done here?

Yeah.

Okay, I made the attached changes to this patch.

David
---
diff --git a/net/core/skbuff.c b/net/core/skbuff.c
index 56d629ea2f3d..f4a5b51aed22 100644
--- a/net/core/skbuff.c
+++ b/net/core/skbuff.c
@@ -6923,10 +6923,10 @@ static void skb_splice_csum_page(struct sk_buff *skb, struct page *page,
 ssize_t skb_splice_from_iter(struct sk_buff *skb, struct iov_iter *iter,
 			     ssize_t maxsize, gfp_t gfp)
 {
+	size_t frag_limit = READ_ONCE(sysctl_max_skb_frags);
 	struct page *pages[8], **ppages = pages;
-	unsigned int i;
 	ssize_t spliced = 0, ret = 0;
-	size_t frag_limit = READ_ONCE(sysctl_max_skb_frags);
+	unsigned int i;
 
 	while (iter->count > 0) {
 		ssize_t space, nr;
@@ -6946,20 +6946,13 @@ ssize_t skb_splice_from_iter(struct sk_buff *skb, struct iov_iter *iter,
 			break;
 		}
 
-		if (space == 0 &&
-		    !skb_can_coalesce(skb, skb_shinfo(skb)->nr_frags,
-				      pages[0], off)) {
-			iov_iter_revert(iter, len);
-			break;
-		}
-
 		i = 0;
 		do {
 			struct page *page = pages[i++];
 			size_t part = min_t(size_t, PAGE_SIZE - off, len);
 
 			ret = -EIO;
-			if (!sendpage_ok(page))
+			if (WARN_ON_ONCE(!sendpage_ok(page)))
 				goto out;
 
 			ret = skb_append_pagefrags(skb, page, off, part,