| Message ID | 20200630142639.22770-2-david@redhat.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | mm / virtio-mem: support ZONE_MOVABLE | expand |
On 06/30/20 at 04:26pm, David Hildenbrand wrote: > Let's move the split comment regarding bootmem allocations and memory > holes, especially in the context of ZONE_MOVABLE, to the PageReserved() > check. > > Cc: Andrew Morton <akpm@linux-foundation.org> > Cc: Michal Hocko <mhocko@suse.com> > Cc: Michael S. Tsirkin <mst@redhat.com> > Signed-off-by: David Hildenbrand <david@redhat.com> > --- > mm/page_alloc.c | 22 ++++++---------------- > 1 file changed, 6 insertions(+), 16 deletions(-) > > diff --git a/mm/page_alloc.c b/mm/page_alloc.c > index 48eb0f1410d47..bd3ebf08f09b9 100644 > --- a/mm/page_alloc.c > +++ b/mm/page_alloc.c > @@ -8207,14 +8207,6 @@ struct page *has_unmovable_pages(struct zone *zone, struct page *page, > unsigned long iter = 0; > unsigned long pfn = page_to_pfn(page); > > - /* > - * TODO we could make this much more efficient by not checking every > - * page in the range if we know all of them are in MOVABLE_ZONE and > - * that the movable zone guarantees that pages are migratable but > - * the later is not the case right now unfortunatelly. E.g. movablecore > - * can still lead to having bootmem allocations in zone_movable. > - */ > - > if (is_migrate_cma_page(page)) { > /* > * CMA allocations (alloc_contig_range) really need to mark > @@ -8233,6 +8225,12 @@ struct page *has_unmovable_pages(struct zone *zone, struct page *page, > > page = pfn_to_page(pfn + iter); > > + /* > + * Both, bootmem allocations and memory holes are marked > + * PG_reserved and are unmovable. We can even have unmovable > + * allocations inside ZONE_MOVABLE, for example when > + * specifying "movable_core". ~~~~ should be 'movablecore', we don't have kernel parameter 'movable_core'. Otherwise, this looks good to me. Esp the code comment at below had been added very long time ago and obsolete. Reviewed-by: Baoquan He <bhe@redhat.com> By the way, David, do you know what is the situation of having unmovable allocations inside ZONE_MOVABLE when specifying 'movablecore'? I quickly went through find_zone_movable_pfns_for_nodes(), but didn't get why. Could you tell a little more detail about it? Thanks Baoquan > + */ > if (PageReserved(page)) > return page; > > @@ -8306,14 +8304,6 @@ struct page *has_unmovable_pages(struct zone *zone, struct page *page, > * it. But now, memory offline itself doesn't call > * shrink_node_slabs() and it still to be fixed. > */ > - /* > - * If the page is not RAM, page_count()should be 0. > - * we don't need more check. This is an _used_ not-movable page. > - * > - * The problematic thing here is PG_reserved pages. PG_reserved > - * is set to both of a memory hole page and a _used_ kernel > - * page at boot. > - */ > return page; > } > return NULL; > -- > 2.26.2 > >
On 28.07.20 15:48, Baoquan He wrote: > On 06/30/20 at 04:26pm, David Hildenbrand wrote: >> Let's move the split comment regarding bootmem allocations and memory >> holes, especially in the context of ZONE_MOVABLE, to the PageReserved() >> check. >> >> Cc: Andrew Morton <akpm@linux-foundation.org> >> Cc: Michal Hocko <mhocko@suse.com> >> Cc: Michael S. Tsirkin <mst@redhat.com> >> Signed-off-by: David Hildenbrand <david@redhat.com> >> --- >> mm/page_alloc.c | 22 ++++++---------------- >> 1 file changed, 6 insertions(+), 16 deletions(-) >> >> diff --git a/mm/page_alloc.c b/mm/page_alloc.c >> index 48eb0f1410d47..bd3ebf08f09b9 100644 >> --- a/mm/page_alloc.c >> +++ b/mm/page_alloc.c >> @@ -8207,14 +8207,6 @@ struct page *has_unmovable_pages(struct zone *zone, struct page *page, >> unsigned long iter = 0; >> unsigned long pfn = page_to_pfn(page); >> >> - /* >> - * TODO we could make this much more efficient by not checking every >> - * page in the range if we know all of them are in MOVABLE_ZONE and >> - * that the movable zone guarantees that pages are migratable but >> - * the later is not the case right now unfortunatelly. E.g. movablecore >> - * can still lead to having bootmem allocations in zone_movable. >> - */ >> - >> if (is_migrate_cma_page(page)) { >> /* >> * CMA allocations (alloc_contig_range) really need to mark >> @@ -8233,6 +8225,12 @@ struct page *has_unmovable_pages(struct zone *zone, struct page *page, >> >> page = pfn_to_page(pfn + iter); >> >> + /* >> + * Both, bootmem allocations and memory holes are marked >> + * PG_reserved and are unmovable. We can even have unmovable >> + * allocations inside ZONE_MOVABLE, for example when >> + * specifying "movable_core". > ~~~~ should be 'movablecore', we don't > have kernel parameter 'movable_core'. Agreed! > > Otherwise, this looks good to me. Esp the code comment at below had been > added very long time ago and obsolete. > > Reviewed-by: Baoquan He <bhe@redhat.com> > > By the way, David, do you know what is the situation of having unmovable > allocations inside ZONE_MOVABLE when specifying 'movablecore'? I quickly > went through find_zone_movable_pfns_for_nodes(), but didn't get why. > Could you tell a little more detail about it? As far as I understand, it can happen that we have memblock allocations during boot that fall into an area the kernel later configures to span the movable zone (via movable_core). > > Thanks > Baoquan
On 07/28/20 at 04:07pm, David Hildenbrand wrote: > On 28.07.20 15:48, Baoquan He wrote: > > On 06/30/20 at 04:26pm, David Hildenbrand wrote: > >> Let's move the split comment regarding bootmem allocations and memory > >> holes, especially in the context of ZONE_MOVABLE, to the PageReserved() > >> check. > >> > >> Cc: Andrew Morton <akpm@linux-foundation.org> > >> Cc: Michal Hocko <mhocko@suse.com> > >> Cc: Michael S. Tsirkin <mst@redhat.com> > >> Signed-off-by: David Hildenbrand <david@redhat.com> > >> --- > >> mm/page_alloc.c | 22 ++++++---------------- > >> 1 file changed, 6 insertions(+), 16 deletions(-) > >> > >> diff --git a/mm/page_alloc.c b/mm/page_alloc.c > >> index 48eb0f1410d47..bd3ebf08f09b9 100644 > >> --- a/mm/page_alloc.c > >> +++ b/mm/page_alloc.c > >> @@ -8207,14 +8207,6 @@ struct page *has_unmovable_pages(struct zone *zone, struct page *page, > >> unsigned long iter = 0; > >> unsigned long pfn = page_to_pfn(page); > >> > >> - /* > >> - * TODO we could make this much more efficient by not checking every > >> - * page in the range if we know all of them are in MOVABLE_ZONE and > >> - * that the movable zone guarantees that pages are migratable but > >> - * the later is not the case right now unfortunatelly. E.g. movablecore > >> - * can still lead to having bootmem allocations in zone_movable. > >> - */ > >> - > >> if (is_migrate_cma_page(page)) { > >> /* > >> * CMA allocations (alloc_contig_range) really need to mark > >> @@ -8233,6 +8225,12 @@ struct page *has_unmovable_pages(struct zone *zone, struct page *page, > >> > >> page = pfn_to_page(pfn + iter); > >> > >> + /* > >> + * Both, bootmem allocations and memory holes are marked > >> + * PG_reserved and are unmovable. We can even have unmovable > >> + * allocations inside ZONE_MOVABLE, for example when > >> + * specifying "movable_core". > > ~~~~ should be 'movablecore', we don't > > have kernel parameter 'movable_core'. > > Agreed! > > > > > Otherwise, this looks good to me. Esp the code comment at below had been > > added very long time ago and obsolete. > > > > Reviewed-by: Baoquan He <bhe@redhat.com> > > > > By the way, David, do you know what is the situation of having unmovable > > allocations inside ZONE_MOVABLE when specifying 'movablecore'? I quickly > > went through find_zone_movable_pfns_for_nodes(), but didn't get why. > > Could you tell a little more detail about it? > > As far as I understand, it can happen that we have memblock allocations > during boot that fall into an area the kernel later configures to span > the movable zone (via movable_core). Seems yes, thanks a lot. Wondering who is still using movablecore|kernelcore in what use case.
On 29.07.20 12:47, Baoquan He wrote: > On 07/28/20 at 04:07pm, David Hildenbrand wrote: >> On 28.07.20 15:48, Baoquan He wrote: >>> On 06/30/20 at 04:26pm, David Hildenbrand wrote: >>>> Let's move the split comment regarding bootmem allocations and memory >>>> holes, especially in the context of ZONE_MOVABLE, to the PageReserved() >>>> check. >>>> >>>> Cc: Andrew Morton <akpm@linux-foundation.org> >>>> Cc: Michal Hocko <mhocko@suse.com> >>>> Cc: Michael S. Tsirkin <mst@redhat.com> >>>> Signed-off-by: David Hildenbrand <david@redhat.com> >>>> --- >>>> mm/page_alloc.c | 22 ++++++---------------- >>>> 1 file changed, 6 insertions(+), 16 deletions(-) >>>> >>>> diff --git a/mm/page_alloc.c b/mm/page_alloc.c >>>> index 48eb0f1410d47..bd3ebf08f09b9 100644 >>>> --- a/mm/page_alloc.c >>>> +++ b/mm/page_alloc.c >>>> @@ -8207,14 +8207,6 @@ struct page *has_unmovable_pages(struct zone *zone, struct page *page, >>>> unsigned long iter = 0; >>>> unsigned long pfn = page_to_pfn(page); >>>> >>>> - /* >>>> - * TODO we could make this much more efficient by not checking every >>>> - * page in the range if we know all of them are in MOVABLE_ZONE and >>>> - * that the movable zone guarantees that pages are migratable but >>>> - * the later is not the case right now unfortunatelly. E.g. movablecore >>>> - * can still lead to having bootmem allocations in zone_movable. >>>> - */ >>>> - >>>> if (is_migrate_cma_page(page)) { >>>> /* >>>> * CMA allocations (alloc_contig_range) really need to mark >>>> @@ -8233,6 +8225,12 @@ struct page *has_unmovable_pages(struct zone *zone, struct page *page, >>>> >>>> page = pfn_to_page(pfn + iter); >>>> >>>> + /* >>>> + * Both, bootmem allocations and memory holes are marked >>>> + * PG_reserved and are unmovable. We can even have unmovable >>>> + * allocations inside ZONE_MOVABLE, for example when >>>> + * specifying "movable_core". >>> ~~~~ should be 'movablecore', we don't >>> have kernel parameter 'movable_core'. >> >> Agreed! >> >>> >>> Otherwise, this looks good to me. Esp the code comment at below had been >>> added very long time ago and obsolete. >>> >>> Reviewed-by: Baoquan He <bhe@redhat.com> >>> >>> By the way, David, do you know what is the situation of having unmovable >>> allocations inside ZONE_MOVABLE when specifying 'movablecore'? I quickly >>> went through find_zone_movable_pfns_for_nodes(), but didn't get why. >>> Could you tell a little more detail about it? >> >> As far as I understand, it can happen that we have memblock allocations >> during boot that fall into an area the kernel later configures to span >> the movable zone (via movable_core). > > Seems yes, thanks a lot. Wondering who is still using > movablecore|kernelcore in what use case. > AFAIK, it's the only (guaranteed) way to get ZONE_MOVABLE without involving memory hotplug. As I learned, the movable zone is also interesting beyond memory hotunplug. It limits unmovable fragmentation and e.g., makes THP/huge pages more likely/easier to allocate.
diff --git a/mm/page_alloc.c b/mm/page_alloc.c index 48eb0f1410d47..bd3ebf08f09b9 100644 --- a/mm/page_alloc.c +++ b/mm/page_alloc.c @@ -8207,14 +8207,6 @@ struct page *has_unmovable_pages(struct zone *zone, struct page *page, unsigned long iter = 0; unsigned long pfn = page_to_pfn(page); - /* - * TODO we could make this much more efficient by not checking every - * page in the range if we know all of them are in MOVABLE_ZONE and - * that the movable zone guarantees that pages are migratable but - * the later is not the case right now unfortunatelly. E.g. movablecore - * can still lead to having bootmem allocations in zone_movable. - */ - if (is_migrate_cma_page(page)) { /* * CMA allocations (alloc_contig_range) really need to mark @@ -8233,6 +8225,12 @@ struct page *has_unmovable_pages(struct zone *zone, struct page *page, page = pfn_to_page(pfn + iter); + /* + * Both, bootmem allocations and memory holes are marked + * PG_reserved and are unmovable. We can even have unmovable + * allocations inside ZONE_MOVABLE, for example when + * specifying "movable_core". + */ if (PageReserved(page)) return page; @@ -8306,14 +8304,6 @@ struct page *has_unmovable_pages(struct zone *zone, struct page *page, * it. But now, memory offline itself doesn't call * shrink_node_slabs() and it still to be fixed. */ - /* - * If the page is not RAM, page_count()should be 0. - * we don't need more check. This is an _used_ not-movable page. - * - * The problematic thing here is PG_reserved pages. PG_reserved - * is set to both of a memory hole page and a _used_ kernel - * page at boot. - */ return page; } return NULL;
Let's move the split comment regarding bootmem allocations and memory holes, especially in the context of ZONE_MOVABLE, to the PageReserved() check. Cc: Andrew Morton <akpm@linux-foundation.org> Cc: Michal Hocko <mhocko@suse.com> Cc: Michael S. Tsirkin <mst@redhat.com> Signed-off-by: David Hildenbrand <david@redhat.com> --- mm/page_alloc.c | 22 ++++++---------------- 1 file changed, 6 insertions(+), 16 deletions(-)