Re: [PATCH v3 06/11] mm/memory_hotplug: Allow arch_remove_pages() without CONFIG_MEMORY_HOTREMOVE
On Mon 15-07-19 12:54:20, David Hildenbrand wrote: [...] > So I'm leaving it like it is. arch_remove_memory() will be mandatory for > architectures implementing arch_add_memory(). I do agree that removing CONFIG_MEMORY_HOTREMOVE makes some sense. But this patch being a mid step should be simpler rather than going half way to get there. I would have liked the above for the purpose of this patch more and then go with another one to remove the config altogether. But Andrew has already sent his patch bomb including this series to Linus so this is all moot. -- Michal Hocko SUSE Labs
Re: [PATCH v3 06/11] mm/memory_hotplug: Allow arch_remove_pages() without CONFIG_MEMORY_HOTREMOVE
On 01.07.19 14:51, Michal Hocko wrote:
> On Mon 01-07-19 10:01:41, Michal Hocko wrote:
>> On Mon 27-05-19 13:11:47, David Hildenbrand wrote:
>>> We want to improve error handling while adding memory by allowing
>>> to use arch_remove_memory() and __remove_pages() even if
>>> CONFIG_MEMORY_HOTREMOVE is not set to e.g., implement something like:
>>>
>>> arch_add_memory()
>>> rc = do_something();
>>> if (rc) {
>>> arch_remove_memory();
>>> }
>>>
>>> We won't get rid of CONFIG_MEMORY_HOTREMOVE for now, as it will require
>>> quite some dependencies for memory offlining.
>>
>> If we cannot really remove CONFIG_MEMORY_HOTREMOVE altogether then why
>> not simply add an empty placeholder for arch_remove_memory when the
>> config is disabled?
>
> In other words, can we replace this by something as simple as:
>
> diff --git a/include/linux/memory_hotplug.h b/include/linux/memory_hotplug.h
> index ae892eef8b82..0329027fe740 100644
> --- a/include/linux/memory_hotplug.h
> +++ b/include/linux/memory_hotplug.h
> @@ -128,6 +128,20 @@ extern void arch_remove_memory(int nid, u64 start, u64
> size,
> struct vmem_altmap *altmap);
> extern void __remove_pages(struct zone *zone, unsigned long start_pfn,
> unsigned long nr_pages, struct vmem_altmap *altmap);
> +#else
> +/*
> + * Allow code using
> + * arch_add_memory();
> + * rc = do_something();
> + * if (rc)
> + * arch_remove_memory();
> + *
> + * without ifdefery.
> + */
> +static inline void arch_remove_memory(int nid, u64 start, u64 size,
> +struct vmem_altmap *altmap)
> +{
> +}
> #endif /* CONFIG_MEMORY_HOTREMOVE */
>
> /*
>
A system configured without CONFIG_MEMORY_HOTREMOVE should not suddenly
behave worse than before when adding of memory fails. What you suggest
result in that.
The goal should be to force architectures to properly implement
arch_remove_memory() right from the start - which is the case for all
architectures after this patch set *except* arm, for which a proper
implementation is on the way.
So I'm leaving it like it is. arch_remove_memory() will be mandatory for
architectures implementing arch_add_memory().
--
Thanks,
David / dhildenb
Re: [PATCH v3 06/11] mm/memory_hotplug: Allow arch_remove_pages() without CONFIG_MEMORY_HOTREMOVE
On Mon 01-07-19 10:01:41, Michal Hocko wrote:
> On Mon 27-05-19 13:11:47, David Hildenbrand wrote:
> > We want to improve error handling while adding memory by allowing
> > to use arch_remove_memory() and __remove_pages() even if
> > CONFIG_MEMORY_HOTREMOVE is not set to e.g., implement something like:
> >
> > arch_add_memory()
> > rc = do_something();
> > if (rc) {
> > arch_remove_memory();
> > }
> >
> > We won't get rid of CONFIG_MEMORY_HOTREMOVE for now, as it will require
> > quite some dependencies for memory offlining.
>
> If we cannot really remove CONFIG_MEMORY_HOTREMOVE altogether then why
> not simply add an empty placeholder for arch_remove_memory when the
> config is disabled?
In other words, can we replace this by something as simple as:
diff --git a/include/linux/memory_hotplug.h b/include/linux/memory_hotplug.h
index ae892eef8b82..0329027fe740 100644
--- a/include/linux/memory_hotplug.h
+++ b/include/linux/memory_hotplug.h
@@ -128,6 +128,20 @@ extern void arch_remove_memory(int nid, u64 start, u64
size,
struct vmem_altmap *altmap);
extern void __remove_pages(struct zone *zone, unsigned long start_pfn,
unsigned long nr_pages, struct vmem_altmap *altmap);
+#else
+/*
+ * Allow code using
+ * arch_add_memory();
+ * rc = do_something();
+ * if (rc)
+ * arch_remove_memory();
+ *
+ * without ifdefery.
+ */
+static inline void arch_remove_memory(int nid, u64 start, u64 size,
+ struct vmem_altmap *altmap)
+{
+}
#endif /* CONFIG_MEMORY_HOTREMOVE */
/*
--
Michal Hocko
SUSE Labs
Re: [PATCH v3 06/11] mm/memory_hotplug: Allow arch_remove_pages() without CONFIG_MEMORY_HOTREMOVE
On Mon 27-05-19 13:11:47, David Hildenbrand wrote:
> We want to improve error handling while adding memory by allowing
> to use arch_remove_memory() and __remove_pages() even if
> CONFIG_MEMORY_HOTREMOVE is not set to e.g., implement something like:
>
> arch_add_memory()
> rc = do_something();
> if (rc) {
> arch_remove_memory();
> }
>
> We won't get rid of CONFIG_MEMORY_HOTREMOVE for now, as it will require
> quite some dependencies for memory offlining.
If we cannot really remove CONFIG_MEMORY_HOTREMOVE altogether then why
not simply add an empty placeholder for arch_remove_memory when the
config is disabled?
> Cc: Tony Luck
> Cc: Fenghua Yu
> Cc: Benjamin Herrenschmidt
> Cc: Paul Mackerras
> Cc: Michael Ellerman
> Cc: Martin Schwidefsky
> Cc: Heiko Carstens
> Cc: Yoshinori Sato
> Cc: Rich Felker
> Cc: Dave Hansen
> Cc: Andy Lutomirski
> Cc: Peter Zijlstra
> Cc: Thomas Gleixner
> Cc: Ingo Molnar
> Cc: Borislav Petkov
> Cc: "H. Peter Anvin"
> Cc: Greg Kroah-Hartman
> Cc: "Rafael J. Wysocki"
> Cc: Andrew Morton
> Cc: Michal Hocko
> Cc: Mike Rapoport
> Cc: David Hildenbrand
> Cc: Oscar Salvador
> Cc: "Kirill A. Shutemov"
> Cc: Alex Deucher
> Cc: "David S. Miller"
> Cc: Mark Brown
> Cc: Chris Wilson
> Cc: Christophe Leroy
> Cc: Nicholas Piggin
> Cc: Vasily Gorbik
> Cc: Rob Herring
> Cc: Masahiro Yamada
> Cc: "mike.tra...@hpe.com"
> Cc: Andrew Banman
> Cc: Pavel Tatashin
> Cc: Wei Yang
> Cc: Arun KS
> Cc: Qian Cai
> Cc: Mathieu Malaterre
> Cc: Baoquan He
> Cc: Logan Gunthorpe
> Cc: Anshuman Khandual
> Signed-off-by: David Hildenbrand
> ---
> arch/arm64/mm/mmu.c| 2 --
> arch/ia64/mm/init.c| 2 --
> arch/powerpc/mm/mem.c | 2 --
> arch/s390/mm/init.c| 2 --
> arch/sh/mm/init.c | 2 --
> arch/x86/mm/init_32.c | 2 --
> arch/x86/mm/init_64.c | 2 --
> drivers/base/memory.c | 2 --
> include/linux/memory.h | 2 --
> include/linux/memory_hotplug.h | 2 --
> mm/memory_hotplug.c| 2 --
> mm/sparse.c| 6 --
> 12 files changed, 28 deletions(-)
>
> diff --git a/arch/arm64/mm/mmu.c b/arch/arm64/mm/mmu.c
> index e569a543c384..9ccd7539f2d4 100644
> --- a/arch/arm64/mm/mmu.c
> +++ b/arch/arm64/mm/mmu.c
> @@ -1084,7 +1084,6 @@ int arch_add_memory(int nid, u64 start, u64 size,
> return __add_pages(nid, start >> PAGE_SHIFT, size >> PAGE_SHIFT,
> restrictions);
> }
> -#ifdef CONFIG_MEMORY_HOTREMOVE
> void arch_remove_memory(int nid, u64 start, u64 size,
> struct vmem_altmap *altmap)
> {
> @@ -1103,4 +1102,3 @@ void arch_remove_memory(int nid, u64 start, u64 size,
> __remove_pages(zone, start_pfn, nr_pages, altmap);
> }
> #endif
> -#endif
> diff --git a/arch/ia64/mm/init.c b/arch/ia64/mm/init.c
> index d28e29103bdb..aae75fd7b810 100644
> --- a/arch/ia64/mm/init.c
> +++ b/arch/ia64/mm/init.c
> @@ -681,7 +681,6 @@ int arch_add_memory(int nid, u64 start, u64 size,
> return ret;
> }
>
> -#ifdef CONFIG_MEMORY_HOTREMOVE
> void arch_remove_memory(int nid, u64 start, u64 size,
> struct vmem_altmap *altmap)
> {
> @@ -693,4 +692,3 @@ void arch_remove_memory(int nid, u64 start, u64 size,
> __remove_pages(zone, start_pfn, nr_pages, altmap);
> }
> #endif
> -#endif
> diff --git a/arch/powerpc/mm/mem.c b/arch/powerpc/mm/mem.c
> index e885fe2aafcc..e4bc2dc3f593 100644
> --- a/arch/powerpc/mm/mem.c
> +++ b/arch/powerpc/mm/mem.c
> @@ -130,7 +130,6 @@ int __ref arch_add_memory(int nid, u64 start, u64 size,
> return __add_pages(nid, start_pfn, nr_pages, restrictions);
> }
>
> -#ifdef CONFIG_MEMORY_HOTREMOVE
> void __ref arch_remove_memory(int nid, u64 start, u64 size,
>struct vmem_altmap *altmap)
> {
> @@ -164,7 +163,6 @@ void __ref arch_remove_memory(int nid, u64 start, u64
> size,
> pr_warn("Hash collision while resizing HPT\n");
> }
> #endif
> -#endif /* CONFIG_MEMORY_HOTPLUG */
>
> #ifndef CONFIG_NEED_MULTIPLE_NODES
> void __init mem_topology_setup(void)
> diff --git a/arch/s390/mm/init.c b/arch/s390/mm/init.c
> index 14955e0a9fcf..ffb81fe95c77 100644
> --- a/arch/s390/mm/init.c
> +++ b/arch/s390/mm/init.c
> @@ -239,7 +239,6 @@ int arch_add_memory(int nid, u64 start, u64 size,
> return rc;
> }
>
> -#ifdef CONFIG_MEMORY_HOTREMOVE
> void arch_remove_memory(int nid, u64 start, u64 size,
> struct vmem_altmap *altmap)
> {
> @@ -251,5 +250,4 @@ void arch_remove_memory(int nid, u64 start, u64 size,
> __remove_pages(zone, start_pfn, nr_pages, altmap);
> vmem_remove_mapping(start, size);
> }
> -#endif
> #endif /* CONFIG_MEMORY_HOTPLUG */
> diff --git a/arch/sh/mm/init.c b/arch/sh/mm/init.c
> index 13c6a6bb5fd9..dfdbaa50946e 100644
> --- a/arch/sh/mm/init.c
> +++ b/arch/sh/mm/init.c
> @@ -429,7 +429,6 @@ int
Re: [PATCH v3 06/11] mm/memory_hotplug: Allow arch_remove_pages() without CONFIG_MEMORY_HOTREMOVE
On Tue, Jun 04, 2019 at 08:59:43AM +0200, David Hildenbrand wrote: >On 04.06.19 00:15, Wei Yang wrote: >> Allow arch_remove_pages() or arch_remove_memory()? > >Looks like I merged __remove_pages() and arch_remove_memory(). > >@Andrew, can you fix this up to > >"mm/memory_hotplug: Allow arch_remove_memory() without >CONFIG_MEMORY_HOTREMOVE" > >? Thanks! > Already merged? >> >> And want to confirm the kernel build on affected arch succeed? > >I compile-tested on s390x and x86. As the patches are in linux-next for >some time, I think the other builds are also fine. > Yep, sounds good~ >Thanks! > >-- > >Thanks, > >David / dhildenb -- Wei Yang Help you, Help me
Re: [PATCH v3 06/11] mm/memory_hotplug: Allow arch_remove_pages() without CONFIG_MEMORY_HOTREMOVE
On 04.06.19 10:31, Wei Yang wrote: > On Tue, Jun 04, 2019 at 08:59:43AM +0200, David Hildenbrand wrote: >> On 04.06.19 00:15, Wei Yang wrote: >>> Allow arch_remove_pages() or arch_remove_memory()? >> >> Looks like I merged __remove_pages() and arch_remove_memory(). >> >> @Andrew, can you fix this up to >> >> "mm/memory_hotplug: Allow arch_remove_memory() without >> CONFIG_MEMORY_HOTREMOVE" >> >> ? Thanks! >> > > Already merged? Andrew picked it up, but it's not in linus' tree yet. -- Thanks, David / dhildenb
Re: [PATCH v3 06/11] mm/memory_hotplug: Allow arch_remove_pages() without CONFIG_MEMORY_HOTREMOVE
On 04.06.19 00:15, Wei Yang wrote: > Allow arch_remove_pages() or arch_remove_memory()? Looks like I merged __remove_pages() and arch_remove_memory(). @Andrew, can you fix this up to "mm/memory_hotplug: Allow arch_remove_memory() without CONFIG_MEMORY_HOTREMOVE" ? Thanks! > > And want to confirm the kernel build on affected arch succeed? I compile-tested on s390x and x86. As the patches are in linux-next for some time, I think the other builds are also fine. Thanks! -- Thanks, David / dhildenb
Re: [PATCH v3 06/11] mm/memory_hotplug: Allow arch_remove_pages() without CONFIG_MEMORY_HOTREMOVE
Allow arch_remove_pages() or arch_remove_memory()?
And want to confirm the kernel build on affected arch succeed?
On Mon, May 27, 2019 at 01:11:47PM +0200, David Hildenbrand wrote:
>We want to improve error handling while adding memory by allowing
>to use arch_remove_memory() and __remove_pages() even if
>CONFIG_MEMORY_HOTREMOVE is not set to e.g., implement something like:
>
> arch_add_memory()
> rc = do_something();
> if (rc) {
> arch_remove_memory();
> }
>
>We won't get rid of CONFIG_MEMORY_HOTREMOVE for now, as it will require
>quite some dependencies for memory offlining.
>
>Cc: Tony Luck
>Cc: Fenghua Yu
>Cc: Benjamin Herrenschmidt
>Cc: Paul Mackerras
>Cc: Michael Ellerman
>Cc: Martin Schwidefsky
>Cc: Heiko Carstens
>Cc: Yoshinori Sato
>Cc: Rich Felker
>Cc: Dave Hansen
>Cc: Andy Lutomirski
>Cc: Peter Zijlstra
>Cc: Thomas Gleixner
>Cc: Ingo Molnar
>Cc: Borislav Petkov
>Cc: "H. Peter Anvin"
>Cc: Greg Kroah-Hartman
>Cc: "Rafael J. Wysocki"
>Cc: Andrew Morton
>Cc: Michal Hocko
>Cc: Mike Rapoport
>Cc: David Hildenbrand
>Cc: Oscar Salvador
>Cc: "Kirill A. Shutemov"
>Cc: Alex Deucher
>Cc: "David S. Miller"
>Cc: Mark Brown
>Cc: Chris Wilson
>Cc: Christophe Leroy
>Cc: Nicholas Piggin
>Cc: Vasily Gorbik
>Cc: Rob Herring
>Cc: Masahiro Yamada
>Cc: "mike.tra...@hpe.com"
>Cc: Andrew Banman
>Cc: Pavel Tatashin
>Cc: Wei Yang
>Cc: Arun KS
>Cc: Qian Cai
>Cc: Mathieu Malaterre
>Cc: Baoquan He
>Cc: Logan Gunthorpe
>Cc: Anshuman Khandual
>Signed-off-by: David Hildenbrand
>---
> arch/arm64/mm/mmu.c| 2 --
> arch/ia64/mm/init.c| 2 --
> arch/powerpc/mm/mem.c | 2 --
> arch/s390/mm/init.c| 2 --
> arch/sh/mm/init.c | 2 --
> arch/x86/mm/init_32.c | 2 --
> arch/x86/mm/init_64.c | 2 --
> drivers/base/memory.c | 2 --
> include/linux/memory.h | 2 --
> include/linux/memory_hotplug.h | 2 --
> mm/memory_hotplug.c| 2 --
> mm/sparse.c| 6 --
> 12 files changed, 28 deletions(-)
>
>diff --git a/arch/arm64/mm/mmu.c b/arch/arm64/mm/mmu.c
>index e569a543c384..9ccd7539f2d4 100644
>--- a/arch/arm64/mm/mmu.c
>+++ b/arch/arm64/mm/mmu.c
>@@ -1084,7 +1084,6 @@ int arch_add_memory(int nid, u64 start, u64 size,
> return __add_pages(nid, start >> PAGE_SHIFT, size >> PAGE_SHIFT,
> restrictions);
> }
>-#ifdef CONFIG_MEMORY_HOTREMOVE
> void arch_remove_memory(int nid, u64 start, u64 size,
> struct vmem_altmap *altmap)
> {
>@@ -1103,4 +1102,3 @@ void arch_remove_memory(int nid, u64 start, u64 size,
> __remove_pages(zone, start_pfn, nr_pages, altmap);
> }
> #endif
>-#endif
>diff --git a/arch/ia64/mm/init.c b/arch/ia64/mm/init.c
>index d28e29103bdb..aae75fd7b810 100644
>--- a/arch/ia64/mm/init.c
>+++ b/arch/ia64/mm/init.c
>@@ -681,7 +681,6 @@ int arch_add_memory(int nid, u64 start, u64 size,
> return ret;
> }
>
>-#ifdef CONFIG_MEMORY_HOTREMOVE
> void arch_remove_memory(int nid, u64 start, u64 size,
> struct vmem_altmap *altmap)
> {
>@@ -693,4 +692,3 @@ void arch_remove_memory(int nid, u64 start, u64 size,
> __remove_pages(zone, start_pfn, nr_pages, altmap);
> }
> #endif
>-#endif
>diff --git a/arch/powerpc/mm/mem.c b/arch/powerpc/mm/mem.c
>index e885fe2aafcc..e4bc2dc3f593 100644
>--- a/arch/powerpc/mm/mem.c
>+++ b/arch/powerpc/mm/mem.c
>@@ -130,7 +130,6 @@ int __ref arch_add_memory(int nid, u64 start, u64 size,
> return __add_pages(nid, start_pfn, nr_pages, restrictions);
> }
>
>-#ifdef CONFIG_MEMORY_HOTREMOVE
> void __ref arch_remove_memory(int nid, u64 start, u64 size,
>struct vmem_altmap *altmap)
> {
>@@ -164,7 +163,6 @@ void __ref arch_remove_memory(int nid, u64 start, u64 size,
> pr_warn("Hash collision while resizing HPT\n");
> }
> #endif
>-#endif /* CONFIG_MEMORY_HOTPLUG */
>
> #ifndef CONFIG_NEED_MULTIPLE_NODES
> void __init mem_topology_setup(void)
>diff --git a/arch/s390/mm/init.c b/arch/s390/mm/init.c
>index 14955e0a9fcf..ffb81fe95c77 100644
>--- a/arch/s390/mm/init.c
>+++ b/arch/s390/mm/init.c
>@@ -239,7 +239,6 @@ int arch_add_memory(int nid, u64 start, u64 size,
> return rc;
> }
>
>-#ifdef CONFIG_MEMORY_HOTREMOVE
> void arch_remove_memory(int nid, u64 start, u64 size,
> struct vmem_altmap *altmap)
> {
>@@ -251,5 +250,4 @@ void arch_remove_memory(int nid, u64 start, u64 size,
> __remove_pages(zone, start_pfn, nr_pages, altmap);
> vmem_remove_mapping(start, size);
> }
>-#endif
> #endif /* CONFIG_MEMORY_HOTPLUG */
>diff --git a/arch/sh/mm/init.c b/arch/sh/mm/init.c
>index 13c6a6bb5fd9..dfdbaa50946e 100644
>--- a/arch/sh/mm/init.c
>+++ b/arch/sh/mm/init.c
>@@ -429,7 +429,6 @@ int memory_add_physaddr_to_nid(u64 addr)
> EXPORT_SYMBOL_GPL(memory_add_physaddr_to_nid);
> #endif
>
>-#ifdef CONFIG_MEMORY_HOTREMOVE
> void arch_remove_memory(int nid, u64 start, u64
Re: [PATCH v3 06/11] mm/memory_hotplug: Allow arch_remove_pages() without CONFIG_MEMORY_HOTREMOVE
On Mon, May 27, 2019 at 7:12 AM David Hildenbrand wrote:
>
> We want to improve error handling while adding memory by allowing
> to use arch_remove_memory() and __remove_pages() even if
> CONFIG_MEMORY_HOTREMOVE is not set to e.g., implement something like:
>
> arch_add_memory()
> rc = do_something();
> if (rc) {
> arch_remove_memory();
> }
>
> We won't get rid of CONFIG_MEMORY_HOTREMOVE for now, as it will require
> quite some dependencies for memory offlining.
I like this simplification, we should really get rid of CONFIG_MEMORY_HOTREMOVE.
Reviewed-by: Pavel Tatashin
