kernel/time: move timer sysctls to its own file
Subsystems (3)
PATCH v5 1/1 kernel/time: move timer sysctls to its own file
kernel/sysctl.c is a kitchen sink where everyone leaves their dirty dishes, this makes it very difficult to maintain.
To help with this maintenance let's start by moving sysctls to places where they actually belong. The proc sysctl maintainers do not want to know what sysctl knobs you wish to add for your own piece of code, we just care about the core logic.
So move the timer_migration sysctls to its own file.
--- include/linux/timer.h | 4 ---- kernel/sysctl.c | 11 ----------- kernel/time/timer.c | 26 ++++++++++++++++++++++++-- 3 files changed, 24 insertions(+), 17 deletions(-)
include/linux/timer.h +0 -4
@@ -198,10 +198,6 @@ extern enum hrtimer_restart it_real_fn(struct hrtimer *);#if defined(CONFIG_SMP ) && defined(CONFIG_NO_HZ_COMMON )struct ctl_table ; - -extern unsigned int sysctl_timer_migration; -int timer_migration_handler (struct ctl_table *table,int write, -void *buffer,size_t *lenp,loff_t *ppos);#endif unsigned long __round_jiffies (unsigned long j,int cpu);
kernel/sysctl.c +0 -11
@@ -2292,17 +2292,6 @@ static struct ctl_table kern_table[] = { .extra1 = SYSCTL_ZERO, .extra2 = SYSCTL_ONE, }, -#if defined(CONFIG_SMP) && defined(CONFIG_NO_HZ_COMMON) - { - .procname = "timer_migration", - .data = &sysctl_timer_migration, - .maxlen = sizeof(unsigned int), - .mode = 0644, - .proc_handler = timer_migration_handler, - .extra1 = SYSCTL_ZERO, - .extra2 = SYSCTL_ONE, - }, -#endif#ifdef CONFIG_BPF_SYSCALL { .procname = "unprivileged_bpf_disabled",
kernel/time/timer.c +24 -2
@@ -44,6 +44,7 @@#include <linux/slab.h> #include <linux/compat.h> #include <linux/random.h> +#include <linux/sysctl.h> #include <linux/uaccess.h> #include <asm/unistd.h>
@@ -223,7 +224,7 @@ static void timer_update_keys(struct work_struct *work); static DECLARE_WORK(timer_update_work, timer_update_keys);#ifdef CONFIG_SMP -unsigned int sysctl_timer_migration =1 ; +static unsigned int sysctl_timer_migration =1 ;DEFINE_STATIC_KEY_FALSE (timers_migration_enabled);
@@ -251,7 +252,8 @@ void timers_update_nohz(void)schedule_work (&timer_update_work); } -int timer_migration_handler (struct ctl_table *table,int write, +#if defined(CONFIG_SMP ) && defined(CONFIG_NO_HZ_COMMON ) +static int timer_migration_handler (struct ctl_table *table,int write,void *buffer,size_t *lenp,loff_t *ppos) {int ret;
@@ -264,6 +266,26 @@ int timer_migration_handler(struct ctl_table *table, int write, return ret; } +static struct ctl_table timer_sysctl[] = { + { + .procname ="timer_migration" , + .data = &sysctl_timer_migration, + .maxlen =sizeof (unsigned int ), + .mode =0644 , + .proc_handler = timer_migration_handler, + .extra1 =SYSCTL_ZERO , + .extra2 =SYSCTL_ONE , + }, + {} +}; + +static int __init timer_sysctl_init (void ) +{ +register_sysctl_init ("kernel" , timer_sysctl); +return 0 ; +} +__initcall (timer_sysctl_init); +#endif static inline bool is_timers_nohz_active (void ) {return static_branch_unlikely (&timers_nohz_active);
Signature · 1 line
2.20.1
kernel test robot
build botThank you for the patch! Yet something to improve:
Hi tangmeng,
Thank you for the patch! Yet something to improve:
[auto build test ERROR on tip/timers/core]
[also build test ERROR on linus/master kees/for-next/pstore v5.17-rc2 next-20220131] [If your patch is applied to the wrong git tree, kindly drop us a note. And when submitting patch, we suggest to use '--base' as documented in
https://git-scm.com/docs/git-format-patch]
url: https://github.com/0day-ci/linux/commits/tangmeng/kernel-time-move-timer-sysctls-to-its-own-file/20220131-182433 base: https://git.kernel.org/pub/scm/linux/kernel/git/tip/tip.git 35e13e9da9afbce13c1d36465504ece4e65f24fe config: arc-randconfig-r043-20220131 (https://download.01.org/0day-ci/archive/20220201/202202012049.OHxc9ybu-lkp(a)intel.com/config) compiler: arc-elf-gcc (GCC) 11.2.0
reproduce (this is a W=1 build):
wget https://raw.githubusercontent.com/intel/lkp-tests/master/sbin/make.cross -O ~/bin/make.cross chmod +x ~/bin/make.cross
# https://github.com/0day-ci/linux/commit/c54967d83b26ebacf4a1b686c6b659150b73306c
git remote add linux-review https://github.com/0day-ci/linux
git fetch --no-tags linux-review tangmeng/kernel-time-move-timer-sysctls-to-its-own-file/20220131-182433 git checkout c54967d83b26ebacf4a1b686c6b659150b73306c
# save the config file to linux build tree
mkdir build_dir
COMPILER_INSTALL_PATH=$HOME/0day COMPILER=gcc-11.2.0 make.cross O=build_dir ARCH=arc SHELL=/bin/bash kernel/time/
If you fix the issue, kindly add following tag as appropriate
All errors (new ones prefixed by >>):
kernel/time/timer.c: In function 'timer_sysctl_init':
>> kernel/time/timer.c:284:9: error: implicit declaration of function 'register_sysctl_init'; did you mean 'timer_sysctl_init'? [-Werror=implicit-function-declaration]
284 | register_sysctl_init("kernel", timer_sysctl);
| ^~~~~~~~~~~~~~~~~~~~
| timer_sysctl_init
In file included from include/linux/perf_event.h:25,
from include/linux/trace_events.h:10,
from include/trace/syscall.h:7,
from include/linux/syscalls.h:88,
from kernel/time/timer.c:35:
At top level:
arch/arc/include/asm/perf_event.h:126:27: warning: 'arc_pmu_cache_map' defined but not used [-Wunused-const-variable=]
126 | static const unsigned int arc_pmu_cache_map[C(MAX)][C(OP_MAX)][C(RESULT_MAX)] = {
| ^~~~~~~~~~~~~~~~~
arch/arc/include/asm/perf_event.h:91:27: warning: 'arc_pmu_ev_hw_map' defined but not used [-Wunused-const-variable=]
91 | static const char * const arc_pmu_ev_hw_map[] = {
| ^~~~~~~~~~~~~~~~~cc1: some warnings being treated as errors
vim +284 kernel/time/timer.c
281
282 static int __init timer_sysctl_init(void)
283 {
> 284 register_sysctl_init("kernel", timer_sysctl); 285 return 0;
286 }
287 __initcall(timer_sysctl_init);
288 #endif
289 static inline bool is_timers_nohz_active(void)
290 {
291 return static_branch_unlikely(&timers_nohz_active);
292 }
293 #else
294 static inline bool is_timers_nohz_active(void) { return false; }
295 #endif /* NO_HZ_COMMON */
296
---
0-DAY CI Kernel Test Service, Intel Corporation
https://lists.01.org/hyperkitty/list/kbuild-all(a)lists.01.org
kernel test robot
build botThank you for the patch! Yet something to improve:
Hi tangmeng,
Thank you for the patch! Yet something to improve:
[auto build test ERROR on tip/timers/core]
[also build test ERROR on linus/master kees/for-next/pstore v5.17-rc2 next-20220131] [If your patch is applied to the wrong git tree, kindly drop us a note. And when submitting patch, we suggest to use '--base' as documented in
https://git-scm.com/docs/git-format-patch]
url: https://github.com/0day-ci/linux/commits/tangmeng/kernel-time-move-timer-sysctls-to-its-own-file/20220131-182433 base: https://git.kernel.org/pub/scm/linux/kernel/git/tip/tip.git 35e13e9da9afbce13c1d36465504ece4e65f24fe config: s390-randconfig-r044-20220131 (https://download.01.org/0day-ci/archive/20220201/202202012006.823BFJHa-lkp(a)intel.com/config) compiler: clang version 14.0.0 (https://github.com/llvm/llvm-project 6b1e844b69f15bb7dffaf9365cd2b355d2eb7579)
reproduce (this is a W=1 build):
wget https://raw.githubusercontent.com/intel/lkp-tests/master/sbin/make.cross -O ~/bin/make.cross chmod +x ~/bin/make.cross
# install s390 cross compiling tool for clang build
# apt-get install binutils-s390x-linux-gnu
# https://github.com/0day-ci/linux/commit/c54967d83b26ebacf4a1b686c6b659150b73306c
git remote add linux-review https://github.com/0day-ci/linux
git fetch --no-tags linux-review tangmeng/kernel-time-move-timer-sysctls-to-its-own-file/20220131-182433 git checkout c54967d83b26ebacf4a1b686c6b659150b73306c
# save the config file to linux build tree
mkdir build_dir
COMPILER_INSTALL_PATH=$HOME/0day COMPILER=clang make.cross W=1 O=build_dir ARCH=s390 SHELL=/bin/bash kernel/time/
If you fix the issue, kindly add following tag as appropriate
All errors (new ones prefixed by >>):
In file included from kernel/time/timer.c:37:
In file included from include/linux/tick.h:8:
In file included from include/linux/clockchips.h:14:
In file included from include/linux/clocksource.h:22:
In file included from arch/s390/include/asm/io.h:75:
include/asm-generic/io.h:464:31: warning: performing pointer arithmetic on a null pointer has undefined behavior [-Wnull-pointer-arithmetic]
val = __raw_readb(PCI_IOBASE + addr);
~~~~~~~~~~ ^
include/asm-generic/io.h:477:61: warning: performing pointer arithmetic on a null pointer has undefined behavior [-Wnull-pointer-arithmetic]
val = __le16_to_cpu((__le16 __force)__raw_readw(PCI_IOBASE + addr));
~~~~~~~~~~ ^
include/uapi/linux/byteorder/big_endian.h:36:59: note: expanded from macro '__le16_to_cpu'
#define __le16_to_cpu(x) __swab16((__force __u16)(__le16)(x))
^
include/uapi/linux/swab.h:102:54: note: expanded from macro '__swab16'
#define __swab16(x) (__u16)__builtin_bswap16((__u16)(x))
^
In file included from kernel/time/timer.c:37:
In file included from include/linux/tick.h:8:
In file included from include/linux/clockchips.h:14:
In file included from include/linux/clocksource.h:22:
In file included from arch/s390/include/asm/io.h:75:
include/asm-generic/io.h:490:61: warning: performing pointer arithmetic on a null pointer has undefined behavior [-Wnull-pointer-arithmetic]
val = __le32_to_cpu((__le32 __force)__raw_readl(PCI_IOBASE + addr));
~~~~~~~~~~ ^
include/uapi/linux/byteorder/big_endian.h:34:59: note: expanded from macro '__le32_to_cpu'
#define __le32_to_cpu(x) __swab32((__force __u32)(__le32)(x))
^
include/uapi/linux/swab.h:115:54: note: expanded from macro '__swab32'
#define __swab32(x) (__u32)__builtin_bswap32((__u32)(x))
^
In file included from kernel/time/timer.c:37:
In file included from include/linux/tick.h:8:
In file included from include/linux/clockchips.h:14:
In file included from include/linux/clocksource.h:22:
In file included from arch/s390/include/asm/io.h:75:
include/asm-generic/io.h:501:33: warning: performing pointer arithmetic on a null pointer has undefined behavior [-Wnull-pointer-arithmetic]
__raw_writeb(value, PCI_IOBASE + addr);
~~~~~~~~~~ ^
include/asm-generic/io.h:511:59: warning: performing pointer arithmetic on a null pointer has undefined behavior [-Wnull-pointer-arithmetic]
__raw_writew((u16 __force)cpu_to_le16(value), PCI_IOBASE + addr);
~~~~~~~~~~ ^
include/asm-generic/io.h:521:59: warning: performing pointer arithmetic on a null pointer has undefined behavior [-Wnull-pointer-arithmetic]
__raw_writel((u32 __force)cpu_to_le32(value), PCI_IOBASE + addr);
~~~~~~~~~~ ^
include/asm-generic/io.h:609:20: warning: performing pointer arithmetic on a null pointer has undefined behavior [-Wnull-pointer-arithmetic]
readsb(PCI_IOBASE + addr, buffer, count);
~~~~~~~~~~ ^
include/asm-generic/io.h:617:20: warning: performing pointer arithmetic on a null pointer has undefined behavior [-Wnull-pointer-arithmetic]
readsw(PCI_IOBASE + addr, buffer, count);
~~~~~~~~~~ ^
include/asm-generic/io.h:625:20: warning: performing pointer arithmetic on a null pointer has undefined behavior [-Wnull-pointer-arithmetic]
readsl(PCI_IOBASE + addr, buffer, count);
~~~~~~~~~~ ^
include/asm-generic/io.h:634:21: warning: performing pointer arithmetic on a null pointer has undefined behavior [-Wnull-pointer-arithmetic]
writesb(PCI_IOBASE + addr, buffer, count);
~~~~~~~~~~ ^
include/asm-generic/io.h:643:21: warning: performing pointer arithmetic on a null pointer has undefined behavior [-Wnull-pointer-arithmetic]
writesw(PCI_IOBASE + addr, buffer, count);
~~~~~~~~~~ ^
include/asm-generic/io.h:652:21: warning: performing pointer arithmetic on a null pointer has undefined behavior [-Wnull-pointer-arithmetic]
writesl(PCI_IOBASE + addr, buffer, count);
~~~~~~~~~~ ^
>> kernel/time/timer.c:284:2: error: implicit declaration of function 'register_sysctl_init' [-Werror,-Wimplicit-function-declaration]
register_sysctl_init("kernel", timer_sysctl);
^12 warnings and 1 error generated.
vim +/register_sysctl_init +284 kernel/time/timer.c
281
282 static int __init timer_sysctl_init(void)
283 {
> 284 register_sysctl_init("kernel", timer_sysctl); 285 return 0;
286 }
287 __initcall(timer_sysctl_init);
288 #endif
289 static inline bool is_timers_nohz_active(void)
290 {
291 return static_branch_unlikely(&timers_nohz_active);
292 }
293 #else
294 static inline bool is_timers_nohz_active(void) { return false; }
295 #endif /* NO_HZ_COMMON */
296
---
0-DAY CI Kernel Test Service, Intel Corporation
https://lists.01.org/hyperkitty/list/kbuild-all(a)lists.01.org
Quoted 11 lines · tangmeng
kernel/sysctl.cis a kitchen sink where everyone leaves their dirty dishes, this makes it very difficult to maintain.To help with this maintenance let's start by moving sysctls to places where they actually belong. The proc sysctl maintainers do not want to know what sysctl knobs you wish to add for your own piece of code, we just care about the core logic.
So move the
timer_migrationsysctls to its own file.Signed-off-by tangmeng <[email protected]>
Luis
Tangmeng,
Quoted 2 lines · tangmeng
kernel/sysctl.cis a kitchen sink where everyone leaves their dirty dishes, this makes it very difficult to maintain.
Sorry. That's just a lame argument. What exactly is hard to maintain on that file? A large table of ifdeffed sysctl entries which changes once in a blue moon is hardly a maintenance problem.
Aside of that, sysctl.c is a very conveniant way to look up the zoo of sysctls which you now spread out all over the source tree.
So you really need to come up with a technical and sensical explanation for this change.
Quoted 4 lines
To help with this maintenance let's start by moving sysctls to places where they actually belong. The proc sysctl maintainers do not want to know what sysctl knobs you wish to add for your own piece of code, we just care about the core logic.
In other words, invite everyone to add random sysctls as they see fit w/o a central review authority. That's not an improvement at all. Quite the contrary.
That aside, I'm tired of this because this is now at V5 and you still failed to fix the fallout reported by the 0-day infrastructure vs. this part of the patch:
kernel/time/timer.c int timer_migration_handler(struct ctl_table *table, int write, · line 286
Quoted 5 lines
kernel/time/timer.c +5
-0+static int __init timer_sysctl_init (void ) +{ +register_sysctl_init ("kernel" , timer_sysctl); +return 0 ; +}
kernel/time/timer.c: In function 'timer_sysctl_init':
Quoted 1 line
Quoted 1 line
kernel/time/timer.c:284:9: error: implicit declaration of function 'register_sysctl_init'; did you mean 'timer_sysctl_init'? [-Werror=implicit-function-declaration]
284 | register_sysctl_init("kernel", timer_sysctl);
| ^~~~~~~~~~~~~~~~~~~~It's pretty damned obvious why this fails to compile and the 0-day reports have all the information you need to reproduce and address this, but you prefer to ignore it and just resend yet another incarnation.
Feel free to ignore these reports, but then please do not be surprised when I ignore your patches. Our development process is well documented and it's not subject to your personal interpretation.
Thanks,
tglx
Quoted 2 lines · Thomas Gleixner
In other words, invite everyone to add random sysctls as they see fit w/o a central review authority.
Everyone already can do that already. We can't stop that.
Quoted 2 lines
That's not an improvement at all. Quite
the contrary.
The idea is to move them to the respective subsystem / driver.
To be clear the argument put forwards to move sysctls out of one file was not started by tangmeng but by me and that work is already mostly merged for at least all the fs stuff. The rest of the patches coming through is help to move the other stuff to other areas.
The truth is kernel/sysctl.c as of the last 2 kernel releases before *was* huge and it can lead to tons of conflicts when doing merges. This makes it hard to maintain and even review changes.
*Today* all filesystem syctls now get reviewed by fs folks. They are all tidied up there.
In the future x86 folks can review their sysctls. But for no reason should I have to review every single knob. That's not scalable.
Quoted 11 lines
That aside, I'm tired of this because this is now at V5 and you still failed to fix the fallout reported by the 0-day infrastructure vs. this part of the patch:
kernel/time/timer.c int timer_migration_handler(struct ctl_table *table, int write, · line 286
Quoted 5 lines
kernel/time/timer.c +5
-0+static int __init timer_sysctl_init (void ) +{ +register_sysctl_init ("kernel" , timer_sysctl); +return 0 ; +}
kernel/time/timer.c: In function 'timer_sysctl_init':
Quoted 4 lines
Quoted 1 line
Quoted 1 line
kernel/time/timer.c:284:9: error: implicit declaration of function 'register_sysctl_init'; did you mean 'timer_sysctl_init'? [-Werror=implicit-function-declaration]284 | register_sysctl_init("kernel", timer_sysctl); | ^~~~~~~~~~~~~~~~~~~~
That's an issue with the patch being tested on a tree where that routine is not present?
Luis
Quoted 6 lines · Luis Chamberlain
On Thu, Feb 03, 2022 at 01:21:46AM +0100, Thomas Gleixner wrote:
*Today* all filesystem syctls now get reviewed by fs folks. They are all tidied up there.In the future x86 folks can review their sysctls. But for no reason should I have to review every single knob. That's not scalable.
Fair enough, but can we please have a changelog which explains the rationale to the people who have not been part of that discussion and decision.
Quoted 16 lines
Quoted 11 lines
That aside, I'm tired of this because this is now at V5 and you still failed to fix the fallout reported by the 0-day infrastructure vs. this part of the patch:
kernel/time/timer.c int timer_migration_handler(struct ctl_table *table, int write, · line 286
Quoted 5 lines
kernel/time/timer.c +5
-0+static int __init timer_sysctl_init (void ) +{ +register_sysctl_init ("kernel" , timer_sysctl); +return 0 ; +}
kernel/time/timer.c: In function 'timer_sysctl_init':Quoted 4 lines
Quoted 1 line
Quoted 1 line
kernel/time/timer.c:284:9: error: implicit declaration of function 'register_sysctl_init'; did you mean 'timer_sysctl_init'? [-Werror=implicit-function-declaration]284 | register_sysctl_init("kernel", timer_sysctl); | ^~~~~~~~~~~~~~~~~~~~
Quoted 2 lines
That's an issue with the patch being tested on a tree where that routine is not present?
From the report:
...
[also build test ERROR on linus/master
Linus tree has this interface. So that's not the problem.
Hint #1: The interfaxce is not available unconditionally
Hint #2: The 0-day reports provide the config file which exposes the
fail
Let me know if you need more hints. :)
Thanks,
tglx
Quoted 11 lines · Thomas Gleixner
Quoted 6 lines · Luis Chamberlain
On Thu, Feb 03, 2022 at 01:21:46AM +0100, Thomas Gleixner wrote:
*Today* all filesystem syctls now get reviewed by fs folks. They are all tidied up there.In the future x86 folks can review their sysctls. But for no reason should I have to review every single knob. That's not scalable.
Fair enough, but can we please have a changelog which explains the rationale to the people who have not been part of that discussion and decision.
Sure thing, tangmeng please update the commit log a bit better.
Quoted 24 lines
Quoted 16 lines
Quoted 11 lines
That aside, I'm tired of this because this is now at V5 and you still failed to fix the fallout reported by the 0-day infrastructure vs. this part of the patch:
kernel/time/timer.c int timer_migration_handler(struct ctl_table *table, int write, · line 286
Quoted 5 lines
kernel/time/timer.c +5
-0+static int __init timer_sysctl_init (void ) +{ +register_sysctl_init ("kernel" , timer_sysctl); +return 0 ; +}
kernel/time/timer.c: In function 'timer_sysctl_init':Quoted 4 lines
Quoted 1 line
Quoted 1 line
kernel/time/timer.c:284:9: error: implicit declaration of function 'register_sysctl_init'; did you mean 'timer_sysctl_init'? [-Werror=implicit-function-declaration]284 | register_sysctl_init("kernel", timer_sysctl); | ^~~~~~~~~~~~~~~~~~~~Quoted 2 lines
That's an issue with the patch being tested on a tree where that routine is not present?
From the report:
...
[also build test ERROR on linus/master
Quoted 6 lines
Linus tree has this interface. So that's not the problem.
Hint #1: The interfaxce is not available unconditionally
Hint #2: The 0-day reports provide the config file which exposes the
fail
tangmeng, please fix.
Luis