On Wed, Dec 17, 2014 at 03:56:08PM -0800, David Rientjes wrote:
> On Mon, 15 Dec 2014, akpm@xxxxxxxxxxxxxxxxxxxx wrote:
>
> > From: Weijie Yang <weijie.yang@xxxxxxxxxxx>
> > Subject: mm: page_isolation: check pfn validity before access
> >
> > In the undo path of start_isolate_page_range(), we need to check the pfn
> > validity before accessing its page, or it will trigger an addressing
> > exception if there is hole in the zone.
> >
> > This issue is found by code-review not a test-trigger. In
> > "CONFIG_HOLES_IN_ZONE" environment, there is a certain chance that it
> > would casue an addressing exception when start_isolate_page_range()
> > fails, this could affect CMA, hugepage and memory-hotplug function.
> >
> > Signed-off-by: Weijie Yang <weijie.yang@xxxxxxxxxxx>
> > Acked-by: Michal Nazarewicz <mina86@xxxxxxxxxx>
> > Reviewed-by: Yasuaki Ishimatsu <isimatu.yasuaki@xxxxxxxxxxxxxx>
> > Cc: KAMEZAWA Hiroyuki <kamezawa.hiroyu@xxxxxxxxxxxxxx>
> > Cc: Minchan Kim <minchan@xxxxxxxxxx>
> > Cc: Mel Gorman <mel@xxxxxxxxx>
> > Cc: Michal Hocko <mhocko@xxxxxxx>
> > Cc: <stable@xxxxxxxxxxxxxxx>
> > Signed-off-by: Andrew Morton <akpm@xxxxxxxxxxxxxxxxxxxx>
> > ---
> >
> > mm/page_isolation.c | 7 +++++--
> > 1 file changed, 5 insertions(+), 2 deletions(-)
> >
> > diff -puN mm/page_isolation.c~mm-page_isolation-check-pfn-validity-before-access mm/page_isolation.c
> > --- a/mm/page_isolation.c~mm-page_isolation-check-pfn-validity-before-access
> > +++ a/mm/page_isolation.c
> > @@ -176,8 +176,11 @@ int start_isolate_page_range(unsigned lo
> > undo:
> > for (pfn = start_pfn;
> > pfn < undo_pfn;
> > - pfn += pageblock_nr_pages)
> > - unset_migratetype_isolate(pfn_to_page(pfn), migratetype);
> > + pfn += pageblock_nr_pages) {
> > + page = __first_valid_page(pfn, pageblock_nr_pages);
> > + if (page)
> > + unset_migratetype_isolate(page, migratetype);
> > + }
> >
> > return -EBUSY;
> > }
>
> This is such an interesting patch because of who acked it and the two
> callers of the function that seem to want different behavior.
>
> The behavior of start_isolate_page_range() is currently to either set the
> migratetype of the pageblocks to MIGRATE_ISOLATE or allow the pageblocks
> to have no valid pages due to a memory hole.
>
> The memory hotplug usecase makes perfect sense since it's entirely
> legitimate to offline memory holes and we would not want to return -EBUSY,
> but that doesn't seem to be what the implementation of
> start_isolate_page_range() is this undo behavior expects pfn_to_page(pfn)
> to be valid up to undo_pfn.
>
> I'm not a CMA expert, but I'm surprised that we want to return success
> here if some pageblocks are actually memory holes. Don't we want to
> return -EBUSY for such a range? That seems to be more in line with the
> comment for start_isolate_page_range() which specifies it returns "-EBUSY
> if any part of range cannot be isolated", which would seem to imply memory
> holes as well, but that doesn't match its implementation.
Can CMA have memory hole?
CMA user should allocate CMA area with cma_declare_contiguous which uses
memblock. I'm not familiar with memblock but I don't think it's possible.
>
> So there's two radically different expectations for this function with
> regard to invalid pfns. Which one do we want?
>
> If we want it to simply disregard memory holes (memory hotplug), then ack
> the patch with a follow-up to fix the comment. If we want it to undo on
> memory holes (CMA), then nack the patch since its current implementation
> is correct and we need to fix memory hotplug.
--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to majordomo@xxxxxxxxx. For more info on Linux MM,
see: http://www.linux-mm.org/ .
Don't email: <a href=mailto:"dont@xxxxxxxxx";> email@xxxxxxxxx </a>