Re: [PATCH] mm: unhide vmstat_text definition for CONFIG_SMP
On Tue, 17 May 2016, Michal Hocko wrote: > On Mon 16-05-16 15:36:56, Andrew Morton wrote: > > On Mon, 16 May 2016 16:23:33 +0200 Michal Hocko wrote: > > > > > Andrew, I think that the following is more straightforward fix and > > > should be folded in to the patch which has introduced vmstat_refresh. > > > --- > > > >From b8dd18fb7df040e1bfe61aadde1d903589de15e4 Mon Sep 17 00:00:00 2001 > > > From: Michal Hocko > > > Date: Mon, 16 May 2016 16:19:53 +0200 > > > Subject: [PATCH] mmotm: > > > mm-proc-sys-vm-stat_refresh-to-force-vmstat-update-fix > > > > > > Arnd has reported: > > > In randconfig builds with sysfs, procfs and numa all disabled, > > > but SMP enabled, we now get a link error in the newly introduced > > > vmstat_refresh function: > > > > > > mm/built-in.o: In function `vmstat_refresh': > > > :(.text+0x15c78): undefined reference to `vmstat_text' > > > > > > vmstat_refresh is proc_fs specific so there is no reason to define it > > > when !CONFIG_PROC_FS. > > > > I already had this: > > > > From: Christoph Lameter > > Subject: Do not build vmstat_refresh if there is no procfs support > > > > It makes no sense to build functionality into the kernel that > > cannot be used and causes build issues. > > > > Link: > > http://lkml.kernel.org/r/alpine.deb.2.20.1605111011260.9...@east.gentwo.org > > Signed-off-by: Christoph Lameter > > Reported-by: Arnd Bergmann > > Signed-off-by: Andrew Morton > > But this is broken: > http://lkml.kernel.org/r/20160516073144.ga23...@dhcp22.suse.cz and > kbuild robot agrees > http://lkml.kernel.org/r/201605171333.anqjcwpy%fengguang...@intel.com Sorry for my noise, sorry for my silence, thanks to Arnd and everyone for chipping in. But now I try it, I find that even Michal's is not quite right: if you build without CONFIG_PROC_FS, then it gives you mm/vmstat.c:1381:13: warning: `refresh_vm_stats' defined but not used [-Wunused-function] (well, that was on a tree with different line numbering). So here's my attempt... Signed-off-by: Hugh Dickins --- Fix to merge into mm-proc-sys-vm-stat_refresh-to-force-vmstat-update.patch mm/vmstat.c |2 ++ 1 file changed, 2 insertions(+) --- 4.6-rc7-mm1/mm/vmstat.c 2016-05-14 08:29:10.609386264 -0700 +++ linux/mm/vmstat.c 2016-05-17 17:43:02.861862648 -0700 @@ -1365,6 +1365,7 @@ static struct workqueue_struct *vmstat_w static DEFINE_PER_CPU(struct delayed_work, vmstat_work); int sysctl_stat_interval __read_mostly = HZ; +#ifdef CONFIG_PROC_FS static void refresh_vm_stats(struct work_struct *work) { refresh_cpu_vm_stats(true); @@ -1422,6 +1423,7 @@ int vmstat_refresh(struct ctl_table *tab *lenp = 0; return 0; } +#endif /* CONFIG_PROC_FS */ static void vmstat_update(struct work_struct *w) {
Re: [PATCH] mm: unhide vmstat_text definition for CONFIG_SMP
On Mon 16-05-16 15:36:56, Andrew Morton wrote: > On Mon, 16 May 2016 16:23:33 +0200 Michal Hocko wrote: > > > Andrew, I think that the following is more straightforward fix and > > should be folded in to the patch which has introduced vmstat_refresh. > > --- > > >From b8dd18fb7df040e1bfe61aadde1d903589de15e4 Mon Sep 17 00:00:00 2001 > > From: Michal Hocko > > Date: Mon, 16 May 2016 16:19:53 +0200 > > Subject: [PATCH] mmotm: > > mm-proc-sys-vm-stat_refresh-to-force-vmstat-update-fix > > > > Arnd has reported: > > In randconfig builds with sysfs, procfs and numa all disabled, > > but SMP enabled, we now get a link error in the newly introduced > > vmstat_refresh function: > > > > mm/built-in.o: In function `vmstat_refresh': > > :(.text+0x15c78): undefined reference to `vmstat_text' > > > > vmstat_refresh is proc_fs specific so there is no reason to define it > > when !CONFIG_PROC_FS. > > I already had this: > > From: Christoph Lameter > Subject: Do not build vmstat_refresh if there is no procfs support > > It makes no sense to build functionality into the kernel that > cannot be used and causes build issues. > > Link: > http://lkml.kernel.org/r/alpine.deb.2.20.1605111011260.9...@east.gentwo.org > Signed-off-by: Christoph Lameter > Reported-by: Arnd Bergmann > Signed-off-by: Andrew Morton But this is broken: http://lkml.kernel.org/r/20160516073144.ga23...@dhcp22.suse.cz and kbuild robot agrees http://lkml.kernel.org/r/201605171333.anqjcwpy%fengguang...@intel.com > --- > > mm/vmstat.c |4 +++- > 1 file changed, 3 insertions(+), 1 deletion(-) > > diff -puN mm/vmstat.c~mm-proc-sys-vm-stat_refresh-to-force-vmstat-update-fix > mm/vmstat.c > --- a/mm/vmstat.c~mm-proc-sys-vm-stat_refresh-to-force-vmstat-update-fix > +++ a/mm/vmstat.c > @@ -1371,7 +1371,6 @@ static const struct file_operations proc > .llseek = seq_lseek, > .release= seq_release, > }; > -#endif /* CONFIG_PROC_FS */ > > #ifdef CONFIG_SMP > static struct workqueue_struct *vmstat_wq; > @@ -1436,7 +1435,10 @@ int vmstat_refresh(struct ctl_table *tab > *lenp = 0; > return 0; > } > +#endif /* CONFIG_SMP */ > +#endif /* CONFIG_PROC_FS */ > > +#ifdef CONFIG_SMP > static void vmstat_update(struct work_struct *w) > { > if (refresh_cpu_vm_stats(true)) { > _ -- Michal Hocko SUSE Labs
Re: [PATCH] mm: unhide vmstat_text definition for CONFIG_SMP
On Mon, 16 May 2016 16:23:33 +0200 Michal Hocko wrote: > Andrew, I think that the following is more straightforward fix and > should be folded in to the patch which has introduced vmstat_refresh. > --- > >From b8dd18fb7df040e1bfe61aadde1d903589de15e4 Mon Sep 17 00:00:00 2001 > From: Michal Hocko > Date: Mon, 16 May 2016 16:19:53 +0200 > Subject: [PATCH] mmotm: mm-proc-sys-vm-stat_refresh-to-force-vmstat-update-fix > > Arnd has reported: > In randconfig builds with sysfs, procfs and numa all disabled, > but SMP enabled, we now get a link error in the newly introduced > vmstat_refresh function: > > mm/built-in.o: In function `vmstat_refresh': > :(.text+0x15c78): undefined reference to `vmstat_text' > > vmstat_refresh is proc_fs specific so there is no reason to define it > when !CONFIG_PROC_FS. I already had this: From: Christoph Lameter Subject: Do not build vmstat_refresh if there is no procfs support It makes no sense to build functionality into the kernel that cannot be used and causes build issues. Link: http://lkml.kernel.org/r/alpine.deb.2.20.1605111011260.9...@east.gentwo.org Signed-off-by: Christoph Lameter Reported-by: Arnd Bergmann Signed-off-by: Andrew Morton --- mm/vmstat.c |4 +++- 1 file changed, 3 insertions(+), 1 deletion(-) diff -puN mm/vmstat.c~mm-proc-sys-vm-stat_refresh-to-force-vmstat-update-fix mm/vmstat.c --- a/mm/vmstat.c~mm-proc-sys-vm-stat_refresh-to-force-vmstat-update-fix +++ a/mm/vmstat.c @@ -1371,7 +1371,6 @@ static const struct file_operations proc .llseek = seq_lseek, .release= seq_release, }; -#endif /* CONFIG_PROC_FS */ #ifdef CONFIG_SMP static struct workqueue_struct *vmstat_wq; @@ -1436,7 +1435,10 @@ int vmstat_refresh(struct ctl_table *tab *lenp = 0; return 0; } +#endif /* CONFIG_SMP */ +#endif /* CONFIG_PROC_FS */ +#ifdef CONFIG_SMP static void vmstat_update(struct work_struct *w) { if (refresh_cpu_vm_stats(true)) { _
Re: [PATCH] mm: unhide vmstat_text definition for CONFIG_SMP
Andrew, I think that the following is more straightforward fix and should be folded in to the patch which has introduced vmstat_refresh. --- >From b8dd18fb7df040e1bfe61aadde1d903589de15e4 Mon Sep 17 00:00:00 2001 From: Michal Hocko Date: Mon, 16 May 2016 16:19:53 +0200 Subject: [PATCH] mmotm: mm-proc-sys-vm-stat_refresh-to-force-vmstat-update-fix Arnd has reported: In randconfig builds with sysfs, procfs and numa all disabled, but SMP enabled, we now get a link error in the newly introduced vmstat_refresh function: mm/built-in.o: In function `vmstat_refresh': :(.text+0x15c78): undefined reference to `vmstat_text' vmstat_refresh is proc_fs specific so there is no reason to define it when !CONFIG_PROC_FS. Reported-by: Arnd Bergmann Acked-by: Christoph Lameter Signed-off-by: Michal Hocko --- mm/vmstat.c | 2 ++ 1 file changed, 2 insertions(+) diff --git a/mm/vmstat.c b/mm/vmstat.c index 57a24e919907..c759b526287b 100644 --- a/mm/vmstat.c +++ b/mm/vmstat.c @@ -1370,6 +1370,7 @@ static void refresh_vm_stats(struct work_struct *work) refresh_cpu_vm_stats(true); } +#ifdef CONFIG_PROC_FS int vmstat_refresh(struct ctl_table *table, int write, void __user *buffer, size_t *lenp, loff_t *ppos) { @@ -1422,6 +1423,7 @@ int vmstat_refresh(struct ctl_table *table, int write, *lenp = 0; return 0; } +#endif static void vmstat_update(struct work_struct *w) { -- 2.8.1 -- Michal Hocko SUSE Labs
Re: [PATCH] mm: unhide vmstat_text definition for CONFIG_SMP
On Mon, 16 May 2016, Michal Hocko wrote: > I agree with Christoph that vmstat_refresh is PROC_FS only so we should > fix it there. It is not like this would be generally reusable helper... > Why don't we just do: Looks good. Acked-by: Christoph Lameter > --- > diff --git a/mm/vmstat.c b/mm/vmstat.c > index 57a24e919907..c759b526287b 100644 > --- a/mm/vmstat.c > +++ b/mm/vmstat.c > @@ -1370,6 +1370,7 @@ static void refresh_vm_stats(struct work_struct *work) > refresh_cpu_vm_stats(true); > } > > +#ifdef CONFIG_PROC_FS > int vmstat_refresh(struct ctl_table *table, int write, > void __user *buffer, size_t *lenp, loff_t *ppos) > { > @@ -1422,6 +1423,7 @@ int vmstat_refresh(struct ctl_table *table, int write, > *lenp = 0; > return 0; > } > +#endif > > static void vmstat_update(struct work_struct *w) > { >
Re: [PATCH] mm: unhide vmstat_text definition for CONFIG_SMP
On Wed 11-05-16 16:54:55, Arnd Bergmann wrote: > In randconfig builds with sysfs, procfs and numa all disabled, > but SMP enabled, we now get a link error in the newly introduced > vmstat_refresh function: > > mm/built-in.o: In function `vmstat_refresh': > :(.text+0x15c78): undefined reference to `vmstat_text' > > This modifes the already elaborate #ifdef to also cover that > configuration. > > Signed-off-by: Arnd Bergmann > Fixes: mmotm ("mm: /proc/sys/vm/stat_refresh to force vmstat update") I agree with Christoph that vmstat_refresh is PROC_FS only so we should fix it there. It is not like this would be generally reusable helper... Why don't we just do: --- diff --git a/mm/vmstat.c b/mm/vmstat.c index 57a24e919907..c759b526287b 100644 --- a/mm/vmstat.c +++ b/mm/vmstat.c @@ -1370,6 +1370,7 @@ static void refresh_vm_stats(struct work_struct *work) refresh_cpu_vm_stats(true); } +#ifdef CONFIG_PROC_FS int vmstat_refresh(struct ctl_table *table, int write, void __user *buffer, size_t *lenp, loff_t *ppos) { @@ -1422,6 +1423,7 @@ int vmstat_refresh(struct ctl_table *table, int write, *lenp = 0; return 0; } +#endif static void vmstat_update(struct work_struct *w) { -- Michal Hocko SUSE Labs
Re: [PATCH] mm: unhide vmstat_text definition for CONFIG_SMP
On Wed 11-05-16 10:32:11, Christoph Lameter wrote: > Subject: Do not build vmstat_refresh if there is no procfs support > > It makes no sense to build functionality into the kernel that > cannot be used and causes build issues. > > Signed-off-by: Christoph Lameter > > Index: linux/mm/vmstat.c > === > --- linux.orig/mm/vmstat.c > +++ linux/mm/vmstat.c > @@ -1358,7 +1358,6 @@ static const struct file_operations proc > .llseek = seq_lseek, > .release= seq_release, > }; > -#endif /* CONFIG_PROC_FS */ > > #ifdef CONFIG_SMP > static struct workqueue_struct *vmstat_wq; This doesn't work because it makes the whole vmstat_wq depend on CONFIG_PROC_FS. Which is obviously bad because we both rely on doing the periodic sync even when counters are not exported to the userspace and it wound't compile anyway... -- Michal Hocko SUSE Labs
Re: [PATCH] mm: unhide vmstat_text definition for CONFIG_SMP
On Wed, 11 May 2016, Arnd Bergmann wrote: > In randconfig builds with sysfs, procfs and numa all disabled, > but SMP enabled, we now get a link error in the newly introduced > vmstat_refresh function: > > mm/built-in.o: In function `vmstat_refresh': Hmmm... vmstat_refresh should not be build if CONFIG_PROC_FS is not set since there will be no way to trigger it. Lets not complicate this further. Subject: Do not build vmstat_refresh if there is no procfs support It makes no sense to build functionality into the kernel that cannot be used and causes build issues. Signed-off-by: Christoph Lameter Index: linux/mm/vmstat.c === --- linux.orig/mm/vmstat.c +++ linux/mm/vmstat.c @@ -1358,7 +1358,6 @@ static const struct file_operations proc .llseek = seq_lseek, .release= seq_release, }; -#endif /* CONFIG_PROC_FS */ #ifdef CONFIG_SMP static struct workqueue_struct *vmstat_wq; @@ -1422,7 +1421,10 @@ int vmstat_refresh(struct ctl_table *tab *lenp = 0; return 0; } +#endif /* CONFIG_SMP */ +#endif /* CONFIG_PROC_FS */ +#ifdef CONFIG_SMP static void vmstat_update(struct work_struct *w) { if (refresh_cpu_vm_stats(true)) {
[PATCH] mm: unhide vmstat_text definition for CONFIG_SMP
In randconfig builds with sysfs, procfs and numa all disabled, but SMP enabled, we now get a link error in the newly introduced vmstat_refresh function: mm/built-in.o: In function `vmstat_refresh': :(.text+0x15c78): undefined reference to `vmstat_text' This modifes the already elaborate #ifdef to also cover that configuration. Signed-off-by: Arnd Bergmann Fixes: mmotm ("mm: /proc/sys/vm/stat_refresh to force vmstat update") --- mm/vmstat.c | 5 +++-- 1 file changed, 3 insertions(+), 2 deletions(-) diff --git a/mm/vmstat.c b/mm/vmstat.c index 57a24e919907..5367eb9b858b 100644 --- a/mm/vmstat.c +++ b/mm/vmstat.c @@ -678,7 +678,8 @@ int fragmentation_index(struct zone *zone, unsigned int order) } #endif -#if defined(CONFIG_PROC_FS) || defined(CONFIG_SYSFS) || defined(CONFIG_NUMA) +#if defined(CONFIG_PROC_FS) || defined(CONFIG_SYSFS) || \ +defined(CONFIG_NUMA) || defined(CONFIG_SMP) #ifdef CONFIG_ZONE_DMA #define TEXT_FOR_DMA(xx) xx "_dma", #else @@ -857,7 +858,7 @@ const char * const vmstat_text[] = { #endif #endif /* CONFIG_VM_EVENTS_COUNTERS */ }; -#endif /* CONFIG_PROC_FS || CONFIG_SYSFS || CONFIG_NUMA */ +#endif /* CONFIG_PROC_FS || CONFIG_SYSFS || CONFIG_NUMA || CONFIG_SMP */ #if (defined(CONFIG_DEBUG_FS) && defined(CONFIG_COMPACTION)) || \ -- 2.7.0