diff mbox series

[RFC,06/37] mm: page_alloc: Allocate from movable pcp lists only if ALLOC_FROM_METADATA

Message ID 20230823131350.114942-7-alexandru.elisei@arm.com (mailing list archive)
State Handled Elsewhere
Headers show
Series [RFC,01/37] mm: page_alloc: Rename gfp_to_alloc_flags_cma -> gfp_to_alloc_flags_fast | expand

Commit Message

Alexandru Elisei Aug. 23, 2023, 1:13 p.m. UTC 
pcp lists keep MIGRATE_METADATA pages on the MIGRATE_MOVABLE list. Make
sure pages from the movable list are allocated only when the
ALLOC_FROM_METADATA alloc flag is set, as otherwise the page allocator
could end up allocating a metadata page when that page cannot be used.

__alloc_pages_bulk() sidesteps rmqueue() and calls __rmqueue_pcplist()
directly. Add a check for the flag before calling __rmqueue_pcplist(), and
fallback to __alloc_pages() if the check is false.

Note that CMA isn't a problem for __alloc_pages_bulk(): an allocation can
always use CMA pages if the requested migratetype is MIGRATE_MOVABLE, which
is not the case with MIGRATE_METADATA pages.

Signed-off-by: Alexandru Elisei <alexandru.elisei@arm.com>
---
 mm/page_alloc.c | 21 +++++++++++++++++----
 1 file changed, 17 insertions(+), 4 deletions(-)

Comments

Hyesoo Yu Oct. 12, 2023, 1:25 a.m. UTC | #1 
On Wed, Aug 23, 2023 at 02:13:19PM +0100, Alexandru Elisei wrote:
> pcp lists keep MIGRATE_METADATA pages on the MIGRATE_MOVABLE list. Make
> sure pages from the movable list are allocated only when the
> ALLOC_FROM_METADATA alloc flag is set, as otherwise the page allocator
> could end up allocating a metadata page when that page cannot be used.
> 
> __alloc_pages_bulk() sidesteps rmqueue() and calls __rmqueue_pcplist()
> directly. Add a check for the flag before calling __rmqueue_pcplist(), and
> fallback to __alloc_pages() if the check is false.
> 
> Note that CMA isn't a problem for __alloc_pages_bulk(): an allocation can
> always use CMA pages if the requested migratetype is MIGRATE_MOVABLE, which
> is not the case with MIGRATE_METADATA pages.
> 
> Signed-off-by: Alexandru Elisei <alexandru.elisei@arm.com>
> ---
>  mm/page_alloc.c | 21 +++++++++++++++++----
>  1 file changed, 17 insertions(+), 4 deletions(-)
> 
> diff --git a/mm/page_alloc.c b/mm/page_alloc.c
> index 829134a4dfa8..a693e23c4733 100644
> --- a/mm/page_alloc.c
> +++ b/mm/page_alloc.c
> @@ -2845,11 +2845,16 @@ struct page *rmqueue(struct zone *preferred_zone,
>  
>  	if (likely(pcp_allowed_order(order))) {
>  		/*
> -		 * MIGRATE_MOVABLE pcplist could have the pages on CMA area and
> -		 * we need to skip it when CMA area isn't allowed.
> +		 * PCP lists keep MIGRATE_CMA/MIGRATE_METADATA pages on the same
> +		 * movable list. Make sure it's allowed to allocate both type of
> +		 * pages before allocating from the movable list.
>  		 */
> -		if (!IS_ENABLED(CONFIG_CMA) || alloc_flags & ALLOC_CMA ||
> -				migratetype != MIGRATE_MOVABLE) {
> +		bool movable_allowed = (!IS_ENABLED(CONFIG_CMA) ||
> +					(alloc_flags & ALLOC_CMA)) &&
> +				       (!IS_ENABLED(CONFIG_MEMORY_METADATA) ||
> +					(alloc_flags & ALLOC_FROM_METADATA));
> +
> +		if (migratetype != MIGRATE_MOVABLE || movable_allowed) {

Hi!

I don't think it would be effcient when the majority of movable pages
do not use GFP_TAGGED.

Metadata pages have a low probability of being in the pcp list
because metadata pages is bypassed when freeing pages.

The allocation performance of most movable pages is likely to decrease
if only the request with ALLOC_FROM_METADATA could be allocated.

How about not including metadata pages in the pcp list at all ?

Thanks,
Hyesoo Yu.

>  			page = rmqueue_pcplist(preferred_zone, zone, order,
>  					migratetype, alloc_flags);
>  			if (likely(page))
> @@ -4388,6 +4393,14 @@ unsigned long __alloc_pages_bulk(gfp_t gfp, int preferred_nid,
>  		goto out;
>  	gfp = alloc_gfp;
>  
> +	/*
> +	 * pcp lists puts MIGRATE_METADATA on the MIGRATE_MOVABLE list, don't
> +	 * use pcp if allocating metadata pages is not allowed.
> +	 */
> +	if (metadata_storage_enabled() && ac.migratetype == MIGRATE_MOVABLE &&
> +	    !(alloc_flags & ALLOC_FROM_METADATA))
> +		goto failed;
> +
>  	/* Find an allowed local zone that meets the low watermark. */
>  	for_each_zone_zonelist_nodemask(zone, z, ac.zonelist, ac.highest_zoneidx, ac.nodemask) {
>  		unsigned long mark;
> -- 
> 2.41.0
> 
>
Alexandru Elisei Oct. 16, 2023, 12:41 p.m. UTC | #2 
Hi,

On Thu, Oct 12, 2023 at 10:25:11AM +0900, Hyesoo Yu wrote:
> On Wed, Aug 23, 2023 at 02:13:19PM +0100, Alexandru Elisei wrote:
> > pcp lists keep MIGRATE_METADATA pages on the MIGRATE_MOVABLE list. Make
> > sure pages from the movable list are allocated only when the
> > ALLOC_FROM_METADATA alloc flag is set, as otherwise the page allocator
> > could end up allocating a metadata page when that page cannot be used.
> > 
> > __alloc_pages_bulk() sidesteps rmqueue() and calls __rmqueue_pcplist()
> > directly. Add a check for the flag before calling __rmqueue_pcplist(), and
> > fallback to __alloc_pages() if the check is false.
> > 
> > Note that CMA isn't a problem for __alloc_pages_bulk(): an allocation can
> > always use CMA pages if the requested migratetype is MIGRATE_MOVABLE, which
> > is not the case with MIGRATE_METADATA pages.
> > 
> > Signed-off-by: Alexandru Elisei <alexandru.elisei@arm.com>
> > ---
> >  mm/page_alloc.c | 21 +++++++++++++++++----
> >  1 file changed, 17 insertions(+), 4 deletions(-)
> > 
> > diff --git a/mm/page_alloc.c b/mm/page_alloc.c
> > index 829134a4dfa8..a693e23c4733 100644
> > --- a/mm/page_alloc.c
> > +++ b/mm/page_alloc.c
> > @@ -2845,11 +2845,16 @@ struct page *rmqueue(struct zone *preferred_zone,
> >  
> >  	if (likely(pcp_allowed_order(order))) {
> >  		/*
> > -		 * MIGRATE_MOVABLE pcplist could have the pages on CMA area and
> > -		 * we need to skip it when CMA area isn't allowed.
> > +		 * PCP lists keep MIGRATE_CMA/MIGRATE_METADATA pages on the same
> > +		 * movable list. Make sure it's allowed to allocate both type of
> > +		 * pages before allocating from the movable list.
> >  		 */
> > -		if (!IS_ENABLED(CONFIG_CMA) || alloc_flags & ALLOC_CMA ||
> > -				migratetype != MIGRATE_MOVABLE) {
> > +		bool movable_allowed = (!IS_ENABLED(CONFIG_CMA) ||
> > +					(alloc_flags & ALLOC_CMA)) &&
> > +				       (!IS_ENABLED(CONFIG_MEMORY_METADATA) ||
> > +					(alloc_flags & ALLOC_FROM_METADATA));
> > +
> > +		if (migratetype != MIGRATE_MOVABLE || movable_allowed) {
> 
> Hi!
> 
> I don't think it would be effcient when the majority of movable pages
> do not use GFP_TAGGED.
> 
> Metadata pages have a low probability of being in the pcp list
> because metadata pages is bypassed when freeing pages.
> 
> The allocation performance of most movable pages is likely to decrease
> if only the request with ALLOC_FROM_METADATA could be allocated.

You're right, I hadn't considered that.

> 
> How about not including metadata pages in the pcp list at all ?

Sounds reasonable, I will keep it in mind for the next iteration of the
series.

Thanks,
Alex

> 
> Thanks,
> Hyesoo Yu.
> 
> >  			page = rmqueue_pcplist(preferred_zone, zone, order,
> >  					migratetype, alloc_flags);
> >  			if (likely(page))
> > @@ -4388,6 +4393,14 @@ unsigned long __alloc_pages_bulk(gfp_t gfp, int preferred_nid,
> >  		goto out;
> >  	gfp = alloc_gfp;
> >  
> > +	/*
> > +	 * pcp lists puts MIGRATE_METADATA on the MIGRATE_MOVABLE list, don't
> > +	 * use pcp if allocating metadata pages is not allowed.
> > +	 */
> > +	if (metadata_storage_enabled() && ac.migratetype == MIGRATE_MOVABLE &&
> > +	    !(alloc_flags & ALLOC_FROM_METADATA))
> > +		goto failed;
> > +
> >  	/* Find an allowed local zone that meets the low watermark. */
> >  	for_each_zone_zonelist_nodemask(zone, z, ac.zonelist, ac.highest_zoneidx, ac.nodemask) {
> >  		unsigned long mark;
> > -- 
> > 2.41.0
> > 
> >
Catalin Marinas Oct. 17, 2023, 10:26 a.m. UTC | #3 
On Mon, Oct 16, 2023 at 01:41:15PM +0100, Alexandru Elisei wrote:
> On Thu, Oct 12, 2023 at 10:25:11AM +0900, Hyesoo Yu wrote:
> > I don't think it would be effcient when the majority of movable pages
> > do not use GFP_TAGGED.
> > 
> > Metadata pages have a low probability of being in the pcp list
> > because metadata pages is bypassed when freeing pages.
> > 
> > The allocation performance of most movable pages is likely to decrease
> > if only the request with ALLOC_FROM_METADATA could be allocated.
> 
> You're right, I hadn't considered that.
> 
> > 
> > How about not including metadata pages in the pcp list at all ?
> 
> Sounds reasonable, I will keep it in mind for the next iteration of the
> series.

BTW, I suggest for the next iteration we drop MIGRATE_METADATA, only use
CMA and assume that the tag storage itself supports tagging. Hopefully
it makes the patches a bit simpler.
diff mbox series

Patch

diff --git a/mm/page_alloc.c b/mm/page_alloc.c
index 829134a4dfa8..a693e23c4733 100644
--- a/mm/page_alloc.c
+++ b/mm/page_alloc.c
@@ -2845,11 +2845,16 @@  struct page *rmqueue(struct zone *preferred_zone,
 
 	if (likely(pcp_allowed_order(order))) {
 		/*
-		 * MIGRATE_MOVABLE pcplist could have the pages on CMA area and
-		 * we need to skip it when CMA area isn't allowed.
+		 * PCP lists keep MIGRATE_CMA/MIGRATE_METADATA pages on the same
+		 * movable list. Make sure it's allowed to allocate both type of
+		 * pages before allocating from the movable list.
 		 */
-		if (!IS_ENABLED(CONFIG_CMA) || alloc_flags & ALLOC_CMA ||
-				migratetype != MIGRATE_MOVABLE) {
+		bool movable_allowed = (!IS_ENABLED(CONFIG_CMA) ||
+					(alloc_flags & ALLOC_CMA)) &&
+				       (!IS_ENABLED(CONFIG_MEMORY_METADATA) ||
+					(alloc_flags & ALLOC_FROM_METADATA));
+
+		if (migratetype != MIGRATE_MOVABLE || movable_allowed) {
 			page = rmqueue_pcplist(preferred_zone, zone, order,
 					migratetype, alloc_flags);
 			if (likely(page))
@@ -4388,6 +4393,14 @@  unsigned long __alloc_pages_bulk(gfp_t gfp, int preferred_nid,
 		goto out;
 	gfp = alloc_gfp;
 
+	/*
+	 * pcp lists puts MIGRATE_METADATA on the MIGRATE_MOVABLE list, don't
+	 * use pcp if allocating metadata pages is not allowed.
+	 */
+	if (metadata_storage_enabled() && ac.migratetype == MIGRATE_MOVABLE &&
+	    !(alloc_flags & ALLOC_FROM_METADATA))
+		goto failed;
+
 	/* Find an allowed local zone that meets the low watermark. */
 	for_each_zone_zonelist_nodemask(zone, z, ac.zonelist, ac.highest_zoneidx, ac.nodemask) {
 		unsigned long mark;