Messages in this thread | | | Date | Mon, 24 Jan 2022 10:55:43 +0100 | From | Oscar Salvador <> | Subject | Re: [PATCH v4 3/7] mm: page_isolation: check specified range for unmovable pages |
| |
On 2022-01-19 20:06, Zi Yan wrote: > From: Zi Yan <ziy@nvidia.com> > > Enable set_migratetype_isolate() to check specified sub-range for > unmovable pages during isolation. Page isolation is done > at max(MAX_ORDER_NR_PAEGS, pageblock_nr_pages) granularity, but not all > pages within that granularity are intended to be isolated. For example, > alloc_contig_range(), which uses page isolation, allows ranges without > alignment. This commit makes unmovable page check only look for > interesting pages, so that page isolation can succeed for any > non-overlapping ranges.
Hi Zi Yan,
I had to re-read this several times as I found this a bit misleading. I was mainly confused by the fact that memory_hotplug does isolation on PAGES_PER_SECTION granularity, and reading the above seems to indicate that either do it at MAX_ORDER_NR_PAGES or at pageblock_nr_pages granularity.
True is that start_isolate_page_range() expects the range to be pageblock aligned and works in pageblock_nr_pages chunks, but I do not think that is what you meant to say here.
Now, to the change itself, below:
> @@ -47,8 +51,8 @@ static struct page *has_unmovable_pages(struct zone > *zone, struct page *page, > return page; > } > > - for (; iter < pageblock_nr_pages - offset; iter++) { > - page = pfn_to_page(pfn + iter); > + for (pfn = first_pfn; pfn < last_pfn; pfn++) {
You already did pfn = first_pfn before.
> /** > * start_isolate_page_range() - make page-allocation-type of range of > pages to > * be MIGRATE_ISOLATE. > - * @start_pfn: The lower PFN of the range to be isolated. > - * @end_pfn: The upper PFN of the range to be isolated. > + * @start_pfn: The lower PFN of the range to be checked for > + * possibility of isolation. > + * @end_pfn: The upper PFN of the range to be checked for > + * possibility of isolation. > + * @isolate_start: The lower PFN of the range to be isolated. > + * @isolate_end: The upper PFN of the range to be isolated.
So, what does "possibility" means here. I think this need to be clarified a bit better.
From what you pointed out in the commit message I think what you are doing is:
- alloc_contig_range() gets a range to be isolated. - then you pass two ranges to start_isolate_page_range() (start_pfn, end_pfn]: which is the unaligned range you got in alloc_contig_range() (isolate_start, isolate_end]: which got aligned to, let's say, to MAX_ORDER_NR_PAGES
Now, most likely, (start_pfn, end_pfn] only covers a sub-range of (isolate_start, isolate_end], and that sub-range is what you really want to isolate (so (start_pfn, end_pfn])?
If so, should not the names be reversed?
-- Oscar Salvador SUSE Labs
| |