linux-kernel.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
From: Andrea Reale <ar@linux.vnet.ibm.com>
To: Xishi Qiu <qiuxishi@huawei.com>
Cc: Maciej Bielski <m.bielski@virtualopensystems.com>,
	scott.branden@broadcom.com, will.deacon@arm.com,
	tech@virtualopensystems.com,
	linux-arm-kernel@lists.infradead.org,
	linux-kernel@vger.kernel.org
Subject: Re: [RFC PATCH] Memory hotplug support for arm64 platform
Date: Thu, 15 Dec 2016 18:31:55 +0000	[thread overview]
Message-ID: <20161215183154.GC4806@samekh> (raw)
In-Reply-To: <58523AC5.10800@huawei.com>

Hi Xishi Qiu,

thanks for your comments. 

The short anwser to your question is the following.  As you hinted,
it is related to the way pfn_valid() is implemented in arm64 when
CONFIG_HAVE_ARCH_PFN_VALID is true (default), i.e., just a check for
the NOMAP flags on the corresponding memblocks.

Since arch_add_memory->__add_pages() expects pfn_valid() to return false
when it is first called, we mark corresponding memory blocks with NOMAP;
however, arch_add_memory->__add_pages()->__add_section()->__add_zone()
expects pfn_valid() to return true when, at the end of its body,
it cycles through pages to call SetPageReserved(). Since blocks are
marked with NOMAP, pages will not be reserved there, henceforth we
need to reserve them after we clear the NOMAP flag inside the body of
arch_add_memory. Having pages reserved at the end of arch_add_memory
is a preconditions for the upcoming onlining of memory blocks (see
memory_block_change_state()).

> It's because that in memmap_init_zone() -> early_pfn_valid(), the new page is still
> invalid, so we need to init it after memblock_clear_nomap()
> 
> So why not use __init_single_page() and set_pageblock_migratetype()?

About your comment on memmap_init_zone()->early_pfn_valid(), I think
that that particular check is never executed in the context of memory
hotplug; in fact, just before the call to early_pfn_valid(), the code
will jump to the `not_early` label, because the context == MEMMAP_HOPTLUG.

Now, the question would be: why there's no need for this hack in
implementation for memory hotplug for other architectures (e.g.,
x86)? The answer we have found lies in the following: as said above,
when CONFIG_HAVE_ARCH_PFN_VALID is true (default), the implementation
of pfn_valid for arm64 just checks the NOMAP flag on the memmblocks,
and returns true if corresponding memblock_add_node() has succeeded (it
seems it does not consider the sparse memory model structures created
by arm64_memory_present() and sparse_init()).

On the contrary, the implementation of pfn_valid() for other
architectures is defined in include/linux/mmzone.h. This
implemenation, among other things, checks for the
SECTION_HAS_MEM_MAP flag on section->section_mem_map. Now,
when we go to memory hotplug, this flag is actually set inside
__add_section()->sparse_add_one_section()->sparse_init_one_section(). This
happens before the call to __add_zone(), so that, when it is eventually
invoked, pfn_valid() would return true and pages would be correctly
reserved.  On the contrary, on arm64, it will keep returning false
because of the different implementation of pfn_valid().

Given the above, we believed it was cleaner to go for this little and
well-confined hack rather then possibly changing the logic of pfn_valid.

However, we are very open for discussion and suggestions for improvement.
In particular, any advices on how to effectively modify pfn_valid()
without breaking existing functionalities are very welcome.

Thanks,
Andrea

  reply	other threads:[~2016-12-15 18:32 UTC|newest]

Thread overview: 12+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2016-12-14 12:16 Maciej Bielski
2016-12-15  6:18 ` Xishi Qiu
2016-12-15  6:40   ` Xishi Qiu
2016-12-15 18:31     ` Andrea Reale [this message]
2016-12-16  1:31       ` Xishi Qiu
2016-12-20 19:12 ` Scott Branden
2016-12-21  9:44   ` Maciej Bielski
2016-12-22  1:20     ` Scott Branden
2017-02-06 11:17       ` Andrea Reale
2017-02-08 20:08         ` Scott Branden
2017-03-30  0:40         ` Florian Fainelli
2017-03-31 14:16           ` Andrea Reale

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=20161215183154.GC4806@samekh \
    --to=ar@linux.vnet.ibm.com \
    --cc=linux-arm-kernel@lists.infradead.org \
    --cc=linux-kernel@vger.kernel.org \
    --cc=m.bielski@virtualopensystems.com \
    --cc=qiuxishi@huawei.com \
    --cc=scott.branden@broadcom.com \
    --cc=tech@virtualopensystems.com \
    --cc=will.deacon@arm.com \
    --subject='Re: [RFC PATCH] Memory hotplug support for arm64 platform' \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).