LinuxLists.cc - [PATCH v5] kernel/time: move timer sysctls to its own file

2022-02-01 16:15:41

Subject: [PATCH v5] 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.

Signed-off-by: tangmeng <[email protected]>
---
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

2022-02-03 05:02:07

by kernel test robot

[permalink] [raw]

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

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/[email protected]/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 <[email protected]>

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/[email protected]

2022-02-03 13:54:28

by Luis Chamberlain

[permalink] [raw]

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

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

2022-02-03 17:23:50

by Thomas Gleixner

[permalink] [raw]

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

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

2022-02-03 17:56:33

by Thomas Gleixner

[permalink] [raw]

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

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

2022-02-03 23:20:32

by Luis Chamberlain

[permalink] [raw]

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

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

2022-02-05 18:34:24

by kernel test robot

[permalink] [raw]

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

2022-02-06 10:40:07

by Luis Chamberlain

[permalink] [raw]

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

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 <[email protected]>

Reviewed-by: Luis Chamberlain <[email protected]>

Luis