* [PATCH v3 0/2] timer: make deferrable cpu unbound timers really not bound to a cpu @ 2020-05-02 18:28 Prasad Sodagudi 2020-05-02 18:28 ` [PATCH v3 1/2] " Prasad Sodagudi 2020-05-02 18:28 ` [PATCH v3 2/2] sched: Add a check for cpu unbound deferrable timers Prasad Sodagudi 0 siblings, 2 replies; 12+ messages in thread From: Prasad Sodagudi @ 2020-05-02 18:28 UTC (permalink / raw) To: tglx, john.stultz, sboyd, tj; +Cc: linux-kernel, saravanak, psodagud, pkondeti These patches are introducing support for global deferrable timers and this feature is very useful for the mobile chipsets. All comments in initial discussion [1] are addressed. Here is the latest discussion - [2] as these patches are in downstream kernel for quite a long time. [1]- https://lore.kernel.org/patchwork/patch/500541/ [2]- https://lkml.org/lkml/2020/3/16/147 Changelog: v2 -> v3: - Fixed 'kbuild test robot' warnings. v1 -> v2: - Fixed tglx comments about warning. - Thanks Pavan for your suggestion. Joonwoo Park (1): timer: make deferrable cpu unbound timers really not bound to a cpu Prasad Sodagudi (1): sched: Add a check for cpu unbound deferrable timers include/linux/timer.h | 3 +++ kernel/time/tick-sched.c | 8 +++++- kernel/time/timer.c | 69 ++++++++++++++++++++++++++++++++++++++++++++---- 3 files changed, 74 insertions(+), 6 deletions(-) -- The Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum, a Linux Foundation Collaborative Project ^ permalink raw reply [flat|nested] 12+ messages in thread
* [PATCH v3 1/2] timer: make deferrable cpu unbound timers really not bound to a cpu 2020-05-02 18:28 [PATCH v3 0/2] timer: make deferrable cpu unbound timers really not bound to a cpu Prasad Sodagudi @ 2020-05-02 18:28 ` Prasad Sodagudi 2020-05-04 18:11 ` kbuild test robot ` (2 more replies) 2020-05-02 18:28 ` [PATCH v3 2/2] sched: Add a check for cpu unbound deferrable timers Prasad Sodagudi 1 sibling, 3 replies; 12+ messages in thread From: Prasad Sodagudi @ 2020-05-02 18:28 UTC (permalink / raw) To: tglx, john.stultz, sboyd, tj Cc: linux-kernel, saravanak, psodagud, pkondeti, Joonwoo Park From: Joonwoo Park <joonwoop@codeaurora.org> When a deferrable work (INIT_DEFERRABLE_WORK, etc.) is queued via queue_delayed_work() it's probably intended to run the work item on any CPU that isn't idle. However, we queue the work to run at a later time by starting a deferrable timer that binds to whatever CPU the work is queued on which is same with queue_delayed_work_on(smp_processor_id()) effectively. As a result WORK_CPU_UNBOUND work items aren't really cpu unbound now. In fact this is perfectly fine with UP kernel and also won't affect much a system without dyntick with SMP kernel too as every cpus run timers periodically. But on SMP systems with dyntick current implementation leads deferrable timers not very scalable because the timer's base which has queued the deferrable timer won't wake up till next non-deferrable timer expires even though there are possible other non idle cpus are running which are able to run expired deferrable timers. The deferrable work is a good example of the current implementation's victim like below. INIT_DEFERRABLE_WORK(&dwork, fn); CPU 0 CPU 1 queue_delayed_work(wq, &dwork, HZ); queue_delayed_work_on(WORK_CPU_UNBOUND); ... __mod_timer() -> queues timer to the current cpu's timer base. ... tick_nohz_idle_enter() -> cpu enters idle. A second later cpu 0 is now in idle. cpu 1 exits idle or wasn't in idle so now it's in active but won't cpu 0 won't wake up till next handle cpu unbound deferrable timer non-deferrable timer expires. as it's in cpu 0's timer base. To make all cpu unbound deferrable timers are scalable, introduce a common timer base which is only for cpu unbound deferrable timers to make those are indeed cpu unbound so that can be scheduled by any of non idle cpus. This common timer fixes scalability issue of delayed work and all other cpu unbound deferrable timer using implementations. Signed-off-by: Joonwoo Park <joonwoop@codeaurora.org> Signed-off-by: Prasad Sodagudi <psodagud@codeaurora.org> --- kernel/time/timer.c | 42 +++++++++++++++++++++++++++++++++++++----- 1 file changed, 37 insertions(+), 5 deletions(-) diff --git a/kernel/time/timer.c b/kernel/time/timer.c index a5221ab..1bf9b49 100644 --- a/kernel/time/timer.c +++ b/kernel/time/timer.c @@ -220,6 +220,7 @@ static void timer_update_keys(struct work_struct *work); static DECLARE_WORK(timer_update_work, timer_update_keys); #ifdef CONFIG_SMP +struct timer_base timer_base_deferrable; unsigned int sysctl_timer_migration = 1; DEFINE_STATIC_KEY_FALSE(timers_migration_enabled); @@ -841,8 +842,14 @@ static inline struct timer_base *get_timer_cpu_base(u32 tflags, u32 cpu) * If the timer is deferrable and NO_HZ_COMMON is set then we need * to use the deferrable base. */ - if (IS_ENABLED(CONFIG_NO_HZ_COMMON) && (tflags & TIMER_DEFERRABLE)) - base = per_cpu_ptr(&timer_bases[BASE_DEF], cpu); + if (IS_ENABLED(CONFIG_NO_HZ_COMMON) && (tflags & TIMER_DEFERRABLE)) { +#ifdef CONFIG_SMP + base = &timer_base_deferrable; +#endif + if (tflags & TIMER_PINNED) + base = per_cpu_ptr(&timer_bases[BASE_DEF], cpu); + } + return base; } @@ -854,8 +861,14 @@ static inline struct timer_base *get_timer_this_cpu_base(u32 tflags) * If the timer is deferrable and NO_HZ_COMMON is set then we need * to use the deferrable base. */ - if (IS_ENABLED(CONFIG_NO_HZ_COMMON) && (tflags & TIMER_DEFERRABLE)) - base = this_cpu_ptr(&timer_bases[BASE_DEF]); + if (IS_ENABLED(CONFIG_NO_HZ_COMMON) && (tflags & TIMER_DEFERRABLE)) { +#ifdef CONFIG_SMP + base = &timer_base_deferrable; +#endif + if (tflags & TIMER_PINNED) + base = this_cpu_ptr(&timer_bases[BASE_DEF]); + } + return base; } @@ -1785,8 +1798,14 @@ static __latent_entropy void run_timer_softirq(struct softirq_action *h) struct timer_base *base = this_cpu_ptr(&timer_bases[BASE_STD]); __run_timers(base); - if (IS_ENABLED(CONFIG_NO_HZ_COMMON)) + if (IS_ENABLED(CONFIG_NO_HZ_COMMON)) { __run_timers(this_cpu_ptr(&timer_bases[BASE_DEF])); +#ifdef CONFIG_SMP + if (tick_do_timer_cpu == TICK_DO_TIMER_NONE || + tick_do_timer_cpu == smp_processor_id()) + __run_timers(&timer_base_deferrable); +#endif + } } /* @@ -2025,6 +2044,16 @@ static void __init init_timer_cpu(int cpu) } } +#if defined(CONFIG_NO_HZ_COMMON) && defined(CONFIG_SMP) +static void __init init_timer_deferrable_global(void) +{ + timer_base_deferrable.cpu = nr_cpu_ids; + raw_spin_lock_init(&timer_base_deferrable.lock); + timer_base_deferrable.clk = jiffies; + timer_base_init_expiry_lock(&timer_base_deferrable); +} +#endif + static void __init init_timer_cpus(void) { int cpu; @@ -2036,6 +2065,9 @@ static void __init init_timer_cpus(void) void __init init_timers(void) { init_timer_cpus(); +#if defined(CONFIG_NO_HZ_COMMON) && defined(CONFIG_SMP) + init_timer_deferrable_global(); +#endif open_softirq(TIMER_SOFTIRQ, run_timer_softirq); } -- The Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum, a Linux Foundation Collaborative Project ^ permalink raw reply related [flat|nested] 12+ messages in thread
* Re: [PATCH v3 1/2] timer: make deferrable cpu unbound timers really not bound to a cpu 2020-05-02 18:28 ` [PATCH v3 1/2] " Prasad Sodagudi @ 2020-05-04 18:11 ` kbuild test robot 2020-05-05 0:08 ` kbuild test robot 2020-05-06 13:28 ` Thomas Gleixner 2 siblings, 0 replies; 12+ messages in thread From: kbuild test robot @ 2020-05-04 18:11 UTC (permalink / raw) To: Prasad Sodagudi, tglx, john.stultz, sboyd, tj Cc: kbuild-all, linux-kernel, saravanak, psodagud, pkondeti, Joonwoo Park [-- Attachment #1: Type: text/plain, Size: 2895 bytes --] Hi Prasad, Thank you for the patch! Yet something to improve: [auto build test ERROR on tip/timers/core] [also build test ERROR on tip/auto-latest tip/timers/nohz v5.7-rc4 next-20200501] [if your patch is applied to the wrong git tree, please drop us a note to help improve the system. BTW, we also suggest to use '--base' option to specify the base tree in git format-patch, please see https://stackoverflow.com/a/37406982] url: https://github.com/0day-ci/linux/commits/Prasad-Sodagudi/timer-make-deferrable-cpu-unbound-timers-really-not-bound-to-a-cpu/20200503-025049 base: https://git.kernel.org/pub/scm/linux/kernel/git/tip/tip.git 4479730e9263befbb9ce9563a09563db2acb8f7c config: riscv-nommu_virt_defconfig (attached as .config) compiler: riscv64-linux-gcc (GCC) 9.3.0 reproduce: wget https://raw.githubusercontent.com/intel/lkp-tests/master/sbin/make.cross -O ~/bin/make.cross chmod +x ~/bin/make.cross # save the attached .config to linux build tree COMPILER_INSTALL_PATH=$HOME/0day GCC_VERSION=9.3.0 make.cross ARCH=riscv If you fix the issue, kindly add following tag as appropriate Reported-by: kbuild test robot <lkp@intel.com> All errors (new ones prefixed by >>): kernel/time/timer.c: In function 'get_timer_cpu_base': >> kernel/time/timer.c:847:11: error: 'timer_base_deferrable' undeclared (first use in this function) 847 | base = &timer_base_deferrable; | ^~~~~~~~~~~~~~~~~~~~~ kernel/time/timer.c:847:11: note: each undeclared identifier is reported only once for each function it appears in kernel/time/timer.c: In function 'get_timer_this_cpu_base': kernel/time/timer.c:866:11: error: 'timer_base_deferrable' undeclared (first use in this function) 866 | base = &timer_base_deferrable; | ^~~~~~~~~~~~~~~~~~~~~ kernel/time/timer.c: In function 'run_timer_softirq': kernel/time/timer.c:1805:18: error: 'timer_base_deferrable' undeclared (first use in this function) 1805 | __run_timers(&timer_base_deferrable); | ^~~~~~~~~~~~~~~~~~~~~ vim +/timer_base_deferrable +847 kernel/time/timer.c 836 837 static inline struct timer_base *get_timer_cpu_base(u32 tflags, u32 cpu) 838 { 839 struct timer_base *base = per_cpu_ptr(&timer_bases[BASE_STD], cpu); 840 841 /* 842 * If the timer is deferrable and NO_HZ_COMMON is set then we need 843 * to use the deferrable base. 844 */ 845 if (IS_ENABLED(CONFIG_NO_HZ_COMMON) && (tflags & TIMER_DEFERRABLE)) { 846 #ifdef CONFIG_SMP > 847 base = &timer_base_deferrable; 848 #endif 849 if (tflags & TIMER_PINNED) 850 base = per_cpu_ptr(&timer_bases[BASE_DEF], cpu); 851 } 852 853 return base; 854 } 855 --- 0-DAY CI Kernel Test Service, Intel Corporation https://lists.01.org/hyperkitty/list/kbuild-all@lists.01.org [-- Attachment #2: .config.gz --] [-- Type: application/gzip, Size: 6789 bytes --] ^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [PATCH v3 1/2] timer: make deferrable cpu unbound timers really not bound to a cpu 2020-05-02 18:28 ` [PATCH v3 1/2] " Prasad Sodagudi 2020-05-04 18:11 ` kbuild test robot @ 2020-05-05 0:08 ` kbuild test robot 2020-05-06 13:28 ` Thomas Gleixner 2 siblings, 0 replies; 12+ messages in thread From: kbuild test robot @ 2020-05-05 0:08 UTC (permalink / raw) To: Prasad Sodagudi, tglx, john.stultz, sboyd, tj Cc: kbuild-all, linux-kernel, saravanak, psodagud, pkondeti, Joonwoo Park [-- Attachment #1: Type: text/plain, Size: 3360 bytes --] Hi Prasad, Thank you for the patch! Yet something to improve: [auto build test ERROR on tip/timers/core] [also build test ERROR on tip/auto-latest tip/timers/nohz v5.7-rc4 next-20200504] [if your patch is applied to the wrong git tree, please drop us a note to help improve the system. BTW, we also suggest to use '--base' option to specify the base tree in git format-patch, please see https://stackoverflow.com/a/37406982] url: https://github.com/0day-ci/linux/commits/Prasad-Sodagudi/timer-make-deferrable-cpu-unbound-timers-really-not-bound-to-a-cpu/20200503-025049 base: https://git.kernel.org/pub/scm/linux/kernel/git/tip/tip.git 4479730e9263befbb9ce9563a09563db2acb8f7c config: ia64-randconfig-a001-20200505 (attached as .config) compiler: ia64-linux-gcc (GCC) 9.3.0 reproduce: wget https://raw.githubusercontent.com/intel/lkp-tests/master/sbin/make.cross -O ~/bin/make.cross chmod +x ~/bin/make.cross # save the attached .config to linux build tree COMPILER_INSTALL_PATH=$HOME/0day GCC_VERSION=9.3.0 make.cross ARCH=ia64 If you fix the issue, kindly add following tag as appropriate Reported-by: kbuild test robot <lkp@intel.com> All errors (new ones prefixed by >>): kernel/time/timer.c: In function 'get_timer_cpu_base': kernel/time/timer.c:847:11: error: 'timer_base_deferrable' undeclared (first use in this function) 847 | base = &timer_base_deferrable; | ^~~~~~~~~~~~~~~~~~~~~ kernel/time/timer.c:847:11: note: each undeclared identifier is reported only once for each function it appears in kernel/time/timer.c: In function 'get_timer_this_cpu_base': kernel/time/timer.c:866:11: error: 'timer_base_deferrable' undeclared (first use in this function) 866 | base = &timer_base_deferrable; | ^~~~~~~~~~~~~~~~~~~~~ kernel/time/timer.c: In function 'run_timer_softirq': >> kernel/time/timer.c:1803:7: error: 'tick_do_timer_cpu' undeclared (first use in this function); did you mean 'tick_dep_clear_cpu'? 1803 | if (tick_do_timer_cpu == TICK_DO_TIMER_NONE || | ^~~~~~~~~~~~~~~~~ | tick_dep_clear_cpu >> kernel/time/timer.c:1803:28: error: 'TICK_DO_TIMER_NONE' undeclared (first use in this function) 1803 | if (tick_do_timer_cpu == TICK_DO_TIMER_NONE || | ^~~~~~~~~~~~~~~~~~ kernel/time/timer.c:1805:18: error: 'timer_base_deferrable' undeclared (first use in this function) 1805 | __run_timers(&timer_base_deferrable); | ^~~~~~~~~~~~~~~~~~~~~ vim +1803 kernel/time/timer.c 1791 1792 /* 1793 * This function runs timers and the timer-tq in bottom half context. 1794 */ 1795 static __latent_entropy void run_timer_softirq(struct softirq_action *h) 1796 { 1797 struct timer_base *base = this_cpu_ptr(&timer_bases[BASE_STD]); 1798 1799 __run_timers(base); 1800 if (IS_ENABLED(CONFIG_NO_HZ_COMMON)) { 1801 __run_timers(this_cpu_ptr(&timer_bases[BASE_DEF])); 1802 #ifdef CONFIG_SMP > 1803 if (tick_do_timer_cpu == TICK_DO_TIMER_NONE || 1804 tick_do_timer_cpu == smp_processor_id()) 1805 __run_timers(&timer_base_deferrable); 1806 #endif 1807 } 1808 } 1809 --- 0-DAY CI Kernel Test Service, Intel Corporation https://lists.01.org/hyperkitty/list/kbuild-all@lists.01.org [-- Attachment #2: .config.gz --] [-- Type: application/gzip, Size: 31009 bytes --] ^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [PATCH v3 1/2] timer: make deferrable cpu unbound timers really not bound to a cpu 2020-05-02 18:28 ` [PATCH v3 1/2] " Prasad Sodagudi 2020-05-04 18:11 ` kbuild test robot 2020-05-05 0:08 ` kbuild test robot @ 2020-05-06 13:28 ` Thomas Gleixner 2020-05-13 19:53 ` psodagud 2 siblings, 1 reply; 12+ messages in thread From: Thomas Gleixner @ 2020-05-06 13:28 UTC (permalink / raw) To: Prasad Sodagudi, john.stultz, sboyd, tj Cc: linux-kernel, saravanak, psodagud, pkondeti, Joonwoo Park Prasad Sodagudi <psodagud@codeaurora.org> writes: > To make all cpu unbound deferrable timers are scalable, introduce a common > timer base which is only for cpu unbound deferrable timers to make those > are indeed cpu unbound so that can be scheduled by any of non idle cpus. > This common timer fixes scalability issue of delayed work and all other cpu > unbound deferrable timer using implementations. Scalability? That's really the wrong term here. A global timer base is the opposite and you really want to explain why this is not creating a scalability problem on large systems. > #ifdef CONFIG_SMP > +struct timer_base timer_base_deferrable; > unsigned int sysctl_timer_migration = 1; > > DEFINE_STATIC_KEY_FALSE(timers_migration_enabled); > @@ -841,8 +842,14 @@ static inline struct timer_base *get_timer_cpu_base(u32 tflags, u32 cpu) > * If the timer is deferrable and NO_HZ_COMMON is set then we need > * to use the deferrable base. > */ > - if (IS_ENABLED(CONFIG_NO_HZ_COMMON) && (tflags & TIMER_DEFERRABLE)) > - base = per_cpu_ptr(&timer_bases[BASE_DEF], cpu); > + if (IS_ENABLED(CONFIG_NO_HZ_COMMON) && (tflags & TIMER_DEFERRABLE)) { > +#ifdef CONFIG_SMP > + base = &timer_base_deferrable; > +#endif There are definitely smarter ways of solving this than sprinkling #ifdef's around the code. > + if (tflags & TIMER_PINNED) > + base = per_cpu_ptr(&timer_bases[BASE_DEF], cpu); > + } > + > return base; > } > @@ -1785,8 +1798,14 @@ static __latent_entropy void run_timer_softirq(struct softirq_action *h) > struct timer_base *base = this_cpu_ptr(&timer_bases[BASE_STD]); > > __run_timers(base); > - if (IS_ENABLED(CONFIG_NO_HZ_COMMON)) > + if (IS_ENABLED(CONFIG_NO_HZ_COMMON)) { > __run_timers(this_cpu_ptr(&timer_bases[BASE_DEF])); > +#ifdef CONFIG_SMP > + if (tick_do_timer_cpu == TICK_DO_TIMER_NONE || > + tick_do_timer_cpu == smp_processor_id()) > + __run_timers(&timer_base_deferrable); > +#endif Again, this can be solved in readable ways. Just slapping #ifdefs all over the place is sloppy and lazy. Aside of that accessing the tick internals here open coded is just a layering violation. > + } > } > > /* > @@ -2025,6 +2044,16 @@ static void __init init_timer_cpu(int cpu) > } > } > > +#if defined(CONFIG_NO_HZ_COMMON) && defined(CONFIG_SMP) > +static void __init init_timer_deferrable_global(void) > +{ > + timer_base_deferrable.cpu = nr_cpu_ids; This was obviously never tested with CONFIG_DEBUG_PER_CPU_MAPS=y as this will simply result in out of bounds accesses. > static void __init init_timer_cpus(void) > { > int cpu; > @@ -2036,6 +2065,9 @@ static void __init init_timer_cpus(void) > void __init init_timers(void) > { > init_timer_cpus(); > +#if defined(CONFIG_NO_HZ_COMMON) && defined(CONFIG_SMP) > + init_timer_deferrable_global(); > +#endif Stub functions exist to avoid this unreadable #ifdef garbage. Thanks, tglx ^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [PATCH v3 1/2] timer: make deferrable cpu unbound timers really not bound to a cpu 2020-05-06 13:28 ` Thomas Gleixner @ 2020-05-13 19:53 ` psodagud 2020-05-13 20:28 ` Thomas Gleixner 0 siblings, 1 reply; 12+ messages in thread From: psodagud @ 2020-05-13 19:53 UTC (permalink / raw) To: Thomas Gleixner Cc: john.stultz, sboyd, tj, linux-kernel, saravanak, pkondeti, Joonwoo Park Hi Tglx, On 2020-05-06 06:28, Thomas Gleixner wrote: > Prasad Sodagudi <psodagud@codeaurora.org> writes: >> To make all cpu unbound deferrable timers are scalable, introduce a >> common >> timer base which is only for cpu unbound deferrable timers to make >> those >> are indeed cpu unbound so that can be scheduled by any of non idle >> cpus. >> This common timer fixes scalability issue of delayed work and all >> other cpu >> unbound deferrable timer using implementations. > > Scalability? That's really the wrong term here. A global timer base is > the opposite and you really want to explain why this is not creating a > scalability problem on large systems. > >> #ifdef CONFIG_SMP >> +struct timer_base timer_base_deferrable; >> unsigned int sysctl_timer_migration = 1; >> >> DEFINE_STATIC_KEY_FALSE(timers_migration_enabled); >> @@ -841,8 +842,14 @@ static inline struct timer_base >> *get_timer_cpu_base(u32 tflags, u32 cpu) >> * If the timer is deferrable and NO_HZ_COMMON is set then we need >> * to use the deferrable base. >> */ >> - if (IS_ENABLED(CONFIG_NO_HZ_COMMON) && (tflags & TIMER_DEFERRABLE)) >> - base = per_cpu_ptr(&timer_bases[BASE_DEF], cpu); >> + if (IS_ENABLED(CONFIG_NO_HZ_COMMON) && (tflags & TIMER_DEFERRABLE)) >> { >> +#ifdef CONFIG_SMP >> + base = &timer_base_deferrable; >> +#endif > > There are definitely smarter ways of solving this than sprinkling > #ifdef's around the code. I am able to understand all other comments and I will address all those comments in the next patch set. It is not clear to me how to avoid #ifdef's in this case. Could you please share an example here? > >> + if (tflags & TIMER_PINNED) >> + base = per_cpu_ptr(&timer_bases[BASE_DEF], cpu); >> + } >> + >> return base; >> } >> @@ -1785,8 +1798,14 @@ static __latent_entropy void >> run_timer_softirq(struct softirq_action *h) >> struct timer_base *base = this_cpu_ptr(&timer_bases[BASE_STD]); >> >> __run_timers(base); >> - if (IS_ENABLED(CONFIG_NO_HZ_COMMON)) >> + if (IS_ENABLED(CONFIG_NO_HZ_COMMON)) { >> __run_timers(this_cpu_ptr(&timer_bases[BASE_DEF])); >> +#ifdef CONFIG_SMP >> + if (tick_do_timer_cpu == TICK_DO_TIMER_NONE || >> + tick_do_timer_cpu == smp_processor_id()) >> + __run_timers(&timer_base_deferrable); >> +#endif > > Again, this can be solved in readable ways. Just slapping #ifdefs all > over the place is sloppy and lazy. Sorry. I will try to address this. It is not clear to me how to avoid #ifdefs in this case too. Please provide me more information. > > Aside of that accessing the tick internals here open coded is just a > layering violation. I will fix this and avoid using referring to tick internals here. > >> + } >> } >> >> /* >> @@ -2025,6 +2044,16 @@ static void __init init_timer_cpu(int cpu) >> } >> } >> >> +#if defined(CONFIG_NO_HZ_COMMON) && defined(CONFIG_SMP) >> +static void __init init_timer_deferrable_global(void) >> +{ >> + timer_base_deferrable.cpu = nr_cpu_ids; > > This was obviously never tested with CONFIG_DEBUG_PER_CPU_MAPS=y as > this > will simply result in out of bounds accesses. Sure. I will test this CONFIG_DEBUG_PER_CPU_MAPS=y enabled before pushing the next patch set. > >> static void __init init_timer_cpus(void) >> { >> int cpu; >> @@ -2036,6 +2065,9 @@ static void __init init_timer_cpus(void) >> void __init init_timers(void) >> { >> init_timer_cpus(); >> +#if defined(CONFIG_NO_HZ_COMMON) && defined(CONFIG_SMP) >> + init_timer_deferrable_global(); >> +#endif > > Stub functions exist to avoid this unreadable #ifdef garbage. Got it. I will fix this in the next patch set. > > Thanks, > > tglx ^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [PATCH v3 1/2] timer: make deferrable cpu unbound timers really not bound to a cpu 2020-05-13 19:53 ` psodagud @ 2020-05-13 20:28 ` Thomas Gleixner 2020-05-13 20:55 ` psodagud 0 siblings, 1 reply; 12+ messages in thread From: Thomas Gleixner @ 2020-05-13 20:28 UTC (permalink / raw) To: psodagud Cc: john.stultz, sboyd, tj, linux-kernel, saravanak, pkondeti, Joonwoo Park psodagud@codeaurora.org writes: > On 2020-05-06 06:28, Thomas Gleixner wrote: >>> #ifdef CONFIG_SMP >>> +struct timer_base timer_base_deferrable; >>> unsigned int sysctl_timer_migration = 1; >>> >>> DEFINE_STATIC_KEY_FALSE(timers_migration_enabled); >>> @@ -841,8 +842,14 @@ static inline struct timer_base >>> *get_timer_cpu_base(u32 tflags, u32 cpu) >>> * If the timer is deferrable and NO_HZ_COMMON is set then we need >>> * to use the deferrable base. >>> */ >>> - if (IS_ENABLED(CONFIG_NO_HZ_COMMON) && (tflags & TIMER_DEFERRABLE)) >>> - base = per_cpu_ptr(&timer_bases[BASE_DEF], cpu); >>> + if (IS_ENABLED(CONFIG_NO_HZ_COMMON) && (tflags & TIMER_DEFERRABLE)) >>> { >>> +#ifdef CONFIG_SMP >>> + base = &timer_base_deferrable; >>> +#endif >> >> There are definitely smarter ways of solving this than sprinkling >> #ifdef's around the code. > > I am able to understand all other comments and I will address all those > comments in the next patch set. > It is not clear to me how to avoid #ifdef's in this case. Could you > please share an example here? The answer is further down already: >> Stub functions exist to avoid this unreadable #ifdef garbage. ^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [PATCH v3 1/2] timer: make deferrable cpu unbound timers really not bound to a cpu 2020-05-13 20:28 ` Thomas Gleixner @ 2020-05-13 20:55 ` psodagud 2020-05-13 21:34 ` Thomas Gleixner 0 siblings, 1 reply; 12+ messages in thread From: psodagud @ 2020-05-13 20:55 UTC (permalink / raw) To: Thomas Gleixner Cc: john.stultz, sboyd, tj, linux-kernel, saravanak, pkondeti, Joonwoo Park On 2020-05-13 13:28, Thomas Gleixner wrote: > psodagud@codeaurora.org writes: >> On 2020-05-06 06:28, Thomas Gleixner wrote: >>>> #ifdef CONFIG_SMP >>>> +struct timer_base timer_base_deferrable; >>>> unsigned int sysctl_timer_migration = 1; >>>> >>>> DEFINE_STATIC_KEY_FALSE(timers_migration_enabled); >>>> @@ -841,8 +842,14 @@ static inline struct timer_base >>>> *get_timer_cpu_base(u32 tflags, u32 cpu) >>>> * If the timer is deferrable and NO_HZ_COMMON is set then we need >>>> * to use the deferrable base. >>>> */ >>>> - if (IS_ENABLED(CONFIG_NO_HZ_COMMON) && (tflags & >>>> TIMER_DEFERRABLE)) >>>> - base = per_cpu_ptr(&timer_bases[BASE_DEF], cpu); >>>> + if (IS_ENABLED(CONFIG_NO_HZ_COMMON) && (tflags & >>>> TIMER_DEFERRABLE)) >>>> { >>>> +#ifdef CONFIG_SMP >>>> + base = &timer_base_deferrable; >>>> +#endif >>> >>> There are definitely smarter ways of solving this than sprinkling >>> #ifdef's around the code. >> >> I am able to understand all other comments and I will address all >> those >> comments in the next patch set. >> It is not clear to me how to avoid #ifdef's in this case. Could you >> please share an example here? > > The answer is further down already: Thanks Tglx for quick response. I think, you are referring stub functions. Yes. I can reduce some of the #ifdefs with stub functions as you mentioned and not all the cases right? I have introduced two variables timer_base_deferrable and deferrable_pending and I can put stub function where ever is possible. But it may not be appropriate to have stub function for all the references of these variables right? Correct me if my understanding is wrong. -Thanks, Prasad > >>> Stub functions exist to avoid this unreadable #ifdef garbage. ^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [PATCH v3 1/2] timer: make deferrable cpu unbound timers really not bound to a cpu 2020-05-13 20:55 ` psodagud @ 2020-05-13 21:34 ` Thomas Gleixner 0 siblings, 0 replies; 12+ messages in thread From: Thomas Gleixner @ 2020-05-13 21:34 UTC (permalink / raw) To: psodagud Cc: john.stultz, sboyd, tj, linux-kernel, saravanak, pkondeti, Joonwoo Park Prasad, psodagud@codeaurora.org writes: > On 2020-05-13 13:28, Thomas Gleixner wrote: >> psodagud@codeaurora.org writes: >>> It is not clear to me how to avoid #ifdef's in this case. Could you >>> please share an example here? >> >> The answer is further down already: > > I think, you are referring stub functions. Yes. I can reduce some of the > #ifdefs with stub functions as you mentioned and not all the cases > right? > I have introduced two variables timer_base_deferrable and > deferrable_pending and I can put stub function where ever is possible. > But it may not be appropriate to have stub function for all the > references of these variables right? Correct me if my understanding is > wrong. Is this a quiz or are you expecting me to make your homework? Thanks, Thomas ^ permalink raw reply [flat|nested] 12+ messages in thread
* [PATCH v3 2/2] sched: Add a check for cpu unbound deferrable timers 2020-05-02 18:28 [PATCH v3 0/2] timer: make deferrable cpu unbound timers really not bound to a cpu Prasad Sodagudi 2020-05-02 18:28 ` [PATCH v3 1/2] " Prasad Sodagudi @ 2020-05-02 18:28 ` Prasad Sodagudi 2020-05-04 19:11 ` kbuild test robot 2020-05-06 14:03 ` Thomas Gleixner 1 sibling, 2 replies; 12+ messages in thread From: Prasad Sodagudi @ 2020-05-02 18:28 UTC (permalink / raw) To: tglx, john.stultz, sboyd, tj; +Cc: linux-kernel, saravanak, psodagud, pkondeti Add a check to find expired unbound deferrable timers and trigger softirq for handling timers. This way a CPU can process all the expired deferrable timers whenever it is out off idle state due to an interrupt. Signed-off-by: Prasad Sodagudi <psodagud@codeaurora.org> --- include/linux/timer.h | 3 +++ kernel/time/tick-sched.c | 8 +++++++- kernel/time/timer.c | 29 ++++++++++++++++++++++++++++- 3 files changed, 38 insertions(+), 2 deletions(-) diff --git a/include/linux/timer.h b/include/linux/timer.h index 0dc19a8..e85dd2d 100644 --- a/include/linux/timer.h +++ b/include/linux/timer.h @@ -172,6 +172,9 @@ extern int del_timer(struct timer_list * timer); extern int mod_timer(struct timer_list *timer, unsigned long expires); extern int mod_timer_pending(struct timer_list *timer, unsigned long expires); extern int timer_reduce(struct timer_list *timer, unsigned long expires); +#ifdef CONFIG_SMP +extern bool check_pending_deferrable_timers(int cpu); +#endif /* * The jiffies value which is added to now, when there is no timer diff --git a/kernel/time/tick-sched.c b/kernel/time/tick-sched.c index 3e2dc9b..16aec80 100644 --- a/kernel/time/tick-sched.c +++ b/kernel/time/tick-sched.c @@ -23,6 +23,7 @@ #include <linux/module.h> #include <linux/irq_work.h> #include <linux/posix-timers.h> +#include <linux/timer.h> #include <linux/context_tracking.h> #include <linux/mm.h> @@ -1274,8 +1275,13 @@ static inline void tick_nohz_irq_enter(void) now = ktime_get(); if (ts->idle_active) tick_nohz_stop_idle(ts, now); - if (ts->tick_stopped) + if (ts->tick_stopped) { tick_nohz_update_jiffies(now); +#ifdef CONFIG_SMP + if (check_pending_deferrable_timers(smp_processor_id())) + raise_softirq_irqoff(TIMER_SOFTIRQ); +#endif + } } #else diff --git a/kernel/time/timer.c b/kernel/time/timer.c index 1bf9b49..5947c63 100644 --- a/kernel/time/timer.c +++ b/kernel/time/timer.c @@ -221,6 +221,7 @@ static DECLARE_WORK(timer_update_work, timer_update_keys); #ifdef CONFIG_SMP struct timer_base timer_base_deferrable; +static atomic_t deferrable_pending; unsigned int sysctl_timer_migration = 1; DEFINE_STATIC_KEY_FALSE(timers_migration_enabled); @@ -1610,6 +1611,31 @@ static u64 cmp_next_hrtimer_event(u64 basem, u64 expires) return DIV_ROUND_UP_ULL(nextevt, TICK_NSEC) * TICK_NSEC; } + +#ifdef CONFIG_SMP +/* + * check_pending_deferrable_timers - Check for unbound deferrable timer expiry + * @cpu - Current CPU + * + * The function checks whether any global deferrable pending timers + * are exipired or not. This function does not check cpu bounded + * diferrable pending timers expiry. + * + * The function returns true when a cpu unbounded deferrable timer is expired. + */ +bool check_pending_deferrable_timers(int cpu) +{ + if (cpu == tick_do_timer_cpu || + tick_do_timer_cpu == TICK_DO_TIMER_NONE) { + if (time_after_eq(jiffies, timer_base_deferrable.clk) + && !atomic_cmpxchg(&deferrable_pending, 0, 1)) { + return true; + } + } + return false; +} +#endif + /** * get_next_timer_interrupt - return the time (clock mono) of the next timer * @basej: base time jiffies @@ -1801,7 +1827,8 @@ static __latent_entropy void run_timer_softirq(struct softirq_action *h) if (IS_ENABLED(CONFIG_NO_HZ_COMMON)) { __run_timers(this_cpu_ptr(&timer_bases[BASE_DEF])); #ifdef CONFIG_SMP - if (tick_do_timer_cpu == TICK_DO_TIMER_NONE || + if ((atomic_cmpxchg(&deferrable_pending, 1, 0) && + tick_do_timer_cpu == TICK_DO_TIMER_NONE) || tick_do_timer_cpu == smp_processor_id()) __run_timers(&timer_base_deferrable); #endif -- The Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum, a Linux Foundation Collaborative Project ^ permalink raw reply related [flat|nested] 12+ messages in thread
* Re: [PATCH v3 2/2] sched: Add a check for cpu unbound deferrable timers 2020-05-02 18:28 ` [PATCH v3 2/2] sched: Add a check for cpu unbound deferrable timers Prasad Sodagudi @ 2020-05-04 19:11 ` kbuild test robot 2020-05-06 14:03 ` Thomas Gleixner 1 sibling, 0 replies; 12+ messages in thread From: kbuild test robot @ 2020-05-04 19:11 UTC (permalink / raw) To: Prasad Sodagudi, tglx, john.stultz, sboyd, tj Cc: kbuild-all, linux-kernel, saravanak, psodagud, pkondeti [-- Attachment #1: Type: text/plain, Size: 3174 bytes --] Hi Prasad, Thank you for the patch! Yet something to improve: [auto build test ERROR on tip/timers/core] [also build test ERROR on tip/auto-latest tip/timers/nohz v5.7-rc4 next-20200501] [if your patch is applied to the wrong git tree, please drop us a note to help improve the system. BTW, we also suggest to use '--base' option to specify the base tree in git format-patch, please see https://stackoverflow.com/a/37406982] url: https://github.com/0day-ci/linux/commits/Prasad-Sodagudi/timer-make-deferrable-cpu-unbound-timers-really-not-bound-to-a-cpu/20200503-025049 base: https://git.kernel.org/pub/scm/linux/kernel/git/tip/tip.git 4479730e9263befbb9ce9563a09563db2acb8f7c config: riscv-nommu_virt_defconfig (attached as .config) compiler: riscv64-linux-gcc (GCC) 9.3.0 reproduce: wget https://raw.githubusercontent.com/intel/lkp-tests/master/sbin/make.cross -O ~/bin/make.cross chmod +x ~/bin/make.cross # save the attached .config to linux build tree COMPILER_INSTALL_PATH=$HOME/0day GCC_VERSION=9.3.0 make.cross ARCH=riscv If you fix the issue, kindly add following tag as appropriate Reported-by: kbuild test robot <lkp@intel.com> All errors (new ones prefixed by >>): kernel/time/timer.c: In function 'get_timer_cpu_base': kernel/time/timer.c:848:11: error: 'timer_base_deferrable' undeclared (first use in this function) 848 | base = &timer_base_deferrable; | ^~~~~~~~~~~~~~~~~~~~~ kernel/time/timer.c:848:11: note: each undeclared identifier is reported only once for each function it appears in kernel/time/timer.c: In function 'get_timer_this_cpu_base': kernel/time/timer.c:867:11: error: 'timer_base_deferrable' undeclared (first use in this function) 867 | base = &timer_base_deferrable; | ^~~~~~~~~~~~~~~~~~~~~ kernel/time/timer.c: In function 'run_timer_softirq': >> kernel/time/timer.c:1829:24: error: 'deferrable_pending' undeclared (first use in this function) 1829 | if ((atomic_cmpxchg(&deferrable_pending, 1, 0) && | ^~~~~~~~~~~~~~~~~~ kernel/time/timer.c:1832:18: error: 'timer_base_deferrable' undeclared (first use in this function) 1832 | __run_timers(&timer_base_deferrable); | ^~~~~~~~~~~~~~~~~~~~~ vim +/deferrable_pending +1829 kernel/time/timer.c 1817 1818 /* 1819 * This function runs timers and the timer-tq in bottom half context. 1820 */ 1821 static __latent_entropy void run_timer_softirq(struct softirq_action *h) 1822 { 1823 struct timer_base *base = this_cpu_ptr(&timer_bases[BASE_STD]); 1824 1825 __run_timers(base); 1826 if (IS_ENABLED(CONFIG_NO_HZ_COMMON)) { 1827 __run_timers(this_cpu_ptr(&timer_bases[BASE_DEF])); 1828 #ifdef CONFIG_SMP > 1829 if ((atomic_cmpxchg(&deferrable_pending, 1, 0) && 1830 tick_do_timer_cpu == TICK_DO_TIMER_NONE) || 1831 tick_do_timer_cpu == smp_processor_id()) 1832 __run_timers(&timer_base_deferrable); 1833 #endif 1834 } 1835 } 1836 --- 0-DAY CI Kernel Test Service, Intel Corporation https://lists.01.org/hyperkitty/list/kbuild-all@lists.01.org [-- Attachment #2: .config.gz --] [-- Type: application/gzip, Size: 6789 bytes --] ^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [PATCH v3 2/2] sched: Add a check for cpu unbound deferrable timers 2020-05-02 18:28 ` [PATCH v3 2/2] sched: Add a check for cpu unbound deferrable timers Prasad Sodagudi 2020-05-04 19:11 ` kbuild test robot @ 2020-05-06 14:03 ` Thomas Gleixner 1 sibling, 0 replies; 12+ messages in thread From: Thomas Gleixner @ 2020-05-06 14:03 UTC (permalink / raw) To: Prasad Sodagudi, john.stultz, sboyd, tj Cc: linux-kernel, saravanak, psodagud, pkondeti Prasad Sodagudi <psodagud@codeaurora.org> writes: > Subject: sched: Add... How is this related to the scheduler? > Add a check to find expired unbound deferrable timers > and trigger softirq for handling timers. This way a CPU > can process all the expired deferrable timers whenever > it is out off idle state due to an interrupt. A little bit more context would be useful. > Signed-off-by: Prasad Sodagudi <psodagud@codeaurora.org> > --- > include/linux/timer.h | 3 +++ > kernel/time/tick-sched.c | 8 +++++++- > kernel/time/timer.c | 29 ++++++++++++++++++++++++++++- > 3 files changed, 38 insertions(+), 2 deletions(-) > > diff --git a/include/linux/timer.h b/include/linux/timer.h > index 0dc19a8..e85dd2d 100644 > --- a/include/linux/timer.h > +++ b/include/linux/timer.h > @@ -172,6 +172,9 @@ extern int del_timer(struct timer_list * timer); > extern int mod_timer(struct timer_list *timer, unsigned long expires); > extern int mod_timer_pending(struct timer_list *timer, unsigned long expires); > extern int timer_reduce(struct timer_list *timer, unsigned long expires); > +#ifdef CONFIG_SMP This #ifdef is useless because ... > +extern bool check_pending_deferrable_timers(int cpu); > +#endif > > /* > * The jiffies value which is added to now, when there is no timer > diff --git a/kernel/time/tick-sched.c b/kernel/time/tick-sched.c > index 3e2dc9b..16aec80 100644 > --- a/kernel/time/tick-sched.c > +++ b/kernel/time/tick-sched.c > @@ -23,6 +23,7 @@ > #include <linux/module.h> > #include <linux/irq_work.h> > #include <linux/posix-timers.h> > +#include <linux/timer.h> > #include <linux/context_tracking.h> > #include <linux/mm.h> > > @@ -1274,8 +1275,13 @@ static inline void tick_nohz_irq_enter(void) > now = ktime_get(); > if (ts->idle_active) > tick_nohz_stop_idle(ts, now); > - if (ts->tick_stopped) > + if (ts->tick_stopped) { > tick_nohz_update_jiffies(now); > +#ifdef CONFIG_SMP > + if (check_pending_deferrable_timers(smp_processor_id())) > + raise_softirq_irqoff(TIMER_SOFTIRQ); > +#endif ... you failed to provide a stub function which avoids this #ifdef > + } > } > > #else > diff --git a/kernel/time/timer.c b/kernel/time/timer.c > index 1bf9b49..5947c63 100644 > --- a/kernel/time/timer.c > +++ b/kernel/time/timer.c > @@ -221,6 +221,7 @@ static DECLARE_WORK(timer_update_work, timer_update_keys); > > #ifdef CONFIG_SMP > struct timer_base timer_base_deferrable; > +static atomic_t deferrable_pending; > unsigned int sysctl_timer_migration = 1; > > DEFINE_STATIC_KEY_FALSE(timers_migration_enabled); > @@ -1610,6 +1611,31 @@ static u64 cmp_next_hrtimer_event(u64 basem, u64 expires) > return DIV_ROUND_UP_ULL(nextevt, TICK_NSEC) * TICK_NSEC; > } > > + > +#ifdef CONFIG_SMP > +/* > + * check_pending_deferrable_timers - Check for unbound deferrable timer expiry > + * @cpu - Current CPU Bogus doc format. > + * > + * The function checks whether any global deferrable pending timers > + * are exipired or not. This function does not check cpu bounded > + * diferrable pending timers expiry. Editors have spell check. > + * > + * The function returns true when a cpu unbounded deferrable timer is expired. > + */ > +bool check_pending_deferrable_timers(int cpu) > +{ > + if (cpu == tick_do_timer_cpu || > + tick_do_timer_cpu == TICK_DO_TIMER_NONE) { The second line conditional wants to be aligned with the first line conditional. > + if (time_after_eq(jiffies, timer_base_deferrable.clk) > + && !atomic_cmpxchg(&deferrable_pending, 0, 1)) { Ditto. Aside of that this deferrable pending magic lacks any form of explanation. > + return true; > + } > + } > + return false; > +} > +#endif > + > /** > * get_next_timer_interrupt - return the time (clock mono) of the next timer > * @basej: base time jiffies > @@ -1801,7 +1827,8 @@ static __latent_entropy void run_timer_softirq(struct softirq_action *h) > if (IS_ENABLED(CONFIG_NO_HZ_COMMON)) { > __run_timers(this_cpu_ptr(&timer_bases[BASE_DEF])); > #ifdef CONFIG_SMP > - if (tick_do_timer_cpu == TICK_DO_TIMER_NONE || > + if ((atomic_cmpxchg(&deferrable_pending, 1, 0) && How is that supposed to compile with NOHZ=n? For every version of these patches the 0-day robot is complaining about exactly the same problem: Your testing is solely done with one config, i.e. the config you are interested in. Is it really so hard to compile test for a total of 4 combinations of SMP and NOHZ? Thanks, tglx ^ permalink raw reply [flat|nested] 12+ messages in thread
end of thread, other threads:[~2020-05-13 21:34 UTC | newest] Thread overview: 12+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2020-05-02 18:28 [PATCH v3 0/2] timer: make deferrable cpu unbound timers really not bound to a cpu Prasad Sodagudi 2020-05-02 18:28 ` [PATCH v3 1/2] " Prasad Sodagudi 2020-05-04 18:11 ` kbuild test robot 2020-05-05 0:08 ` kbuild test robot 2020-05-06 13:28 ` Thomas Gleixner 2020-05-13 19:53 ` psodagud 2020-05-13 20:28 ` Thomas Gleixner 2020-05-13 20:55 ` psodagud 2020-05-13 21:34 ` Thomas Gleixner 2020-05-02 18:28 ` [PATCH v3 2/2] sched: Add a check for cpu unbound deferrable timers Prasad Sodagudi 2020-05-04 19:11 ` kbuild test robot 2020-05-06 14:03 ` Thomas Gleixner
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for NNTP newsgroup(s).