Re: [PATCH RFC 06/37] mm: page_alloc: Allocate from movable pcp lists only if ALLOC_FROM_METADATA
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 > --- > 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 > >
Re: [PATCH RFC 06/37] mm: page_alloc: Allocate from movable pcp lists only if ALLOC_FROM_METADATA
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 > > --- > > 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 > > > >
Re: [PATCH RFC 06/37] mm: page_alloc: Allocate from movable pcp lists only if ALLOC_FROM_METADATA
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. -- Catalin