Message ID | 1601387687-6077-4-git-send-email-anshuman.khandual@arm.com (mailing list archive) |
---|---|
State | New, archived |
Headers | show |
Series | arm64/mm/hotplug: Improve memory offline event notifier | expand |
Hi Anshuman, On 9/29/20 11:54 PM, Anshuman Khandual wrote: > This adds a validation function that scans the entire boot memory and makes > sure that all early memory sections are online. This check is essential for > the memory notifier to work properly, as it cannot prevent any boot memory > from offlining, if all sections are not online to begin with. The notifier > registration is skipped, if this validation does not go through. Although > the boot section scanning is selectively enabled with DEBUG_VM. > > Cc: Catalin Marinas <catalin.marinas@arm.com> > Cc: Will Deacon <will@kernel.org> > Cc: Mark Rutland <mark.rutland@arm.com> > Cc: Marc Zyngier <maz@kernel.org> > Cc: Steve Capper <steve.capper@arm.com> > Cc: Mark Brown <broonie@kernel.org> > Cc: linux-arm-kernel@lists.infradead.org > Cc: linux-kernel@vger.kernel.org > Signed-off-by: Anshuman Khandual <anshuman.khandual@arm.com> > --- > arch/arm64/mm/mmu.c | 59 +++++++++++++++++++++++++++++++++++++++++++++ > 1 file changed, 59 insertions(+) I don't understand why this is necessary. The core already ensure the corresponding section is online when trying to offline it. It's guranteed that section is online when the notifier is triggered. I'm not sure if there is anything I missed? > diff --git a/arch/arm64/mm/mmu.c b/arch/arm64/mm/mmu.c > index 90a30f5ebfc0..b67a657ea1ad 100644 > --- a/arch/arm64/mm/mmu.c > +++ b/arch/arm64/mm/mmu.c > @@ -1522,6 +1522,62 @@ static struct notifier_block prevent_bootmem_remove_nb = { > .notifier_call = prevent_bootmem_remove_notifier, > }; > > +/* > + * This ensures that boot memory sections on the plaltform are online ^^^^^^^^^ > + * during early boot. They could not be prevented from being offlined > + * if for some reason they are not brought online to begin with. This > + * help validate the basic assumption on which the above memory event > + * notifier works to prevent boot memory offlining and it's possible > + * removal. > + */ > +static bool validate_bootmem_online(void) > +{ > + struct memblock_region *mblk; > + struct mem_section *ms; > + unsigned long pfn, end_pfn, start, end; > + bool all_online = true; > + > + /* > + * Scanning across all memblock might be expensive > + * on some big memory systems. Hence enable this > + * validation only with DEBUG_VM. > + */ > + if (!IS_ENABLED(CONFIG_DEBUG_VM)) > + return all_online; > + > + for_each_memblock(memory, mblk) { > + pfn = PHYS_PFN(mblk->base); > + end_pfn = PHYS_PFN(mblk->base + mblk->size); > + It's not a good idea to access @mblk->{base, size}. There are two accessors: memblock_region_memory_{base, end}_pfn(). > + for (; pfn < end_pfn; pfn += PAGES_PER_SECTION) { > + ms = __pfn_to_section(pfn); > + > + /* > + * All memory ranges in the system at this point > + * should have been marked early sections. > + */ > + WARN_ON(!early_section(ms)); > + > + /* > + * Memory notifier mechanism here to prevent boot > + * memory offlining depends on the fact that each > + * early section memory on the system is intially > + * online. Otherwise a given memory section which > + * is already offline will be overlooked and can > + * be removed completely. Call out such sections. > + */ s/intially/initially > + if (!online_section(ms)) { > + start = PFN_PHYS(pfn); > + end = start + (1UL << PA_SECTION_SHIFT); > + pr_err("Memory range [%lx %lx] is offline\n", start, end); > + pr_err("Memory range [%lx %lx] can be removed\n", start, end); > + all_online = false; These two error messages can be combined: pr_err("Memory range [%lx %lx] not online, can't be offlined\n", start, end); I think you need to return @all_online immediately, without checking if the subsequent sections are online or not? :) > + } > + } > + } > + return all_online; > +} > + > static int __init prevent_bootmem_remove_init(void) > { > int ret = 0; > @@ -1529,6 +1585,9 @@ static int __init prevent_bootmem_remove_init(void) > if (!IS_ENABLED(CONFIG_MEMORY_HOTREMOVE)) > return ret; > > + if (!validate_bootmem_online()) > + return -EINVAL; > + > ret = register_memory_notifier(&prevent_bootmem_remove_nb); > if (ret) > pr_err("%s: Notifier registration failed %d\n", __func__, ret); > Cheers, Gavin
On 10/01/2020 06:23 AM, Gavin Shan wrote: > Hi Anshuman, > > On 9/29/20 11:54 PM, Anshuman Khandual wrote: >> This adds a validation function that scans the entire boot memory and makes >> sure that all early memory sections are online. This check is essential for >> the memory notifier to work properly, as it cannot prevent any boot memory >> from offlining, if all sections are not online to begin with. The notifier >> registration is skipped, if this validation does not go through. Although >> the boot section scanning is selectively enabled with DEBUG_VM. >> >> Cc: Catalin Marinas <catalin.marinas@arm.com> >> Cc: Will Deacon <will@kernel.org> >> Cc: Mark Rutland <mark.rutland@arm.com> >> Cc: Marc Zyngier <maz@kernel.org> >> Cc: Steve Capper <steve.capper@arm.com> >> Cc: Mark Brown <broonie@kernel.org> >> Cc: linux-arm-kernel@lists.infradead.org >> Cc: linux-kernel@vger.kernel.org >> Signed-off-by: Anshuman Khandual <anshuman.khandual@arm.com> >> --- >> arch/arm64/mm/mmu.c | 59 +++++++++++++++++++++++++++++++++++++++++++++ >> 1 file changed, 59 insertions(+) > > I don't understand why this is necessary. The core already ensure the > corresponding section is online when trying to offline it. It's guranteed > that section is online when the notifier is triggered. I'm not sure if > there is anything I missed? Current memory notifier blocks any boot memory hot removal attempt via blocking its offlining step itself. So if some sections in boot memory are not online (because of a bug or change in init sequence) by the time memory block device can be removed, the notifier loses the ability to prevent its removal. This validation here, ensures that entire boot memory is in online state, otherwise call out sections that are not, with an warning that those boot memory can be removed. > > >> diff --git a/arch/arm64/mm/mmu.c b/arch/arm64/mm/mmu.c >> index 90a30f5ebfc0..b67a657ea1ad 100644 >> --- a/arch/arm64/mm/mmu.c >> +++ b/arch/arm64/mm/mmu.c >> @@ -1522,6 +1522,62 @@ static struct notifier_block prevent_bootmem_remove_nb = { >> .notifier_call = prevent_bootmem_remove_notifier, >> }; >> +/* >> + * This ensures that boot memory sections on the plaltform are online Will fix. > ^^^^^^^^^ >> + * during early boot. They could not be prevented from being offlined >> + * if for some reason they are not brought online to begin with. This >> + * help validate the basic assumption on which the above memory event >> + * notifier works to prevent boot memory offlining and it's possible >> + * removal. >> + */ >> +static bool validate_bootmem_online(void) >> +{ >> + struct memblock_region *mblk; >> + struct mem_section *ms; >> + unsigned long pfn, end_pfn, start, end; >> + bool all_online = true; >> + >> + /* >> + * Scanning across all memblock might be expensive >> + * on some big memory systems. Hence enable this >> + * validation only with DEBUG_VM. >> + */ >> + if (!IS_ENABLED(CONFIG_DEBUG_VM)) >> + return all_online; >> + >> + for_each_memblock(memory, mblk) { >> + pfn = PHYS_PFN(mblk->base); >> + end_pfn = PHYS_PFN(mblk->base + mblk->size); >> + > > It's not a good idea to access @mblk->{base, size}. There are two > accessors: memblock_region_memory_{base, end}_pfn(). Sure, will replace. > >> + for (; pfn < end_pfn; pfn += PAGES_PER_SECTION) { >> + ms = __pfn_to_section(pfn); >> + >> + /* >> + * All memory ranges in the system at this point >> + * should have been marked early sections. >> + */ >> + WARN_ON(!early_section(ms)); >> + >> + /* >> + * Memory notifier mechanism here to prevent boot >> + * memory offlining depends on the fact that each >> + * early section memory on the system is intially >> + * online. Otherwise a given memory section which >> + * is already offline will be overlooked and can >> + * be removed completely. Call out such sections. >> + */ > > s/intially/initially Will change. > >> + if (!online_section(ms)) { >> + start = PFN_PHYS(pfn); >> + end = start + (1UL << PA_SECTION_SHIFT); >> + pr_err("Memory range [%lx %lx] is offline\n", start, end); >> + pr_err("Memory range [%lx %lx] can be removed\n", start, end); >> + all_online = false; > > These two error messages can be combined: > > pr_err("Memory range [%lx %lx] not online, can't be offlined\n", > start, end); Will change but it is actually s/can't be offlined/can be removed/ instead. > > I think you need to return @all_online immediately, without > checking if the subsequent sections are online or not? :) Thinking about this again. It might be better if the notifier registration does not depend on return value from validate_bootmem_online(). Instead it should proceed either way but after calling out all boot memory sections that are not online. In that case notifier will atleast prevent removal of some parts of boot memory which are online.
Hi Anshuman, On 10/6/20 2:11 PM, Anshuman Khandual wrote: > On 10/01/2020 06:23 AM, Gavin Shan wrote: >> On 9/29/20 11:54 PM, Anshuman Khandual wrote: >>> This adds a validation function that scans the entire boot memory and makes >>> sure that all early memory sections are online. This check is essential for >>> the memory notifier to work properly, as it cannot prevent any boot memory >>> from offlining, if all sections are not online to begin with. The notifier >>> registration is skipped, if this validation does not go through. Although >>> the boot section scanning is selectively enabled with DEBUG_VM. >>> >>> Cc: Catalin Marinas <catalin.marinas@arm.com> >>> Cc: Will Deacon <will@kernel.org> >>> Cc: Mark Rutland <mark.rutland@arm.com> >>> Cc: Marc Zyngier <maz@kernel.org> >>> Cc: Steve Capper <steve.capper@arm.com> >>> Cc: Mark Brown <broonie@kernel.org> >>> Cc: linux-arm-kernel@lists.infradead.org >>> Cc: linux-kernel@vger.kernel.org >>> Signed-off-by: Anshuman Khandual <anshuman.khandual@arm.com> >>> --- >>> arch/arm64/mm/mmu.c | 59 +++++++++++++++++++++++++++++++++++++++++++++ >>> 1 file changed, 59 insertions(+) >> >> I don't understand why this is necessary. The core already ensure the >> corresponding section is online when trying to offline it. It's guranteed >> that section is online when the notifier is triggered. I'm not sure if >> there is anything I missed? > > Current memory notifier blocks any boot memory hot removal attempt via > blocking its offlining step itself. So if some sections in boot memory > are not online (because of a bug or change in init sequence) by the > time memory block device can be removed, the notifier loses the ability > to prevent its removal. This validation here, ensures that entire boot > memory is in online state, otherwise call out sections that are not, > with an warning that those boot memory can be removed. > Well. I think it should be very rare. I guess you don't observe the errornous case so far? However, I think it's fine to add the check since it's only enabled with CONFIG_DEBUG_VM. >> >> >>> diff --git a/arch/arm64/mm/mmu.c b/arch/arm64/mm/mmu.c >>> index 90a30f5ebfc0..b67a657ea1ad 100644 >>> --- a/arch/arm64/mm/mmu.c >>> +++ b/arch/arm64/mm/mmu.c >>> @@ -1522,6 +1522,62 @@ static struct notifier_block prevent_bootmem_remove_nb = { >>> .notifier_call = prevent_bootmem_remove_notifier, >>> }; >>> +/* >>> + * This ensures that boot memory sections on the plaltform are online > > Will fix. > >> ^^^^^^^^^ >>> + * during early boot. They could not be prevented from being offlined >>> + * if for some reason they are not brought online to begin with. This >>> + * help validate the basic assumption on which the above memory event >>> + * notifier works to prevent boot memory offlining and it's possible >>> + * removal. >>> + */ >>> +static bool validate_bootmem_online(void) >>> +{ >>> + struct memblock_region *mblk; >>> + struct mem_section *ms; >>> + unsigned long pfn, end_pfn, start, end; >>> + bool all_online = true; >>> + >>> + /* >>> + * Scanning across all memblock might be expensive >>> + * on some big memory systems. Hence enable this >>> + * validation only with DEBUG_VM. >>> + */ >>> + if (!IS_ENABLED(CONFIG_DEBUG_VM)) >>> + return all_online; >>> + >>> + for_each_memblock(memory, mblk) { >>> + pfn = PHYS_PFN(mblk->base); >>> + end_pfn = PHYS_PFN(mblk->base + mblk->size); >>> + >> >> It's not a good idea to access @mblk->{base, size}. There are two >> accessors: memblock_region_memory_{base, end}_pfn(). > > Sure, will replace. > >> >>> + for (; pfn < end_pfn; pfn += PAGES_PER_SECTION) { >>> + ms = __pfn_to_section(pfn); >>> + >>> + /* >>> + * All memory ranges in the system at this point >>> + * should have been marked early sections. >>> + */ >>> + WARN_ON(!early_section(ms)); >>> + >>> + /* >>> + * Memory notifier mechanism here to prevent boot >>> + * memory offlining depends on the fact that each >>> + * early section memory on the system is intially >>> + * online. Otherwise a given memory section which >>> + * is already offline will be overlooked and can >>> + * be removed completely. Call out such sections. >>> + */ >> >> s/intially/initially > > Will change. > >> >>> + if (!online_section(ms)) { >>> + start = PFN_PHYS(pfn); >>> + end = start + (1UL << PA_SECTION_SHIFT); >>> + pr_err("Memory range [%lx %lx] is offline\n", start, end); >>> + pr_err("Memory range [%lx %lx] can be removed\n", start, end); >>> + all_online = false; >> >> These two error messages can be combined: >> >> pr_err("Memory range [%lx %lx] not online, can't be offlined\n", >> start, end); > > Will change but it is actually s/can't be offlined/can be removed/ instead. > >> >> I think you need to return @all_online immediately, without >> checking if the subsequent sections are online or not? :) > > Thinking about this again. It might be better if the notifier registration > does not depend on return value from validate_bootmem_online(). Instead it > should proceed either way but after calling out all boot memory sections > that are not online. In that case notifier will atleast prevent removal of > some parts of boot memory which are online. > Yes, agreed. However, the most important part is to print the errornous messages introduced in validate_bootmem_online(). Cheers, Gavin
diff --git a/arch/arm64/mm/mmu.c b/arch/arm64/mm/mmu.c index 90a30f5ebfc0..b67a657ea1ad 100644 --- a/arch/arm64/mm/mmu.c +++ b/arch/arm64/mm/mmu.c @@ -1522,6 +1522,62 @@ static struct notifier_block prevent_bootmem_remove_nb = { .notifier_call = prevent_bootmem_remove_notifier, }; +/* + * This ensures that boot memory sections on the plaltform are online + * during early boot. They could not be prevented from being offlined + * if for some reason they are not brought online to begin with. This + * help validate the basic assumption on which the above memory event + * notifier works to prevent boot memory offlining and it's possible + * removal. + */ +static bool validate_bootmem_online(void) +{ + struct memblock_region *mblk; + struct mem_section *ms; + unsigned long pfn, end_pfn, start, end; + bool all_online = true; + + /* + * Scanning across all memblock might be expensive + * on some big memory systems. Hence enable this + * validation only with DEBUG_VM. + */ + if (!IS_ENABLED(CONFIG_DEBUG_VM)) + return all_online; + + for_each_memblock(memory, mblk) { + pfn = PHYS_PFN(mblk->base); + end_pfn = PHYS_PFN(mblk->base + mblk->size); + + for (; pfn < end_pfn; pfn += PAGES_PER_SECTION) { + ms = __pfn_to_section(pfn); + + /* + * All memory ranges in the system at this point + * should have been marked early sections. + */ + WARN_ON(!early_section(ms)); + + /* + * Memory notifier mechanism here to prevent boot + * memory offlining depends on the fact that each + * early section memory on the system is intially + * online. Otherwise a given memory section which + * is already offline will be overlooked and can + * be removed completely. Call out such sections. + */ + if (!online_section(ms)) { + start = PFN_PHYS(pfn); + end = start + (1UL << PA_SECTION_SHIFT); + pr_err("Memory range [%lx %lx] is offline\n", start, end); + pr_err("Memory range [%lx %lx] can be removed\n", start, end); + all_online = false; + } + } + } + return all_online; +} + static int __init prevent_bootmem_remove_init(void) { int ret = 0; @@ -1529,6 +1585,9 @@ static int __init prevent_bootmem_remove_init(void) if (!IS_ENABLED(CONFIG_MEMORY_HOTREMOVE)) return ret; + if (!validate_bootmem_online()) + return -EINVAL; + ret = register_memory_notifier(&prevent_bootmem_remove_nb); if (ret) pr_err("%s: Notifier registration failed %d\n", __func__, ret);
This adds a validation function that scans the entire boot memory and makes sure that all early memory sections are online. This check is essential for the memory notifier to work properly, as it cannot prevent any boot memory from offlining, if all sections are not online to begin with. The notifier registration is skipped, if this validation does not go through. Although the boot section scanning is selectively enabled with DEBUG_VM. Cc: Catalin Marinas <catalin.marinas@arm.com> Cc: Will Deacon <will@kernel.org> Cc: Mark Rutland <mark.rutland@arm.com> Cc: Marc Zyngier <maz@kernel.org> Cc: Steve Capper <steve.capper@arm.com> Cc: Mark Brown <broonie@kernel.org> Cc: linux-arm-kernel@lists.infradead.org Cc: linux-kernel@vger.kernel.org Signed-off-by: Anshuman Khandual <anshuman.khandual@arm.com> --- arch/arm64/mm/mmu.c | 59 +++++++++++++++++++++++++++++++++++++++++++++ 1 file changed, 59 insertions(+)