* [PATCH v2 for v5.9] mm/page_alloc: handle a missing case for memalloc_nocma_{save/restore} APIs @ 2020-09-28 8:50 js1304 2020-09-28 23:52 ` Andrew Morton ` (2 more replies) 0 siblings, 3 replies; 12+ messages in thread From: js1304 @ 2020-09-28 8:50 UTC (permalink / raw) To: Andrew Morton Cc: linux-mm, linux-kernel, Michal Hocko, Vlastimil Babka, Aneesh Kumar K . V, Mel Gorman, kernel-team, Joonsoo Kim From: Joonsoo Kim <iamjoonsoo.kim@lge.com> memalloc_nocma_{save/restore} APIs can be used to skip page allocation on CMA area, but, there is a missing case and the page on CMA area could be allocated even if APIs are used. This patch handles this case to fix the potential issue. Missing case is an allocation from the pcplist. MIGRATE_MOVABLE pcplist could have the pages on CMA area so we need to skip it if ALLOC_CMA isn't specified. Fixes: 8510e69c8efe (mm/page_alloc: fix memalloc_nocma_{save/restore} APIs) Signed-off-by: Joonsoo Kim <iamjoonsoo.kim@lge.com> --- mm/page_alloc.c | 13 ++++++++++--- 1 file changed, 10 insertions(+), 3 deletions(-) diff --git a/mm/page_alloc.c b/mm/page_alloc.c index fab5e97..104d2e1 100644 --- a/mm/page_alloc.c +++ b/mm/page_alloc.c @@ -3367,9 +3367,16 @@ struct page *rmqueue(struct zone *preferred_zone, struct page *page; if (likely(order == 0)) { - page = rmqueue_pcplist(preferred_zone, zone, gfp_flags, + /* + * MIGRATE_MOVABLE pcplist could have the pages on CMA area and + * we need to skip it when CMA area isn't allowed. + */ + if (!IS_ENABLED(CONFIG_CMA) || alloc_flags & ALLOC_CMA || + migratetype != MIGRATE_MOVABLE) { + page = rmqueue_pcplist(preferred_zone, zone, gfp_flags, migratetype, alloc_flags); - goto out; + goto out; + } } /* @@ -3381,7 +3388,7 @@ struct page *rmqueue(struct zone *preferred_zone, do { page = NULL; - if (alloc_flags & ALLOC_HARDER) { + if (order > 0 && alloc_flags & ALLOC_HARDER) { page = __rmqueue_smallest(zone, order, MIGRATE_HIGHATOMIC); if (page) trace_mm_page_alloc_zone_locked(page, order, migratetype); -- 2.7.4 ^ permalink raw reply related [flat|nested] 12+ messages in thread
* Re: [PATCH v2 for v5.9] mm/page_alloc: handle a missing case for memalloc_nocma_{save/restore} APIs 2020-09-28 8:50 [PATCH v2 for v5.9] mm/page_alloc: handle a missing case for memalloc_nocma_{save/restore} APIs js1304 @ 2020-09-28 23:52 ` Andrew Morton 2020-09-29 1:28 ` Joonsoo Kim 2020-09-29 8:08 ` Michal Hocko 2020-09-29 8:13 ` Vlastimil Babka 2 siblings, 1 reply; 12+ messages in thread From: Andrew Morton @ 2020-09-28 23:52 UTC (permalink / raw) To: js1304 Cc: linux-mm, linux-kernel, Michal Hocko, Vlastimil Babka, Aneesh Kumar K . V, Mel Gorman, kernel-team, Joonsoo Kim On Mon, 28 Sep 2020 17:50:46 +0900 js1304@gmail.com wrote: > From: Joonsoo Kim <iamjoonsoo.kim@lge.com> > > memalloc_nocma_{save/restore} APIs can be used to skip page allocation > on CMA area, but, there is a missing case and the page on CMA area could > be allocated even if APIs are used. This patch handles this case to fix > the potential issue. > > Missing case is an allocation from the pcplist. MIGRATE_MOVABLE pcplist > could have the pages on CMA area so we need to skip it if ALLOC_CMA isn't > specified. Changelog doesn't describe the end-user visible effects of the bug. Please send this description? > --- a/mm/page_alloc.c > +++ b/mm/page_alloc.c > @@ -3367,9 +3367,16 @@ struct page *rmqueue(struct zone *preferred_zone, > struct page *page; > > if (likely(order == 0)) { > - page = rmqueue_pcplist(preferred_zone, zone, gfp_flags, > + /* > + * MIGRATE_MOVABLE pcplist could have the pages on CMA area and > + * we need to skip it when CMA area isn't allowed. > + */ > + if (!IS_ENABLED(CONFIG_CMA) || alloc_flags & ALLOC_CMA || > + migratetype != MIGRATE_MOVABLE) { > + page = rmqueue_pcplist(preferred_zone, zone, gfp_flags, > migratetype, alloc_flags); > - goto out; > + goto out; > + } > } > > /* We still really really don't want to be adding overhead to the page allocation hotpath for a really really obscure feature from a single callsite. Do we have an understanding of how many people's kernels are enabling CONFIG_CMA? I previously suggested retrying the allocation in __gup_longterm_locked() but you said "it cannot ensure that we eventually get the non-CMA page". Please explain why? What about manually emptying the pcplists beforehand? Or byassing the pcplists for this caller and calling __rmqueue() directly? > @@ -3381,7 +3388,7 @@ struct page *rmqueue(struct zone *preferred_zone, > > do { > page = NULL; > - if (alloc_flags & ALLOC_HARDER) { > + if (order > 0 && alloc_flags & ALLOC_HARDER) { > page = __rmqueue_smallest(zone, order, MIGRATE_HIGHATOMIC); > if (page) > trace_mm_page_alloc_zone_locked(page, order, migratetype); What does this hunk do? ^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [PATCH v2 for v5.9] mm/page_alloc: handle a missing case for memalloc_nocma_{save/restore} APIs 2020-09-28 23:52 ` Andrew Morton @ 2020-09-29 1:28 ` Joonsoo Kim 0 siblings, 0 replies; 12+ messages in thread From: Joonsoo Kim @ 2020-09-29 1:28 UTC (permalink / raw) To: Andrew Morton Cc: Linux Memory Management List, LKML, Michal Hocko, Vlastimil Babka, Aneesh Kumar K . V, Mel Gorman, kernel-team, Joonsoo Kim 2020년 9월 29일 (화) 오전 8:52, Andrew Morton <akpm@linux-foundation.org>님이 작성: > > On Mon, 28 Sep 2020 17:50:46 +0900 js1304@gmail.com wrote: > > > From: Joonsoo Kim <iamjoonsoo.kim@lge.com> > > > > memalloc_nocma_{save/restore} APIs can be used to skip page allocation > > on CMA area, but, there is a missing case and the page on CMA area could > > be allocated even if APIs are used. This patch handles this case to fix > > the potential issue. > > > > Missing case is an allocation from the pcplist. MIGRATE_MOVABLE pcplist > > could have the pages on CMA area so we need to skip it if ALLOC_CMA isn't > > specified. > > Changelog doesn't describe the end-user visible effects of the bug. > Please send this description? How about this one? memalloc_nocma_{save/restore} APIs can be used to skip page allocation on CMA area, but, there is a missing case and the page on CMA area could be allocated even if APIs are used. This patch handles this case to fix the potential issue. For now, these APIs are used to prevent long-term pinning on the CMA page. When the long-term pinning is requested on the CMA page, it is migrated to the non-CMA page before pinning. This non-CMA page is allocated by using memalloc_nocma_{save/restore} APIs. If APIs doesn't work as intended, the CMA page is allocated and it is pinned for a long time. This long-term pin for the CMA page causes cma_alloc() failure and it could result in wrong behaviour on the device driver who uses the cma_alloc(). Missing case is an allocation from the pcplist. MIGRATE_MOVABLE pcplist could have the pages on CMA area so we need to skip it if ALLOC_CMA isn't specified. > > --- a/mm/page_alloc.c > > +++ b/mm/page_alloc.c > > @@ -3367,9 +3367,16 @@ struct page *rmqueue(struct zone *preferred_zone, > > struct page *page; > > > > if (likely(order == 0)) { > > - page = rmqueue_pcplist(preferred_zone, zone, gfp_flags, > > + /* > > + * MIGRATE_MOVABLE pcplist could have the pages on CMA area and > > + * we need to skip it when CMA area isn't allowed. > > + */ > > + if (!IS_ENABLED(CONFIG_CMA) || alloc_flags & ALLOC_CMA || > > + migratetype != MIGRATE_MOVABLE) { > > + page = rmqueue_pcplist(preferred_zone, zone, gfp_flags, > > migratetype, alloc_flags); > > - goto out; > > + goto out; > > + } > > } > > > > /* > > We still really really don't want to be adding overhead to the page > allocation hotpath for a really really obscure feature from a single > callsite. In fact, if we clear the __GFP_MOVABLE flag when initializing the allocation context, we can avoid CMA page allocation without adding this overhead to the page allocation hotpath. But, I think this is not a good practice since it cheats the allocation type. There would be a side-effect, for example, we cannot use the memory on the ZONE_MOVABLE in this case. > Do we have an understanding of how many people's kernels are enabling > CONFIG_CMA? AFAIK, the almost embedded system enables CONFIG_CMA. For example, the handset, TV, AVN in a car. Recently, Roman makes CMA usable for huge page allocation so users are continuously increased. > I previously suggested retrying the allocation in > __gup_longterm_locked() but you said "it cannot ensure that we > eventually get the non-CMA page". Please explain why? To avoid allocating a CMA page, the pcplist should be empty. Retry doesn't guarantee that we will hit the case that the pcplist is empty. It increases the probability for this case, but, I think that relying on the probability is not a good practice. > What about manually emptying the pcplists beforehand? It also increases the probability. schedule() or interrupt after emptying but before the allocation could invalidate the effect. > Or byassing the pcplists for this caller and calling __rmqueue() directly? What this patch does is this one. > > @@ -3381,7 +3388,7 @@ struct page *rmqueue(struct zone *preferred_zone, > > > > do { > > page = NULL; > > - if (alloc_flags & ALLOC_HARDER) { > > + if (order > 0 && alloc_flags & ALLOC_HARDER) { > > page = __rmqueue_smallest(zone, order, MIGRATE_HIGHATOMIC); > > if (page) > > trace_mm_page_alloc_zone_locked(page, order, migratetype); > > What does this hunk do? MIGRATE_HIGHATOMIC is a reserved area for high order atomic allocation. This hunk makes that order-0 allocation skip this area. Thanks. ^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [PATCH v2 for v5.9] mm/page_alloc: handle a missing case for memalloc_nocma_{save/restore} APIs @ 2020-09-29 1:28 ` Joonsoo Kim 0 siblings, 0 replies; 12+ messages in thread From: Joonsoo Kim @ 2020-09-29 1:28 UTC (permalink / raw) To: Andrew Morton Cc: Linux Memory Management List, LKML, Michal Hocko, Vlastimil Babka, Aneesh Kumar K . V, Mel Gorman, kernel-team, Joonsoo Kim 2020년 9월 29일 (화) 오전 8:52, Andrew Morton <akpm@linux-foundation.org>님이 작성: > > On Mon, 28 Sep 2020 17:50:46 +0900 js1304@gmail.com wrote: > > > From: Joonsoo Kim <iamjoonsoo.kim@lge.com> > > > > memalloc_nocma_{save/restore} APIs can be used to skip page allocation > > on CMA area, but, there is a missing case and the page on CMA area could > > be allocated even if APIs are used. This patch handles this case to fix > > the potential issue. > > > > Missing case is an allocation from the pcplist. MIGRATE_MOVABLE pcplist > > could have the pages on CMA area so we need to skip it if ALLOC_CMA isn't > > specified. > > Changelog doesn't describe the end-user visible effects of the bug. > Please send this description? How about this one? memalloc_nocma_{save/restore} APIs can be used to skip page allocation on CMA area, but, there is a missing case and the page on CMA area could be allocated even if APIs are used. This patch handles this case to fix the potential issue. For now, these APIs are used to prevent long-term pinning on the CMA page. When the long-term pinning is requested on the CMA page, it is migrated to the non-CMA page before pinning. This non-CMA page is allocated by using memalloc_nocma_{save/restore} APIs. If APIs doesn't work as intended, the CMA page is allocated and it is pinned for a long time. This long-term pin for the CMA page causes cma_alloc() failure and it could result in wrong behaviour on the device driver who uses the cma_alloc(). Missing case is an allocation from the pcplist. MIGRATE_MOVABLE pcplist could have the pages on CMA area so we need to skip it if ALLOC_CMA isn't specified. > > --- a/mm/page_alloc.c > > +++ b/mm/page_alloc.c > > @@ -3367,9 +3367,16 @@ struct page *rmqueue(struct zone *preferred_zone, > > struct page *page; > > > > if (likely(order == 0)) { > > - page = rmqueue_pcplist(preferred_zone, zone, gfp_flags, > > + /* > > + * MIGRATE_MOVABLE pcplist could have the pages on CMA area and > > + * we need to skip it when CMA area isn't allowed. > > + */ > > + if (!IS_ENABLED(CONFIG_CMA) || alloc_flags & ALLOC_CMA || > > + migratetype != MIGRATE_MOVABLE) { > > + page = rmqueue_pcplist(preferred_zone, zone, gfp_flags, > > migratetype, alloc_flags); > > - goto out; > > + goto out; > > + } > > } > > > > /* > > We still really really don't want to be adding overhead to the page > allocation hotpath for a really really obscure feature from a single > callsite. In fact, if we clear the __GFP_MOVABLE flag when initializing the allocation context, we can avoid CMA page allocation without adding this overhead to the page allocation hotpath. But, I think this is not a good practice since it cheats the allocation type. There would be a side-effect, for example, we cannot use the memory on the ZONE_MOVABLE in this case. > Do we have an understanding of how many people's kernels are enabling > CONFIG_CMA? AFAIK, the almost embedded system enables CONFIG_CMA. For example, the handset, TV, AVN in a car. Recently, Roman makes CMA usable for huge page allocation so users are continuously increased. > I previously suggested retrying the allocation in > __gup_longterm_locked() but you said "it cannot ensure that we > eventually get the non-CMA page". Please explain why? To avoid allocating a CMA page, the pcplist should be empty. Retry doesn't guarantee that we will hit the case that the pcplist is empty. It increases the probability for this case, but, I think that relying on the probability is not a good practice. > What about manually emptying the pcplists beforehand? It also increases the probability. schedule() or interrupt after emptying but before the allocation could invalidate the effect. > Or byassing the pcplists for this caller and calling __rmqueue() directly? What this patch does is this one. > > @@ -3381,7 +3388,7 @@ struct page *rmqueue(struct zone *preferred_zone, > > > > do { > > page = NULL; > > - if (alloc_flags & ALLOC_HARDER) { > > + if (order > 0 && alloc_flags & ALLOC_HARDER) { > > page = __rmqueue_smallest(zone, order, MIGRATE_HIGHATOMIC); > > if (page) > > trace_mm_page_alloc_zone_locked(page, order, migratetype); > > What does this hunk do? MIGRATE_HIGHATOMIC is a reserved area for high order atomic allocation. This hunk makes that order-0 allocation skip this area. Thanks. ^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [PATCH v2 for v5.9] mm/page_alloc: handle a missing case for memalloc_nocma_{save/restore} APIs 2020-09-29 1:28 ` Joonsoo Kim (?) @ 2020-09-29 4:50 ` Andrew Morton 2020-09-29 6:46 ` Joonsoo Kim -1 siblings, 1 reply; 12+ messages in thread From: Andrew Morton @ 2020-09-29 4:50 UTC (permalink / raw) To: Joonsoo Kim Cc: Linux Memory Management List, LKML, Michal Hocko, Vlastimil Babka, Aneesh Kumar K . V, Mel Gorman, kernel-team, Joonsoo Kim On Tue, 29 Sep 2020 10:28:05 +0900 Joonsoo Kim <js1304@gmail.com> wrote: > > What about manually emptying the pcplists beforehand? > > It also increases the probability. schedule() or interrupt after emptying but > before the allocation could invalidate the effect. Keep local interrupts disabled across the pcp drain and the allocation attempt. > > Or byassing the pcplists for this caller and calling __rmqueue() directly? > > What this patch does is this one. I meant via a different function rather than by adding overhead to the existing commonly-used function. ^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [PATCH v2 for v5.9] mm/page_alloc: handle a missing case for memalloc_nocma_{save/restore} APIs 2020-09-29 4:50 ` Andrew Morton @ 2020-09-29 6:46 ` Joonsoo Kim 0 siblings, 0 replies; 12+ messages in thread From: Joonsoo Kim @ 2020-09-29 6:46 UTC (permalink / raw) To: Andrew Morton Cc: Linux Memory Management List, LKML, Michal Hocko, Vlastimil Babka, Aneesh Kumar K . V, Mel Gorman, kernel-team, Joonsoo Kim 2020년 9월 29일 (화) 오후 1:50, Andrew Morton <akpm@linux-foundation.org>님이 작성: > > On Tue, 29 Sep 2020 10:28:05 +0900 Joonsoo Kim <js1304@gmail.com> wrote: > > > > What about manually emptying the pcplists beforehand? > > > > It also increases the probability. schedule() or interrupt after emptying but > > before the allocation could invalidate the effect. > > Keep local interrupts disabled across the pcp drain and the allocation > attempt. As said before, it's an allocation context API and actual allocation happens later. Doing such things there is not an easy job. > > > Or byassing the pcplists for this caller and calling __rmqueue() directly? > > > > What this patch does is this one. > > I meant via a different function rather than by adding overhead to the > existing commonly-used function. Got it. One idea could be disabling/enabling pcp list on these APIs but it's overhead would not be appropriate on these APIs. I don't have another idea that doesn't touch the allocation path. Thanks. ^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [PATCH v2 for v5.9] mm/page_alloc: handle a missing case for memalloc_nocma_{save/restore} APIs @ 2020-09-29 6:46 ` Joonsoo Kim 0 siblings, 0 replies; 12+ messages in thread From: Joonsoo Kim @ 2020-09-29 6:46 UTC (permalink / raw) To: Andrew Morton Cc: Linux Memory Management List, LKML, Michal Hocko, Vlastimil Babka, Aneesh Kumar K . V, Mel Gorman, kernel-team, Joonsoo Kim 2020년 9월 29일 (화) 오후 1:50, Andrew Morton <akpm@linux-foundation.org>님이 작성: > > On Tue, 29 Sep 2020 10:28:05 +0900 Joonsoo Kim <js1304@gmail.com> wrote: > > > > What about manually emptying the pcplists beforehand? > > > > It also increases the probability. schedule() or interrupt after emptying but > > before the allocation could invalidate the effect. > > Keep local interrupts disabled across the pcp drain and the allocation > attempt. As said before, it's an allocation context API and actual allocation happens later. Doing such things there is not an easy job. > > > Or byassing the pcplists for this caller and calling __rmqueue() directly? > > > > What this patch does is this one. > > I meant via a different function rather than by adding overhead to the > existing commonly-used function. Got it. One idea could be disabling/enabling pcp list on these APIs but it's overhead would not be appropriate on these APIs. I don't have another idea that doesn't touch the allocation path. Thanks. ^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [PATCH v2 for v5.9] mm/page_alloc: handle a missing case for memalloc_nocma_{save/restore} APIs 2020-09-28 8:50 [PATCH v2 for v5.9] mm/page_alloc: handle a missing case for memalloc_nocma_{save/restore} APIs js1304 2020-09-28 23:52 ` Andrew Morton @ 2020-09-29 8:08 ` Michal Hocko 2020-09-29 8:38 ` Joonsoo Kim 2020-09-29 8:13 ` Vlastimil Babka 2 siblings, 1 reply; 12+ messages in thread From: Michal Hocko @ 2020-09-29 8:08 UTC (permalink / raw) To: js1304 Cc: Andrew Morton, linux-mm, linux-kernel, Vlastimil Babka, Aneesh Kumar K . V, Mel Gorman, kernel-team, Joonsoo Kim On Mon 28-09-20 17:50:46, Joonsoo Kim wrote: > From: Joonsoo Kim <iamjoonsoo.kim@lge.com> > > memalloc_nocma_{save/restore} APIs can be used to skip page allocation > on CMA area, but, there is a missing case and the page on CMA area could > be allocated even if APIs are used. This patch handles this case to fix > the potential issue. > > Missing case is an allocation from the pcplist. MIGRATE_MOVABLE pcplist > could have the pages on CMA area so we need to skip it if ALLOC_CMA isn't > specified. > > Fixes: 8510e69c8efe (mm/page_alloc: fix memalloc_nocma_{save/restore} APIs) > Signed-off-by: Joonsoo Kim <iamjoonsoo.kim@lge.com> > --- > mm/page_alloc.c | 13 ++++++++++--- > 1 file changed, 10 insertions(+), 3 deletions(-) > > diff --git a/mm/page_alloc.c b/mm/page_alloc.c > index fab5e97..104d2e1 100644 > --- a/mm/page_alloc.c > +++ b/mm/page_alloc.c > @@ -3367,9 +3367,16 @@ struct page *rmqueue(struct zone *preferred_zone, > struct page *page; > > if (likely(order == 0)) { > - page = rmqueue_pcplist(preferred_zone, zone, gfp_flags, > + /* > + * MIGRATE_MOVABLE pcplist could have the pages on CMA area and > + * we need to skip it when CMA area isn't allowed. > + */ > + if (!IS_ENABLED(CONFIG_CMA) || alloc_flags & ALLOC_CMA || > + migratetype != MIGRATE_MOVABLE) { > + page = rmqueue_pcplist(preferred_zone, zone, gfp_flags, > migratetype, alloc_flags); > - goto out; > + goto out; > + } > } This approach looks definitely better than the previous version. > > /* > @@ -3381,7 +3388,7 @@ struct page *rmqueue(struct zone *preferred_zone, > > do { > page = NULL; > - if (alloc_flags & ALLOC_HARDER) { > + if (order > 0 && alloc_flags & ALLOC_HARDER) { > page = __rmqueue_smallest(zone, order, MIGRATE_HIGHATOMIC); > if (page) > trace_mm_page_alloc_zone_locked(page, order, migratetype); But this condition is not clear to me. __rmqueue_smallest doesn't access pcp lists. Maybe I have missed the point in the original discussion but this deserves a comment at least. > -- > 2.7.4 -- Michal Hocko SUSE Labs ^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [PATCH v2 for v5.9] mm/page_alloc: handle a missing case for memalloc_nocma_{save/restore} APIs 2020-09-29 8:08 ` Michal Hocko @ 2020-09-29 8:38 ` Joonsoo Kim 0 siblings, 0 replies; 12+ messages in thread From: Joonsoo Kim @ 2020-09-29 8:38 UTC (permalink / raw) To: Michal Hocko Cc: Andrew Morton, Linux Memory Management List, LKML, Vlastimil Babka, Aneesh Kumar K . V, Mel Gorman, kernel-team, Joonsoo Kim 2020년 9월 29일 (화) 오후 5:08, Michal Hocko <mhocko@suse.com>님이 작성: > > On Mon 28-09-20 17:50:46, Joonsoo Kim wrote: > > From: Joonsoo Kim <iamjoonsoo.kim@lge.com> > > > > memalloc_nocma_{save/restore} APIs can be used to skip page allocation > > on CMA area, but, there is a missing case and the page on CMA area could > > be allocated even if APIs are used. This patch handles this case to fix > > the potential issue. > > > > Missing case is an allocation from the pcplist. MIGRATE_MOVABLE pcplist > > could have the pages on CMA area so we need to skip it if ALLOC_CMA isn't > > specified. > > > > Fixes: 8510e69c8efe (mm/page_alloc: fix memalloc_nocma_{save/restore} APIs) > > Signed-off-by: Joonsoo Kim <iamjoonsoo.kim@lge.com> > > --- > > mm/page_alloc.c | 13 ++++++++++--- > > 1 file changed, 10 insertions(+), 3 deletions(-) > > > > diff --git a/mm/page_alloc.c b/mm/page_alloc.c > > index fab5e97..104d2e1 100644 > > --- a/mm/page_alloc.c > > +++ b/mm/page_alloc.c > > @@ -3367,9 +3367,16 @@ struct page *rmqueue(struct zone *preferred_zone, > > struct page *page; > > > > if (likely(order == 0)) { > > - page = rmqueue_pcplist(preferred_zone, zone, gfp_flags, > > + /* > > + * MIGRATE_MOVABLE pcplist could have the pages on CMA area and > > + * we need to skip it when CMA area isn't allowed. > > + */ > > + if (!IS_ENABLED(CONFIG_CMA) || alloc_flags & ALLOC_CMA || > > + migratetype != MIGRATE_MOVABLE) { > > + page = rmqueue_pcplist(preferred_zone, zone, gfp_flags, > > migratetype, alloc_flags); > > - goto out; > > + goto out; > > + } > > } > > This approach looks definitely better than the previous version. Thanks! > > > > /* > > @@ -3381,7 +3388,7 @@ struct page *rmqueue(struct zone *preferred_zone, > > > > do { > > page = NULL; > > - if (alloc_flags & ALLOC_HARDER) { > > + if (order > 0 && alloc_flags & ALLOC_HARDER) { > > page = __rmqueue_smallest(zone, order, MIGRATE_HIGHATOMIC); > > if (page) > > trace_mm_page_alloc_zone_locked(page, order, migratetype); > > But this condition is not clear to me. __rmqueue_smallest doesn't access > pcp lists. Maybe I have missed the point in the original discussion but > this deserves a comment at least. Before the pcplist skipping is applied, order-0 request can not reach here. But, now, an order-0 request can reach here. Free memory on MIGRATE_HIGHATOMIC is reserved for high-order atomic allocation so an order-0 request should skip it. I will add a code comment on the next version. Thanks. ^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [PATCH v2 for v5.9] mm/page_alloc: handle a missing case for memalloc_nocma_{save/restore} APIs @ 2020-09-29 8:38 ` Joonsoo Kim 0 siblings, 0 replies; 12+ messages in thread From: Joonsoo Kim @ 2020-09-29 8:38 UTC (permalink / raw) To: Michal Hocko Cc: Andrew Morton, Linux Memory Management List, LKML, Vlastimil Babka, Aneesh Kumar K . V, Mel Gorman, kernel-team, Joonsoo Kim 2020년 9월 29일 (화) 오후 5:08, Michal Hocko <mhocko@suse.com>님이 작성: > > On Mon 28-09-20 17:50:46, Joonsoo Kim wrote: > > From: Joonsoo Kim <iamjoonsoo.kim@lge.com> > > > > memalloc_nocma_{save/restore} APIs can be used to skip page allocation > > on CMA area, but, there is a missing case and the page on CMA area could > > be allocated even if APIs are used. This patch handles this case to fix > > the potential issue. > > > > Missing case is an allocation from the pcplist. MIGRATE_MOVABLE pcplist > > could have the pages on CMA area so we need to skip it if ALLOC_CMA isn't > > specified. > > > > Fixes: 8510e69c8efe (mm/page_alloc: fix memalloc_nocma_{save/restore} APIs) > > Signed-off-by: Joonsoo Kim <iamjoonsoo.kim@lge.com> > > --- > > mm/page_alloc.c | 13 ++++++++++--- > > 1 file changed, 10 insertions(+), 3 deletions(-) > > > > diff --git a/mm/page_alloc.c b/mm/page_alloc.c > > index fab5e97..104d2e1 100644 > > --- a/mm/page_alloc.c > > +++ b/mm/page_alloc.c > > @@ -3367,9 +3367,16 @@ struct page *rmqueue(struct zone *preferred_zone, > > struct page *page; > > > > if (likely(order == 0)) { > > - page = rmqueue_pcplist(preferred_zone, zone, gfp_flags, > > + /* > > + * MIGRATE_MOVABLE pcplist could have the pages on CMA area and > > + * we need to skip it when CMA area isn't allowed. > > + */ > > + if (!IS_ENABLED(CONFIG_CMA) || alloc_flags & ALLOC_CMA || > > + migratetype != MIGRATE_MOVABLE) { > > + page = rmqueue_pcplist(preferred_zone, zone, gfp_flags, > > migratetype, alloc_flags); > > - goto out; > > + goto out; > > + } > > } > > This approach looks definitely better than the previous version. Thanks! > > > > /* > > @@ -3381,7 +3388,7 @@ struct page *rmqueue(struct zone *preferred_zone, > > > > do { > > page = NULL; > > - if (alloc_flags & ALLOC_HARDER) { > > + if (order > 0 && alloc_flags & ALLOC_HARDER) { > > page = __rmqueue_smallest(zone, order, MIGRATE_HIGHATOMIC); > > if (page) > > trace_mm_page_alloc_zone_locked(page, order, migratetype); > > But this condition is not clear to me. __rmqueue_smallest doesn't access > pcp lists. Maybe I have missed the point in the original discussion but > this deserves a comment at least. Before the pcplist skipping is applied, order-0 request can not reach here. But, now, an order-0 request can reach here. Free memory on MIGRATE_HIGHATOMIC is reserved for high-order atomic allocation so an order-0 request should skip it. I will add a code comment on the next version. Thanks. ^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [PATCH v2 for v5.9] mm/page_alloc: handle a missing case for memalloc_nocma_{save/restore} APIs 2020-09-29 8:38 ` Joonsoo Kim (?) @ 2020-09-29 9:53 ` Michal Hocko -1 siblings, 0 replies; 12+ messages in thread From: Michal Hocko @ 2020-09-29 9:53 UTC (permalink / raw) To: Joonsoo Kim Cc: Andrew Morton, Linux Memory Management List, LKML, Vlastimil Babka, Aneesh Kumar K . V, Mel Gorman, kernel-team, Joonsoo Kim On Tue 29-09-20 17:38:43, Joonsoo Kim wrote: > 2020년 9월 29일 (화) 오후 5:08, Michal Hocko <mhocko@suse.com>님이 작성: > > > > On Mon 28-09-20 17:50:46, Joonsoo Kim wrote: > > > From: Joonsoo Kim <iamjoonsoo.kim@lge.com> > > > > > > memalloc_nocma_{save/restore} APIs can be used to skip page allocation > > > on CMA area, but, there is a missing case and the page on CMA area could > > > be allocated even if APIs are used. This patch handles this case to fix > > > the potential issue. > > > > > > Missing case is an allocation from the pcplist. MIGRATE_MOVABLE pcplist > > > could have the pages on CMA area so we need to skip it if ALLOC_CMA isn't > > > specified. > > > > > > Fixes: 8510e69c8efe (mm/page_alloc: fix memalloc_nocma_{save/restore} APIs) > > > Signed-off-by: Joonsoo Kim <iamjoonsoo.kim@lge.com> > > > --- > > > mm/page_alloc.c | 13 ++++++++++--- > > > 1 file changed, 10 insertions(+), 3 deletions(-) > > > > > > diff --git a/mm/page_alloc.c b/mm/page_alloc.c > > > index fab5e97..104d2e1 100644 > > > --- a/mm/page_alloc.c > > > +++ b/mm/page_alloc.c > > > @@ -3367,9 +3367,16 @@ struct page *rmqueue(struct zone *preferred_zone, > > > struct page *page; > > > > > > if (likely(order == 0)) { > > > - page = rmqueue_pcplist(preferred_zone, zone, gfp_flags, > > > + /* > > > + * MIGRATE_MOVABLE pcplist could have the pages on CMA area and > > > + * we need to skip it when CMA area isn't allowed. > > > + */ > > > + if (!IS_ENABLED(CONFIG_CMA) || alloc_flags & ALLOC_CMA || > > > + migratetype != MIGRATE_MOVABLE) { > > > + page = rmqueue_pcplist(preferred_zone, zone, gfp_flags, > > > migratetype, alloc_flags); > > > - goto out; > > > + goto out; > > > + } > > > } > > > > This approach looks definitely better than the previous version. > > Thanks! > > > > > > > /* > > > @@ -3381,7 +3388,7 @@ struct page *rmqueue(struct zone *preferred_zone, > > > > > > do { > > > page = NULL; > > > - if (alloc_flags & ALLOC_HARDER) { > > > + if (order > 0 && alloc_flags & ALLOC_HARDER) { > > > page = __rmqueue_smallest(zone, order, MIGRATE_HIGHATOMIC); > > > if (page) > > > trace_mm_page_alloc_zone_locked(page, order, migratetype); > > > > But this condition is not clear to me. __rmqueue_smallest doesn't access > > pcp lists. Maybe I have missed the point in the original discussion but > > this deserves a comment at least. > > Before the pcplist skipping is applied, order-0 request can not reach here. > But, now, an order-0 request can reach here. Free memory on > MIGRATE_HIGHATOMIC is reserved for high-order atomic allocation > so an order-0 request should skip it. OK, I see. Thanks for the clarification. > I will add a code comment on the next version. Thanks, that would be indeed helpful. With that, feel free to add Acked-by: Michal Hocko <mhocko@suse.com> -- Michal Hocko SUSE Labs ^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [PATCH v2 for v5.9] mm/page_alloc: handle a missing case for memalloc_nocma_{save/restore} APIs 2020-09-28 8:50 [PATCH v2 for v5.9] mm/page_alloc: handle a missing case for memalloc_nocma_{save/restore} APIs js1304 2020-09-28 23:52 ` Andrew Morton 2020-09-29 8:08 ` Michal Hocko @ 2020-09-29 8:13 ` Vlastimil Babka 2 siblings, 0 replies; 12+ messages in thread From: Vlastimil Babka @ 2020-09-29 8:13 UTC (permalink / raw) To: js1304, Andrew Morton Cc: linux-mm, linux-kernel, Michal Hocko, Aneesh Kumar K . V, Mel Gorman, kernel-team, Joonsoo Kim On 9/28/20 10:50 AM, js1304@gmail.com wrote: > From: Joonsoo Kim <iamjoonsoo.kim@lge.com> > > memalloc_nocma_{save/restore} APIs can be used to skip page allocation > on CMA area, but, there is a missing case and the page on CMA area could > be allocated even if APIs are used. This patch handles this case to fix > the potential issue. > > Missing case is an allocation from the pcplist. MIGRATE_MOVABLE pcplist > could have the pages on CMA area so we need to skip it if ALLOC_CMA isn't > specified. > > Fixes: 8510e69c8efe (mm/page_alloc: fix memalloc_nocma_{save/restore} APIs) > Signed-off-by: Joonsoo Kim <iamjoonsoo.kim@lge.com> It's unfortunate, but hopefully we can still get the CMA in ZONE_MOVABLE one day and get rid of all of this again? :) For that reason I'd prefer simple solutions even if there's some potential overhead. I think those tests should be in the noise, and avoided completely with !CONFIG_CMA. Acked-by: Vlastimil Babka <vbabka@suse.cz> > --- > mm/page_alloc.c | 13 ++++++++++--- > 1 file changed, 10 insertions(+), 3 deletions(-) > > diff --git a/mm/page_alloc.c b/mm/page_alloc.c > index fab5e97..104d2e1 100644 > --- a/mm/page_alloc.c > +++ b/mm/page_alloc.c > @@ -3367,9 +3367,16 @@ struct page *rmqueue(struct zone *preferred_zone, > struct page *page; > > if (likely(order == 0)) { > - page = rmqueue_pcplist(preferred_zone, zone, gfp_flags, > + /* > + * MIGRATE_MOVABLE pcplist could have the pages on CMA area and > + * we need to skip it when CMA area isn't allowed. > + */ > + if (!IS_ENABLED(CONFIG_CMA) || alloc_flags & ALLOC_CMA || > + migratetype != MIGRATE_MOVABLE) { > + page = rmqueue_pcplist(preferred_zone, zone, gfp_flags, > migratetype, alloc_flags); > - goto out; > + goto out; > + } > } > > /* > @@ -3381,7 +3388,7 @@ struct page *rmqueue(struct zone *preferred_zone, > > do { > page = NULL; > - if (alloc_flags & ALLOC_HARDER) { > + if (order > 0 && alloc_flags & ALLOC_HARDER) { > page = __rmqueue_smallest(zone, order, MIGRATE_HIGHATOMIC); > if (page) > trace_mm_page_alloc_zone_locked(page, order, migratetype); > ^ permalink raw reply [flat|nested] 12+ messages in thread
end of thread, other threads:[~2020-09-29 9:53 UTC | newest] Thread overview: 12+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2020-09-28 8:50 [PATCH v2 for v5.9] mm/page_alloc: handle a missing case for memalloc_nocma_{save/restore} APIs js1304 2020-09-28 23:52 ` Andrew Morton 2020-09-29 1:28 ` Joonsoo Kim 2020-09-29 1:28 ` Joonsoo Kim 2020-09-29 4:50 ` Andrew Morton 2020-09-29 6:46 ` Joonsoo Kim 2020-09-29 6:46 ` Joonsoo Kim 2020-09-29 8:08 ` Michal Hocko 2020-09-29 8:38 ` Joonsoo Kim 2020-09-29 8:38 ` Joonsoo Kim 2020-09-29 9:53 ` Michal Hocko 2020-09-29 8:13 ` Vlastimil Babka
This is an external index of several public inboxes, see mirroring instructions on how to clone and mirror all data and code used by this external index.