On Mon 17-02-20 19:20:54, Baoquan He wrote:
> On 02/17/20 at 11:38am, David Hildenbrand wrote:
> > On 17.02.20 11:33, Baoquan He wrote:
> > > On 02/17/20 at 11:24am, David Hildenbrand wrote:
> > >> On 17.02.20 11:13, Baoquan He wrote:
> > >>> On 02/17/20 at 10:34am, Oscar Salvador wrote:
> > >>>> On Mon, Feb 17, 2020 at 02:46:27PM +0900, kkabe@xxxxxxxxxxx wrote:
> > >>>>> ===========================================
> > >>>>> struct page * __meminit populate_section_memmap(unsigned long pfn,
> > >>>>> unsigned long nr_pages, int nid, struct vmem_altmap *altmap)
> > >>>>> {
> > >>>>> struct page *page, *ret;
> > >>>>> unsigned long memmap_size = sizeof(struct page) * PAGES_PER_SECTION;
> > >>>>>
> > >>>>> page = alloc_pages(GFP_KERNEL|__GFP_NOWARN, get_order(memmap_size));
> > >>>>> if (page) {
> > >>>>> goto got_map_page;
> > >>>>> }
> > >>>>> pr_info("%s: alloc_pages() returned 0x%p (should be 0), reverting to vmalloc(memmap_size=%lu)\n", __func__, page, memmap_size);
> > >>>>> BUG_ON(page != 0);
> > >>>>>
> > >>>>> ret = vmalloc(memmap_size);
> > >>>>> pr_info("%s: vmalloc(%lu) returned 0x%p\n", __func__, memmap_size, ret);
> > >>>>> if (ret) {
> > >>>>> goto got_map_ptr;
> > >>>>> }
> > >>>>>
> > >>>>> return NULL;
> > >>>>> got_map_page:
> > >>>>> ret = (struct page *)pfn_to_kaddr(page_to_pfn(page));
> > >>>>> pr_info("%s: allocated struct page *page=0x%p\n", __func__, page);
> > >>>>> got_map_ptr:
> > >>>>>
> > >>>>> pr_info("%s: returning struct page * =0x%p\n", __func__, ret);
> > >>>>> return ret;
> > >>>>> }
> > >>>>
> > >>>> Could you please replace %p with %px. Wih the first, pointers are hashed so it is trickier
> > >>>> to get an overview of the meaning.
> > >>>>
> > >>>> David could be right about ZONE_NORMAL vs ZONE_HIGHMEM.
> > >>>> IIUC, default_kernel_zone_for_pfn and default_zone_for_pfn seem to only deal with
> > >>>> (ZONE_DMA,ZONE_NORMAL] or ZONE_MOVABLE.
> > >>>
> > >>> Ah, I think you both have spotted the problem.
> > >>>
> > >>> In i386, if w/o momory hot add, normal memory will only include those
> > >>> below 896M and they are added into normal zone. The left are added into
> > >>> highmem zone.
> > >>>
> > >>> How this influence the page allocation?
> > >>>
> > >>> Very huge. As we know, in i386, normal memory can be accessed with
> > >>> virt_to_phys, namely PAGE_OFFSET + phys. But highmem has to be accessed
> > >>> with kmap. However, the later hot added memory are all put into normal
> > >>> memmory, accessing into them will stump into vmalloc area, I would say.
> > >>>
> > >>> So, i386 doesn't support memory hot add well. Not sure if below change
> > >>> can make it work normally.
> > >>>
>
> Please try below code instead, see if it works. However, as David and
> and Michal said in other reply, if no real use case, we may not be so
> eager to support mem hotplug on i386.
Yes please. Can we just mark it broken until there is a real usecase?
Convoluting the code even more for something that is not in use is just
adding a maintenance burden and the memory hotplug is seriously
understuffed in man power already.
This is likely a fallout of the hotplug rework (c6f03e2903c9e) from 2
years ago. I cannot really say whether the code worked reasonably before
the rework because I never considered hotplug on 32b to be something to
even try TBH. Mostly because lowmem is unlikely to ever benefit from
hotplug and adding more highmem just makes all the lowmem problems even
worse so this is dubious in itself.
That being said, I am willing to investigate further if there is a real
usecase for this but considering that nobody has noticed the breakage in
almost 3 years then I simply suspect that this is not really interesting
and marking it explicitly BROKEN is a better option.
--
Michal Hocko
SUSE Labs
