Re: [PATCH v2 1/3] mm/shuffle: don't move pages between zones and don't read garbage memmaps

From: David Hildenbrand <david@redhat.com>
To: Andrew Morton <akpm@linux-foundation.org>,
	Wei Yang <richard.weiyang@linux.alibaba.com>
Cc: Wei Yang <richard.weiyang@gmail.com>,
	linux-kernel@vger.kernel.org, linux-mm@kvack.org,
	Michal Hocko <mhocko@suse.com>,
	stable@vger.kernel.org, Johannes Weiner <hannes@cmpxchg.org>,
	Minchan Kim <minchan@kernel.org>,
	Huang Ying <ying.huang@intel.com>,
	Mel Gorman <mgorman@techsingularity.net>,
	Dan Williams <dan.j.williams@intel.com>
Subject: Re: [PATCH v2 1/3] mm/shuffle: don't move pages between zones and don't read garbage memmaps
Date: Fri, 24 Jul 2020 10:20:23 +0200	[thread overview]
Message-ID: <d16a2f0f-6150-d41b-f44c-96e8497bee72@redhat.com> (raw)
In-Reply-To: <20200723200846.768513d7c122ac11b6e73538@linux-foundation.org>

On 24.07.20 05:08, Andrew Morton wrote:
> On Tue, 23 Jun 2020 17:30:18 +0800 Wei Yang <richard.weiyang@linux.alibaba.com> wrote:
> 
>> On Tue, Jun 23, 2020 at 09:55:43AM +0200, David Hildenbrand wrote:
>>> On 23.06.20 09:39, David Hildenbrand wrote:
>>>>> Hmm.. I thought this is the behavior for early section, while it looks current
>>>>> code doesn't work like this:
>>>>>
>>>>>        if (section_is_early && memmap)
>>>>>                free_map_bootmem(memmap);
>>>>>        else
>>>>> 	       depopulate_section_memmap(pfn, nr_pages, altmap);
>>>>>
>>>>> section_is_early is always "true" for early section, while memmap is not-NULL
>>>>> only when sub-section map is empty.
>>>>>
>>>>> If my understanding is correct, when we remove a sub-section in early section,
>>>>> the code would call depopulate_section_memmap(), which in turn free related
>>>>> memmap. By removing the memmap, the return value from pfn_to_online_page() is
>>>>> not a valid one.
>>>>
>>>> I think you're right, and pfn_valid() would also return true, as it is
>>>> an early section. This looks broken.
>>>>
>>>>>
>>>>> Maybe we want to write the code like this:
>>>>>
>>>>>        if (section_is_early)
>>>>>                if (memmap)
>>>>>                        free_map_bootmem(memmap);
>>>>>        else
>>>>> 	       depopulate_section_memmap(pfn, nr_pages, altmap);
>>>>>
>>>>
>>>> I guess that should be the way to go
>>>>
>>>> @Dan, I think what Wei proposes here is correct, right? Or how does it
>>>> work in the VMEMMAP case with early sections?
>>>>
>>>
>>> Especially, if you would re-hot-add, section_activate() would assume
>>> there is a memmap, it must not be removed.
>>>
>>
>> You are right here. I didn't notice it.
>>
>>> @Wei, can you send a patch?
>>>
>>
>> Sure, let me prepare for it.
> 
> Still awaiting this, and the v3 patch was identical to this v2 patch.
> 
> It's tagged for -stable, so there's some urgency.  Should we just go
> ahead with the decently-tested v2?

This patch (mm/shuffle: don't move pages between zones and don't read
garbage memmaps) is good enough for upstream. While the issue reported
by Wei was valid (and needs to be fixed), the user in this patch is just
one of many affected users. Nothing special.

-- 
Thanks,

David / dhildenb