Received-SPF: pass (google.com: domain of linux-kernel+bounces-134595-linux.lists.archive=gmail.com@vger.kernel.org designates 2604:1380:40f1:3f00::1 as permitted sender) client-ip=2604:1380:40f1:3f00::1;
Message-ID: <30a6635dedf305e23e623c501e614e55bca1ab41.camel@gmail.com>
Subject: Re: [PATCH net-next v1 01/12] mm: Move the page fragment allocator
 from page_alloc into its own file
From: Alexander H Duyck <alexander.duyck@gmail.com>
To: Yunsheng Lin <linyunsheng@huawei.com>, davem@davemloft.net,
 kuba@kernel.org,  pabeni@redhat.com
Cc: netdev@vger.kernel.org, linux-kernel@vger.kernel.org, David Howells
	 <dhowells@redhat.com>, Andrew Morton <akpm@linux-foundation.org>, 
	linux-mm@kvack.org
Date: Sun, 07 Apr 2024 10:42:22 -0700
In-Reply-To: <20240407130850.19625-2-linyunsheng@huawei.com>
References: <20240407130850.19625-1-linyunsheng@huawei.com>
	 <20240407130850.19625-2-linyunsheng@huawei.com>
Content-Type: text/plain; charset="UTF-8"
Content-Transfer-Encoding: quoted-printable
User-Agent: Evolution 3.48.4 (3.48.4-1.fc38) 
Precedence: bulk
MIME-Version: 1.0

On Sun, 2024-04-07 at 21:08 +0800, Yunsheng Lin wrote:
> Inspired by [1], but use free_unref_page() to replace free_the_page()
> instead of __free_pages(), use VM_BUG_ON() to catch that we can use
> free_unref_page() directly, also add its own header file.

Instead of doing this all as one patch it would be better to split this
into 2. Make the refactor first, and then move the code. Don't do it
all in one patch.

Adding a refactor that changes out functions called in addition to
moving the functions makes it very difficult to provide useful feedback
and review.

One of the big things with free_unref_page vx free_the_page is a check
to see how costly it is to free the page. Right now the page frags are
on the verge of crossing that threshold with defaulting to order 3
pages.

> As the API is only used by the networking, it may make sense to
> move it to the networking directory like the page_pool does in the
> future if we can make the free_unref_page() callable outside of the
> mm subsystem. And we can utilize that to decouple the 'struct page'
> in the networking subsystem in the future.
>=20
> 1. https://lore.kernel.org/all/20230411160902.4134381-3-dhowells@redhat.c=
om/

The problem with moving this out to networking is the fact that this is
memory allocation. In my opinion it would better for it to live there.

> CC: David Howells <dhowells@redhat.com>
> Signed-off-by: Yunsheng Lin <linyunsheng@huawei.com>
> ---
>  include/linux/gfp.h             |  22 -----
>  include/linux/mm_types.h        |  18 ----
>  include/linux/page_frag_cache.h |  47 ++++++++++
>  include/linux/skbuff.h          |   1 +
>  mm/Makefile                     |   1 +
>  mm/page_alloc.c                 | 136 -----------------------------
>  mm/page_frag_cache.c            | 149 ++++++++++++++++++++++++++++++++
>  7 files changed, 198 insertions(+), 176 deletions(-)
>  create mode 100644 include/linux/page_frag_cache.h
>  create mode 100644 mm/page_frag_cache.c

I would add comments inline below about the changes you made but it is
hard to keep them in any sort of context since what is contained in the
"-" block is well spaced out from the "+" block.