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: <6cf2ea121c4fdbd04682224c5acf6c73cc47f2f7.camel@redhat.com>
Subject: Re: [PATCH net-next v3 01/18] net: Copy slab data for
 sendmsg(MSG_SPLICE_PAGES)
From:   Paolo Abeni <pabeni@redhat.com>
To:     David Howells <dhowells@redhat.com>
Cc:     netdev@vger.kernel.org,
        Alexander Duyck <alexander.duyck@gmail.com>,
        "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>,
        Jens Axboe <axboe@kernel.dk>, linux-mm@kvack.org,
        linux-kernel@vger.kernel.org, Menglong Dong <imagedong@tencent.com>
Date:   Fri, 23 Jun 2023 11:37:12 +0200
In-Reply-To: <1969720.1687511219@warthog.procyon.org.uk>
References: <634c885ccfb2e49e284aedc60e157bb12e5f3530.camel@redhat.com>
         <20230620145338.1300897-1-dhowells@redhat.com>
         <20230620145338.1300897-2-dhowells@redhat.com>
         <1969720.1687511219@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 Fri, 2023-06-23 at 10:06 +0100, David Howells wrote:
> Paolo Abeni <pabeni@redhat.com> wrote:
>=20
> > IMHO this function uses a bit too much labels and would be more easy to
> > read, e.g. moving the above chunk of code in conditional branch.
>=20
> Maybe.  I was trying to put the fast path up at the top without the slow =
path
> bits in it, but I can put the "insufficient_space" bit there.

I *think* you could move the insufficient_space in a separate helped,
that should achieve your goal with fewer labels and hopefully no
additional complexity.

>=20
> > Even without such change, I think the above 'goto try_again;'
> > introduces an unneeded conditional, as at this point we know 'fragsz <=
=3D
> > fsize'.
>=20
> Good point.
>=20
> > > +		cache->pfmemalloc =3D folio_is_pfmemalloc(spare);
> > > +		if (cache->folio)
> > > +			goto reload;
> >=20
> > I think there is some problem with the above.
> >=20
> > If cache->folio is !=3D NULL, and cache->folio was not pfmemalloc-ed
> > while the spare one is, it looks like the wrong policy will be used.
> > And should be even worse if folio was pfmemalloc-ed while spare is not.
> >=20
> > I think moving 'cache->pfmemalloc' initialization...
> >=20
> > > +	}
> > > +
> >=20
> > ... here should fix the above.
>=20
> Yeah.  We might have raced with someone else or been moved to another cpu=
 and
> there might now be a folio we can allocate from.
>=20
> > > +	/* Reset page count bias and offset to start of new frag */
> > > +	cache->pagecnt_bias =3D PAGE_FRAG_CACHE_MAX_SIZE + 1;
> > > +	offset =3D folio_size(folio);
> > > +	goto try_again;
> >=20
> > What if fragsz > PAGE_SIZE, we are consistently unable to allocate an
> > high order page, but order-0, pfmemalloc-ed page allocation is
> > successful? It looks like this could become an unbounded loop?
>=20
> It shouldn't.  It should go:
>=20
> 	try_again:
> 		if (fragsz > offset)
> 			goto insufficient_space;
> 	insufficient_space:
> 		/* See if we can refurbish the current folio. */
> 		...

I think the critical path is with pfmemalloc-ed pages:

		if (unlikely(cache->pfmemalloc)) {
			__folio_put(folio);
			goto get_new_folio;
		}

just before the following.

> 		fsize =3D folio_size(folio);
> 		if (unlikely(fragsz > fsize))
> 			goto frag_too_big;
> 	frag_too_big:
> 		...
> 		return NULL;
>=20
> Though for safety's sake, it would make sense to put in a size check in t=
he
> case we fail to allocate a larger-order folio.
>=20
> > >  		do {
> > >  			struct page *page =3D pages[i++];
> > >  			size_t part =3D min_t(size_t, PAGE_SIZE - off, len);
> > > -
> > > -			ret =3D -EIO;
> > > -			if (WARN_ON_ONCE(!sendpage_ok(page)))
> > > +			bool put =3D false;
> > > +
> > > +			if (PageSlab(page)) {
> >=20
> > I'm a bit concerned from the above. If I read correctly, tcp 0-copy
>=20
> Well, splice()-to-tcp will; MSG_ZEROCOPY is unaffected.

Ah right! I got lost in some 'if' branch.

> > will go through that for every page, even if the expected use-case is
> > always !PageSlub(page). compound_head() could be costly if the head
> > page is not hot on cache and I'm not sure if that could be the case for
> > tcp 0-copy. The bottom line is that I fear a possible regression here.
>=20
> I can put the PageSlab() check inside the sendpage_ok() so the page flag =
is
> only checked once. =C2=A0

Perhaps I'm lost again, but AFAICS:

__PAGEFLAG(Slab, slab, PF_NO_TAIL)

// ...
#define __PAGEFLAG(uname, lname, policy)			\
	TESTPAGEFLAG(uname, lname, policy)			\
// ...

#define TESTPAGEFLAG(uname, lname, policy)				\
static __always_inline bool folio_test_##lname(struct folio *folio)	\
{ return test_bit(PG_##lname, folio_flags(folio, FOLIO_##policy));}	\
static __always_inline int Page##uname(struct page *page)               \
{ return test_bit(PG_##lname, &policy(page, 0)->flags); }
// ... 'policy' is PF_NO_TAIL here

#define PF_NO_TAIL(page, enforce) ({                                    \
                VM_BUG_ON_PGFLAGS(enforce && PageTail(page), page);	\
                PF_POISONED_CHECK(compound_head(page)); })

It looks at compound_head in the end ?!?

> But PageSlab() doesn't check the headpage, only the page
> it is given.  sendpage_ok() is more the problem as it also calls
> page_count().  I could drop the check.

Once the head page is hot on cache due to the previous check, it should
be cheap?

Cheers,

Paolo