[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]
Message-ID: <1347187.1684403608@warthog.procyon.org.uk>
Date: Thu, 18 May 2023 10:53:28 +0100
From: David Howells <dhowells@...hat.com>
To: Paolo Abeni <pabeni@...hat.com>
Cc: dhowells@...hat.com, netdev@...r.kernel.org,
"David S. Miller" <davem@...emloft.net>,
Eric Dumazet <edumazet@...gle.com>, Jakub Kicinski <kuba@...nel.org>,
Willem de Bruijn <willemdebruijn.kernel@...il.com>,
David Ahern <dsahern@...nel.org>,
Matthew Wilcox <willy@...radead.org>,
Al Viro <viro@...iv.linux.org.uk>,
Christoph Hellwig <hch@...radead.org>, Jens Axboe <axboe@...nel.dk>,
Jeff Layton <jlayton@...nel.org>,
Christian Brauner <brauner@...nel.org>,
Chuck Lever III <chuck.lever@...cle.com>,
Linus Torvalds <torvalds@...ux-foundation.org>,
linux-fsdevel@...r.kernel.org, linux-kernel@...r.kernel.org,
linux-mm@...ck.org
Subject: Re: [PATCH net-next v7 03/16] net: Add a function to splice pages into an skbuff for MSG_SPLICE_PAGES
Paolo Abeni <pabeni@...hat.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,
Powered by blists - more mailing lists