Re: [PATCH] mm, isolation: avoid checking unmovable pages across pageblock boundary

[David Hildenbrand <david@redhat.com>]

On 25.08.20 10:26, Michal Hocko wrote:
> On Mon 24-08-20 14:58:11, Li Xinhai wrote:
>> In has_unmovable_pages(), the page parameter would not always be the
>> first page within a pageblock (see how the page pointer is passed in from
>> start_isolate_page_range() after call __first_valid_page()), so that
>> would cause checking unmovable pages span two pageblocks.
> 
> This might lead to false negatives when an unrelated block would cause
> an isolation failure.
> 
>> After this patch, the checking is enforced within one pageblock no matter
>> the page is first one or not, and obey the semantics of this function.
>>
>> This issue is found by code inspection.
>>
>> Signed-off-by: Li Xinhai <lixinhai.lxh@gmail.com>
> 
> I have tried to find the commit which has introduced this but there was
> so much churn around that I gave up. Not that we care all that much
> because it seems that we simply never try to isolate pageblocks with
> holes. Memory hotplug disallows that explicitly and the CMA allocator
> doesn't trip over that either. Or maybe we were just lucky or a silent
> failure didn't really trigger any attention.

It will never happen.

- memory offlining excludes ranges without holes.
- virtio-mem (alloc_contig_range()) never has ranges with holes.
- gigantic pages (alloc_contig_range()) never use ranges with holes
- CMA (alloc_contig_range()) never uses ranges with holes.

Trying to allocate something that's not there would be troublesome already.

I *guess* the handling is in place for corner cases of
alloc_contig_range(), whereby one tries to allocate a sub-MAX_ORDER-1
page, and  alloc_contig_range() automatically tries to isolate all
pageblocks spanning full MAX_ORDER-1 pages. See
pfn_max_align_down/pfn_max_align_up. If there would be a memory hole
close by, that could trigger.

It would, however, only trigger for CMA, as that is the only user
allocating in sub-MAX_ORDER-1 granularity right now.

I think we should just disallow/check for holes in the extended range
(pfn_max_align_down/pfn_max_align_up) in alloc_contig_range() and
document the expected behavior for start_isolate_page_range() - to not
contain any holes. This get's rid of a whole bunch of unnecessary
pfn_to_online_page() calls.

We would no longer able to alloc_contig_range() pages close to a memory
hole in case of CMA (in corner cases) - but I doubt this is an actual issue.

If we agree, I can send patches.

-- 
Thanks,

David / dhildenb