Re: [PATCH v1 1/6] mm/page_alloc: tweak comments in has_unmovable_pages()

Baoquan He
2020-07-28T13:48:47Z

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
>
>