Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) client-ip=2620:137:e000::1:20;
Message-ID: <47caea363e844bf716867c6a128d374cae4a5772.camel@redhat.com>
Subject: Re: [PATCH net-next v7 03/16] net: Add a function to splice pages
 into an skbuff for MSG_SPLICE_PAGES
From:   Paolo Abeni <pabeni@redhat.com>
To:     David Howells <dhowells@redhat.com>
Cc:     netdev@vger.kernel.org, "David S. Miller" <davem@davemloft.net>,
        Eric Dumazet <edumazet@google.com>,
        Jakub Kicinski <kuba@kernel.org>,
        Willem de Bruijn <willemdebruijn.kernel@gmail.com>,
        David Ahern <dsahern@kernel.org>,
        Matthew Wilcox <willy@infradead.org>,
        Al Viro <viro@zeniv.linux.org.uk>,
        Christoph Hellwig <hch@infradead.org>,
        Jens Axboe <axboe@kernel.dk>, Jeff Layton <jlayton@kernel.org>,
        Christian Brauner <brauner@kernel.org>,
        Chuck Lever III <chuck.lever@oracle.com>,
        Linus Torvalds <torvalds@linux-foundation.org>,
        linux-fsdevel@vger.kernel.org, linux-kernel@vger.kernel.org,
        linux-mm@kvack.org
Date:   Thu, 18 May 2023 12:25:13 +0200
In-Reply-To: <1347187.1684403608@warthog.procyon.org.uk>
References: <93aba6cc363e94a6efe433b3c77ec1b6b54f2919.camel@redhat.com>
         <20230515093345.396978-1-dhowells@redhat.com>
         <20230515093345.396978-4-dhowells@redhat.com>
         <1347187.1684403608@warthog.procyon.org.uk>
Content-Type: text/plain; charset="UTF-8"
Content-Transfer-Encoding: quoted-printable
User-Agent: Evolution 3.46.4 (3.46.4-1.fc37) 
MIME-Version: 1.0
Precedence: bulk

On Thu, 2023-05-18 at 10:53 +0100, David Howells wrote:
> Paolo Abeni <pabeni@redhat.com> wrote:
>=20
> > Minor nit: please respect the reverse x-mas tree order (there are a few
> > other occurrences around)
>=20
> I hadn't come across that.  Normally I only apply that to the types so th=
at
> the names aren't all over the place.  But whatever.
>=20
> > > +		if (space =3D=3D 0 &&
> > > +		    !skb_can_coalesce(skb, skb_shinfo(skb)->nr_frags,
> > > +				      pages[0], off)) {
> > > +			iov_iter_revert(iter, len);
> > > +			break;
> > > +		}
> >=20
> > 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?
>=20
> 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.
>=20
> > > +			ret =3D -EIO;
> > > +			if (!sendpage_ok(page))
> > > +				goto out;
> >=20
> > 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().
> >=20
> > I think the same could be done here?
>=20
> Yeah.
>=20
> Okay, I made the attached changes to this patch.
>=20
> 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 =3D READ_ONCE(sysctl_max_skb_frags);
>  	struct page *pages[8], **ppages =3D pages;
> -	unsigned int i;
>  	ssize_t spliced =3D 0, ret =3D 0;
> -	size_t frag_limit =3D READ_ONCE(sysctl_max_skb_frags);
> +	unsigned int i;
> =20
>  	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;
>  		}
> =20
> -		if (space =3D=3D 0 &&
> -		    !skb_can_coalesce(skb, skb_shinfo(skb)->nr_frags,
> -				      pages[0], off)) {
> -			iov_iter_revert(iter, len);
> -			break;
> -		}
> -
>  		i =3D 0;
>  		do {
>  			struct page *page =3D pages[i++];
>  			size_t part =3D min_t(size_t, PAGE_SIZE - off, len);
> =20
>  			ret =3D -EIO;
> -			if (!sendpage_ok(page))
> +			if (WARN_ON_ONCE(!sendpage_ok(page)))

FWIS the current TCP code also has a 'IS_ENABLED(CONFIG_DEBUG_VM) &&'
guard, but I guess the plain WARN_ON_ONCE should be ok.

Side node: we need the whole series alltogether, you need to repost
even the unmodified patches.

Thanks!

Paolo