Hi Nathan,
I agree that the ehea cannot be built without MEMORY_HOTPLUG. The problem is
the fact that the ppc walk_memory_resource declaration is in the scope of
MEMORY_HOTPLUG. At the moment I don't have complete overview if the move of the
code as you propose in your patch has any side effects. We probably need to
talk to Badari who provided the walk_memory_resource code. We can also just
throw it onto one of our boxes to see what happens. ;)
Regards
Hannes
On Wednesday 28 May 2008 18:44:05 Nathan Lynch wrote:
> Hello,
>
> Hannes Hering wrote:
> > The new ehea memory hot plug implementation depends on MEMORY_HOTPLUG.
> >
> > Signed-off-by: Hannes Hering <[email protected]>
> > ---
> >
> > diff --git a/drivers/net/Kconfig b/drivers/net/Kconfig
> > index f90a86b..181cd86 100644
> > --- a/drivers/net/Kconfig
> > +++ b/drivers/net/Kconfig
> > @@ -2440,7 +2440,7 @@ config CHELSIO_T3
> >
> > config EHEA
> > tristate "eHEA Ethernet support"
> > - depends on IBMEBUS && INET && SPARSEMEM
> > + depends on IBMEBUS && INET && SPARSEMEM && MEMORY_HOTPLUG
> > select INET_LRO
> > ---help---
> > This driver supports the IBM pSeries eHEA ethernet adapter.
>
> I disagree with this change.
>
> It makes it impossible to build the ehea driver without memory hotplug
> enabled. Presumably, this commit was intended to work around a build
> break of this sort (with EHEA=m and MEMORY_HOTPLUG=n):
>
> drivers/net/ehea/ehea_qmr.c: In function 'ehea_create_busmap':
> drivers/net/ehea/ehea_qmr.c:635: error: implicit declaration of function 'walk_memory_resource'
>
> (some indication of this should have been in the commit message, btw)
>
> I think this was the wrong way to fix the issue. EHEA=m and
> MEMORY_HOTPLUG=n is a valid configuration for machines I test.
>
> Any thoughts on the following, which makes walk_memory_resource()
> available regardless of MEMORY_HOTPLUG's setting? I've tested it on a
> JS22 (Power6 blade).
>
> ---
>
> arch/powerpc/mm/mem.c | 3 +--
> drivers/net/Kconfig | 2 +-
> include/linux/memory_hotplug.h | 16 ++++++++--------
> 3 files changed, 10 insertions(+), 11 deletions(-)
>
> diff --git a/arch/powerpc/mm/mem.c b/arch/powerpc/mm/mem.c
> index f67e118..51f82d8 100644
> --- a/arch/powerpc/mm/mem.c
> +++ b/arch/powerpc/mm/mem.c
> @@ -151,6 +151,7 @@ out:
> return ret;
> }
> #endif /* CONFIG_MEMORY_HOTREMOVE */
> +#endif /* CONFIG_MEMORY_HOTPLUG */
>
> /*
> * walk_memory_resource() needs to make sure there is no holes in a given
> @@ -184,8 +185,6 @@ walk_memory_resource(unsigned long start_pfn, unsigned long nr_pages, void *arg,
> }
> EXPORT_SYMBOL_GPL(walk_memory_resource);
>
> -#endif /* CONFIG_MEMORY_HOTPLUG */
> -
> void show_mem(void)
> {
> unsigned long total = 0, reserved = 0;
> diff --git a/drivers/net/Kconfig b/drivers/net/Kconfig
> index dd0ec9e..f4182cf 100644
> --- a/drivers/net/Kconfig
> +++ b/drivers/net/Kconfig
> @@ -2426,7 +2426,7 @@ config CHELSIO_T3
>
> config EHEA
> tristate "eHEA Ethernet support"
> - depends on IBMEBUS && INET && SPARSEMEM && MEMORY_HOTPLUG
> + depends on IBMEBUS && INET && SPARSEMEM
> select INET_LRO
> ---help---
> This driver supports the IBM pSeries eHEA ethernet adapter.
> diff --git a/include/linux/memory_hotplug.h b/include/linux/memory_hotplug.h
> index 73e3586..ea9f5ad 100644
> --- a/include/linux/memory_hotplug.h
> +++ b/include/linux/memory_hotplug.h
> @@ -77,14 +77,6 @@ extern int __add_pages(struct zone *zone, unsigned long start_pfn,
> extern int __remove_pages(struct zone *zone, unsigned long start_pfn,
> unsigned long nr_pages);
>
> -/*
> - * Walk through all memory which is registered as resource.
> - * arg is (start_pfn, nr_pages, private_arg_pointer)
> - */
> -extern int walk_memory_resource(unsigned long start_pfn,
> - unsigned long nr_pages, void *arg,
> - int (*func)(unsigned long, unsigned long, void *));
> -
> #ifdef CONFIG_NUMA
> extern int memory_add_physaddr_to_nid(u64 start);
> #else
> @@ -199,6 +191,14 @@ static inline void register_page_bootmem_info_node(struct pglist_data *pgdat)
>
> #endif /* ! CONFIG_MEMORY_HOTPLUG */
>
> +/*
> + * Walk through all memory which is registered as resource.
> + * arg is (start_pfn, nr_pages, private_arg_pointer)
> + */
> +extern int walk_memory_resource(unsigned long start_pfn,
> + unsigned long nr_pages, void *arg,
> + int (*func)(unsigned long, unsigned long, void *));
> +
> extern int add_memory(int nid, u64 start, u64 size);
> extern int arch_add_memory(int nid, u64 start, u64 size);
> extern int remove_memory(u64 start, u64 size);
>
Hannes Hering wrote:
>
> On Wednesday 28 May 2008 18:44:05 Nathan Lynch wrote:
> >
> > Hannes Hering wrote:
> > > The new ehea memory hot plug implementation depends on MEMORY_HOTPLUG.
> > >
> > > Signed-off-by: Hannes Hering <[email protected]>
> > > ---
> > >
> > > diff --git a/drivers/net/Kconfig b/drivers/net/Kconfig
> > > index f90a86b..181cd86 100644
> > > --- a/drivers/net/Kconfig
> > > +++ b/drivers/net/Kconfig
> > > @@ -2440,7 +2440,7 @@ config CHELSIO_T3
> > >
> > > config EHEA
> > > tristate "eHEA Ethernet support"
> > > - depends on IBMEBUS && INET && SPARSEMEM
> > > + depends on IBMEBUS && INET && SPARSEMEM && MEMORY_HOTPLUG
> > > select INET_LRO
> > > ---help---
> > > This driver supports the IBM pSeries eHEA ethernet adapter.
> >
> > I disagree with this change.
> >
> > It makes it impossible to build the ehea driver without memory hotplug
> > enabled. Presumably, this commit was intended to work around a build
> > break of this sort (with EHEA=m and MEMORY_HOTPLUG=n):
> >
> > drivers/net/ehea/ehea_qmr.c: In function 'ehea_create_busmap':
> > drivers/net/ehea/ehea_qmr.c:635: error: implicit declaration of function 'walk_memory_resource'
> >
> > (some indication of this should have been in the commit message, btw)
> >
> > I think this was the wrong way to fix the issue. EHEA=m and
> > MEMORY_HOTPLUG=n is a valid configuration for machines I test.
> >
> > Any thoughts on the following, which makes walk_memory_resource()
> > available regardless of MEMORY_HOTPLUG's setting? I've tested it on a
> > JS22 (Power6 blade).
>
> I agree that the ehea cannot be built without MEMORY_HOTPLUG. The
> problem is the fact that the ppc walk_memory_resource declaration is
> in the scope of MEMORY_HOTPLUG. At the moment I don't have complete
> overview if the move of the code as you propose in your patch has
> any side effects. We probably need to talk to Badari who provided
> the walk_memory_resource code. We can also just throw it onto one of
> our boxes to see what happens. ;)
I would certainly appreciate any additional testing.
You wrote the ehea code that uses walk_memory_resource, so I was
hoping you could speak to whether ehea needs that interface when
CONFIG_MEMORY_HOTPLUG=n. Or maybe there should be a no-op version of
walk_memory_resource for that case? And what about the
arch-independent version in kernel/resource.c? Badari?
It would be nice to get this resolved for 2.6.26 -- this new
dependency causes working 2.6.25 configs to effectively fail (by
deselecting CONFIG_EHEA during make oldconfig).
On Tue, 2008-06-03 at 15:49 -0500, Nathan Lynch wrote:
> Hannes Hering wrote:
> >
> > On Wednesday 28 May 2008 18:44:05 Nathan Lynch wrote:
> > >
> > > Hannes Hering wrote:
> > > > The new ehea memory hot plug implementation depends on MEMORY_HOTPLUG.
> > > >
> > > > Signed-off-by: Hannes Hering <[email protected]>
> > > > ---
> > > >
> > > > diff --git a/drivers/net/Kconfig b/drivers/net/Kconfig
> > > > index f90a86b..181cd86 100644
> > > > --- a/drivers/net/Kconfig
> > > > +++ b/drivers/net/Kconfig
> > > > @@ -2440,7 +2440,7 @@ config CHELSIO_T3
> > > >
> > > > config EHEA
> > > > tristate "eHEA Ethernet support"
> > > > - depends on IBMEBUS && INET && SPARSEMEM
> > > > + depends on IBMEBUS && INET && SPARSEMEM && MEMORY_HOTPLUG
> > > > select INET_LRO
> > > > ---help---
> > > > This driver supports the IBM pSeries eHEA ethernet adapter.
> > >
> > > I disagree with this change.
> > >
> > > It makes it impossible to build the ehea driver without memory hotplug
> > > enabled. Presumably, this commit was intended to work around a build
> > > break of this sort (with EHEA=m and MEMORY_HOTPLUG=n):
> > >
> > > drivers/net/ehea/ehea_qmr.c: In function 'ehea_create_busmap':
> > > drivers/net/ehea/ehea_qmr.c:635: error: implicit declaration of function 'walk_memory_resource'
> > >
> > > (some indication of this should have been in the commit message, btw)
> > >
> > > I think this was the wrong way to fix the issue. EHEA=m and
> > > MEMORY_HOTPLUG=n is a valid configuration for machines I test.
> > >
> > > Any thoughts on the following, which makes walk_memory_resource()
> > > available regardless of MEMORY_HOTPLUG's setting? I've tested it on a
> > > JS22 (Power6 blade).
> >
> > I agree that the ehea cannot be built without MEMORY_HOTPLUG. The
> > problem is the fact that the ppc walk_memory_resource declaration is
> > in the scope of MEMORY_HOTPLUG. At the moment I don't have complete
> > overview if the move of the code as you propose in your patch has
> > any side effects. We probably need to talk to Badari who provided
> > the walk_memory_resource code. We can also just throw it onto one of
> > our boxes to see what happens. ;)
>
> I would certainly appreciate any additional testing.
I think we can make walk_memory_resource() for ppc64 available
outside of MEMORY_HOTPLUG. It doesn't require any MEMORY_HOTPLUG
functionality to work correctly. Its a generic enough interface.
Only reason I placed it under MEMORY_HOTPLUG is to have parity
with the arch-independent version + we needed for eHEA driver whic
needs MEMORY_HOTPLUG.
> You wrote the ehea code that uses walk_memory_resource, so I was
> hoping you could speak to whether ehea needs that interface when
> CONFIG_MEMORY_HOTPLUG=n. Or maybe there should be a no-op version of
> walk_memory_resource for that case? And what about the
> arch-independent version in kernel/resource.c? Badari?
I would leave the arch-independent version alone - since its not
exported and there are no users for it.
> It would be nice to get this resolved for 2.6.26 -- this new
> dependency causes working 2.6.25 configs to effectively fail (by
> deselecting CONFIG_EHEA during make oldconfig).
Thanks,
Badari
Badari Pulavarty wrote:
>> It would be nice to get this resolved for 2.6.26 -- this new
>> dependency causes working 2.6.25 configs to effectively fail (by
>> deselecting CONFIG_EHEA during make oldconfig).
When everybody else is happy, I'm happy :)
The ehea driver was recently changed[1] to use walk_memory_resource() to
detect the system's memory layout. However, walk_memory_resource() is
available only when memory hotplug is enabled. So CONFIG_EHEA was
made to depend on MEMORY_HOTPLUG [2], but it is inappropriate for a
network driver to have such a dependency.
Make the declaration of walk_memory_resource() and its powerpc
implementation (ehea is powerpc-specific) unconditionally available.
[1] 48cfb14f8b89d4d5b3df6c16f08b258686fb12ad
"ehea: Add DLPAR memory remove support"
[2] fb7b6ca2b6b7c23b52be143bdd5f55a23b9780c8
"ehea: Add dependency to Kconfig"
Signed-off-by: Nathan Lynch <[email protected]>
---
arch/powerpc/mm/mem.c | 3 +--
include/linux/memory_hotplug.h | 16 ++++++++--------
2 files changed, 9 insertions(+), 10 deletions(-)
diff --git a/arch/powerpc/mm/mem.c b/arch/powerpc/mm/mem.c
index f67e118..51f82d8 100644
--- a/arch/powerpc/mm/mem.c
+++ b/arch/powerpc/mm/mem.c
@@ -151,6 +151,7 @@ out:
return ret;
}
#endif /* CONFIG_MEMORY_HOTREMOVE */
+#endif /* CONFIG_MEMORY_HOTPLUG */
/*
* walk_memory_resource() needs to make sure there is no holes in a given
@@ -184,8 +185,6 @@ walk_memory_resource(unsigned long start_pfn, unsigned long nr_pages, void *arg,
}
EXPORT_SYMBOL_GPL(walk_memory_resource);
-#endif /* CONFIG_MEMORY_HOTPLUG */
-
void show_mem(void)
{
unsigned long total = 0, reserved = 0;
diff --git a/include/linux/memory_hotplug.h b/include/linux/memory_hotplug.h
index 73e3586..ea9f5ad 100644
--- a/include/linux/memory_hotplug.h
+++ b/include/linux/memory_hotplug.h
@@ -77,14 +77,6 @@ extern int __add_pages(struct zone *zone, unsigned long start_pfn,
extern int __remove_pages(struct zone *zone, unsigned long start_pfn,
unsigned long nr_pages);
-/*
- * Walk through all memory which is registered as resource.
- * arg is (start_pfn, nr_pages, private_arg_pointer)
- */
-extern int walk_memory_resource(unsigned long start_pfn,
- unsigned long nr_pages, void *arg,
- int (*func)(unsigned long, unsigned long, void *));
-
#ifdef CONFIG_NUMA
extern int memory_add_physaddr_to_nid(u64 start);
#else
@@ -199,6 +191,14 @@ static inline void register_page_bootmem_info_node(struct pglist_data *pgdat)
#endif /* ! CONFIG_MEMORY_HOTPLUG */
+/*
+ * Walk through all memory which is registered as resource.
+ * arg is (start_pfn, nr_pages, private_arg_pointer)
+ */
+extern int walk_memory_resource(unsigned long start_pfn,
+ unsigned long nr_pages, void *arg,
+ int (*func)(unsigned long, unsigned long, void *));
+
extern int add_memory(int nid, u64 start, u64 size);
extern int arch_add_memory(int nid, u64 start, u64 size);
extern int remove_memory(u64 start, u64 size);
--
1.5.5
Now that walk_memory_resource() is available regardless of
MEMORY_HOTPLUG's setting, this dependency is not needed.
Signed-off-by: Nathan Lynch <[email protected]>
---
drivers/net/Kconfig | 2 +-
1 files changed, 1 insertions(+), 1 deletions(-)
diff --git a/drivers/net/Kconfig b/drivers/net/Kconfig
index dd0ec9e..f4182cf 100644
--- a/drivers/net/Kconfig
+++ b/drivers/net/Kconfig
@@ -2426,7 +2426,7 @@ config CHELSIO_T3
config EHEA
tristate "eHEA Ethernet support"
- depends on IBMEBUS && INET && SPARSEMEM && MEMORY_HOTPLUG
+ depends on IBMEBUS && INET && SPARSEMEM
select INET_LRO
---help---
This driver supports the IBM pSeries eHEA ethernet adapter.
--
1.5.5
Nathan Lynch wrote:
> Now that walk_memory_resource() is available regardless of
> MEMORY_HOTPLUG's setting, this dependency is not needed.
>
> Signed-off-by: Nathan Lynch <[email protected]>
> ---
> drivers/net/Kconfig | 2 +-
> 1 files changed, 1 insertions(+), 1 deletions(-)
>
> diff --git a/drivers/net/Kconfig b/drivers/net/Kconfig
> index dd0ec9e..f4182cf 100644
> --- a/drivers/net/Kconfig
> +++ b/drivers/net/Kconfig
> @@ -2426,7 +2426,7 @@ config CHELSIO_T3
>
> config EHEA
> tristate "eHEA Ethernet support"
> - depends on IBMEBUS && INET && SPARSEMEM && MEMORY_HOTPLUG
> + depends on IBMEBUS && INET && SPARSEMEM
> select INET_LRO
ACK, please send via appropriate target for memory hotplug stuff
Hello.
I'm working for memory hotplug. This patch looks good to me.
Acked-by: Yasunori Goto <[email protected]>
Thanks.
> Nathan Lynch wrote:
> > Now that walk_memory_resource() is available regardless of
> > MEMORY_HOTPLUG's setting, this dependency is not needed.
> >
> > Signed-off-by: Nathan Lynch <[email protected]>
> > ---
> > drivers/net/Kconfig | 2 +-
> > 1 files changed, 1 insertions(+), 1 deletions(-)
> >
> > diff --git a/drivers/net/Kconfig b/drivers/net/Kconfig
> > index dd0ec9e..f4182cf 100644
> > --- a/drivers/net/Kconfig
> > +++ b/drivers/net/Kconfig
> > @@ -2426,7 +2426,7 @@ config CHELSIO_T3
> >
> > config EHEA
> > tristate "eHEA Ethernet support"
> > - depends on IBMEBUS && INET && SPARSEMEM && MEMORY_HOTPLUG
> > + depends on IBMEBUS && INET && SPARSEMEM
> > select INET_LRO
>
> ACK, please send via appropriate target for memory hotplug stuff
>
>
> --
> To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
> the body of a message to [email protected]
> More majordomo info at http://vger.kernel.org/majordomo-info.html
> Please read the FAQ at http://www.tux.org/lkml/
--
Yasunori Goto
On Tue, 2008-06-03 at 17:30 -0500, Nathan Lynch wrote:
> The ehea driver was recently changed[1] to use walk_memory_resource() to
> detect the system's memory layout. However, walk_memory_resource() is
> available only when memory hotplug is enabled. So CONFIG_EHEA was
> made to depend on MEMORY_HOTPLUG [2], but it is inappropriate for a
> network driver to have such a dependency.
>
> Make the declaration of walk_memory_resource() and its powerpc
> implementation (ehea is powerpc-specific) unconditionally available.
>
> [1] 48cfb14f8b89d4d5b3df6c16f08b258686fb12ad
> "ehea: Add DLPAR memory remove support"
>
> [2] fb7b6ca2b6b7c23b52be143bdd5f55a23b9780c8
> "ehea: Add dependency to Kconfig"
>
> Signed-off-by: Nathan Lynch <[email protected]>
> ---
> arch/powerpc/mm/mem.c | 3 +--
> include/linux/memory_hotplug.h | 16 ++++++++--------
> 2 files changed, 9 insertions(+), 10 deletions(-)
>
> diff --git a/arch/powerpc/mm/mem.c b/arch/powerpc/mm/mem.c
> index f67e118..51f82d8 100644
> --- a/arch/powerpc/mm/mem.c
> +++ b/arch/powerpc/mm/mem.c
> @@ -151,6 +151,7 @@ out:
> return ret;
> }
> #endif /* CONFIG_MEMORY_HOTREMOVE */
> +#endif /* CONFIG_MEMORY_HOTPLUG */
>
> /*
> * walk_memory_resource() needs to make sure there is no holes in a given
> @@ -184,8 +185,6 @@ walk_memory_resource(unsigned long start_pfn, unsigned long nr_pages, void *arg,
> }
> EXPORT_SYMBOL_GPL(walk_memory_resource);
>
> -#endif /* CONFIG_MEMORY_HOTPLUG */
> -
> void show_mem(void)
> {
> unsigned long total = 0, reserved = 0;
> diff --git a/include/linux/memory_hotplug.h b/include/linux/memory_hotplug.h
> index 73e3586..ea9f5ad 100644
> --- a/include/linux/memory_hotplug.h
> +++ b/include/linux/memory_hotplug.h
> @@ -77,14 +77,6 @@ extern int __add_pages(struct zone *zone, unsigned long start_pfn,
> extern int __remove_pages(struct zone *zone, unsigned long start_pfn,
> unsigned long nr_pages);
>
> -/*
> - * Walk through all memory which is registered as resource.
> - * arg is (start_pfn, nr_pages, private_arg_pointer)
> - */
> -extern int walk_memory_resource(unsigned long start_pfn,
> - unsigned long nr_pages, void *arg,
> - int (*func)(unsigned long, unsigned long, void *));
> -
> #ifdef CONFIG_NUMA
> extern int memory_add_physaddr_to_nid(u64 start);
> #else
> @@ -199,6 +191,14 @@ static inline void register_page_bootmem_info_node(struct pglist_data *pgdat)
>
> #endif /* ! CONFIG_MEMORY_HOTPLUG */
>
> +/*
> + * Walk through all memory which is registered as resource.
> + * arg is (start_pfn, nr_pages, private_arg_pointer)
> + */
> +extern int walk_memory_resource(unsigned long start_pfn,
> + unsigned long nr_pages, void *arg,
> + int (*func)(unsigned long, unsigned long, void *));
> +
> extern int add_memory(int nid, u64 start, u64 size);
> extern int arch_add_memory(int nid, u64 start, u64 size);
> extern int remove_memory(u64 start, u64 size);
Sorry. I couldn't get back earlier.
Acked-by: Badari Pulavarty <[email protected]>
Nathan Lynch wrote:
> The ehea driver was recently changed[1] to use walk_memory_resource() to
> detect the system's memory layout. However, walk_memory_resource() is
> available only when memory hotplug is enabled. So CONFIG_EHEA was
> made to depend on MEMORY_HOTPLUG [2], but it is inappropriate for a
> network driver to have such a dependency.
>
> Make the declaration of walk_memory_resource() and its powerpc
> implementation (ehea is powerpc-specific) unconditionally available.
Paul, can you take these two patches, or should I send them to akpm?
Nathan Lynch writes:
> Paul, can you take these two patches, or should I send them to akpm?
I'll take them. I assume we want this in 2.6.26.
Paul.