[PATCH v5] kernel/time: move timer sysctls to its own file

[PATCH v5] kernel/time: move timer sysctls to its own file

[tangmeng]

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.

Signed-off-by: tangmeng <tangmeng@uniontech.com>
---
 include/linux/timer.h |  4 ----
 kernel/sysctl.c       | 11 -----------
 kernel/time/timer.c   | 26 ++++++++++++++++++++++++--
 3 files changed, 24 insertions(+), 17 deletions(-)

diff --git a/include/linux/timer.h b/include/linux/timer.h
index fda13c9d1256..793b6b7c5a3e 100644
--- a/include/linux/timer.h
+++ b/include/linux/timer.h
@@ -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);
diff --git a/kernel/sysctl.c b/kernel/sysctl.c
index 5ae443b2882e..d6d133423e5d 100644
--- a/kernel/sysctl.c
+++ b/kernel/sysctl.c
@@ -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",
diff --git a/kernel/time/timer.c b/kernel/time/timer.c
index 85f1021ad459..2d3f4295614b 100644
--- a/kernel/time/timer.c
+++ b/kernel/time/timer.c
@@ -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);
-- 
2.20.1

Re: [PATCH v5] kernel/time: move timer sysctls to its own file

[kernel test robot]

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@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
Reported-by: kernel test robot <lkp@intel.com>

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@lists.01.org

Re: [PATCH v5] kernel/time: move timer sysctls to its own file

[kernel test robot]

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@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
Reported-by: kernel test robot <lkp@intel.com>

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@lists.01.org

Re: [PATCH v5] kernel/time: move timer sysctls to its own file

[Luis Chamberlain]

On Mon, Jan 31, 2022 at 06:22:14PM +0800, tangmeng wrote:
> 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.
> 
> Signed-off-by: tangmeng <tangmeng@uniontech.com>

Reviewed-by: Luis Chamberlain <mcgrof@kernel.org>

  Luis

Re: [PATCH v5] kernel/time: move timer sysctls to its own file

[Thomas Gleixner]

Tangmeng,

On Mon, Jan 31 2022 at 18:22, tangmeng wrote:
> kernel/sysctl.c is 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.

> 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:

> +static int __init timer_sysctl_init(void)
> +{
> +	register_sysctl_init("kernel", timer_sysctl);
> +	return 0;
> +}

    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);
	  |         ^~~~~~~~~~~~~~~~~~~~

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

Re: [PATCH v5] kernel/time: move timer sysctls to its own file

[Luis Chamberlain]

On Thu, Feb 03, 2022 at 01:21:46AM +0100, Thomas Gleixner wrote:
> 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.

> 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.

> 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:
> 
> > +static int __init timer_sysctl_init(void)
> > +{
> > +	register_sysctl_init("kernel", timer_sysctl);
> > +	return 0;
> > +}
> 
>     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);
> 	  |         ^~~~~~~~~~~~~~~~~~~~
> 

That's an issue with the patch being tested on a tree where that
routine is not present?

  Luis

Re: [PATCH v5] kernel/time: move timer sysctls to its own file

[Thomas Gleixner]

On Wed, Feb 02 2022 at 17:17, Luis Chamberlain wrote:
> 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.

>> 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:
>> 
>> > +static int __init timer_sysctl_init(void)
>> > +{
>> > +	register_sysctl_init("kernel", timer_sysctl);
>> > +	return 0;
>> > +}
>> 
>>     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);
>> 	  |         ^~~~~~~~~~~~~~~~~~~~
>> 
>
> 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

Re: [PATCH v5] kernel/time: move timer sysctls to its own file

[Luis Chamberlain]

On Thu, Feb 03, 2022 at 10:35:06AM +0100, Thomas Gleixner wrote:
> On Wed, Feb 02 2022 at 17:17, Luis Chamberlain wrote:
> > 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.

> >> 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:
> >> 
> >> > +static int __init timer_sysctl_init(void)
> >> > +{
> >> > +	register_sysctl_init("kernel", timer_sysctl);
> >> > +	return 0;
> >> > +}
> >> 
> >>     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);
> >> 	  |         ^~~~~~~~~~~~~~~~~~~~
> >> 
> >
> > 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

tangmeng, please fix.

  Luis