| Message ID | 20230105125218.031928326@redhat.com (mailing list archive) |
|---|---|
| Headers | show |
| Series | Ensure quiet_vmstat() is called when returning to userpace and when idle tick is stopped | expand |
On Fri, Jan 06, 2023 at 08:12:44AM +0800, Hillf Danton wrote: > On 05 Jan 2023 09:52:21 -0300 Marcelo Tosatti <mtosatti@redhat.com> > > For nohz full CPUs, we'd like the per-CPU vm statistics to be > > synchronized when userspace is executing. Otherwise, > > the vmstat_shepherd might queue a work item to synchronize them, > > which is undesired intereference for isolated CPUs. > > > > This means that its necessary to check for, and possibly sync, > > the statistics when returning to userspace. This means that > > there are now two execution contexes, on different CPUs, > > which require awareness about each other: context switch > > and vmstat shepherd kernel threadr. > > > > To avoid the shared variables between these two contexes (which > > would require atomic accesses), delegate the responsability > > of statistics synchronization from vmstat_shepherd to local CPU > > context, for nohz_full CPUs. > > > > Do that by queueing a delayed work when marking per-CPU vmstat dirty. > > > > When returning to userspace, fold the stats and cancel the delayed work. > > > > When entering idle, only fold the stats. > > > > Signed-off-by: Marcelo Tosatti <mtosatti@redhat.com> > > --- > > include/linux/vmstat.h | 4 ++-- > > kernel/time/tick-sched.c | 2 +- > > mm/vmstat.c | 41 ++++++++++++++++++++++++++++++++--------- > > 3 files changed, 35 insertions(+), 12 deletions(-) > > > > Index: linux-2.6/mm/vmstat.c > > =================================================================== > > --- linux-2.6.orig/mm/vmstat.c > > +++ linux-2.6/mm/vmstat.c > > @@ -28,6 +28,7 @@ > > #include <linux/mm_inline.h> > > #include <linux/page_ext.h> > > #include <linux/page_owner.h> > > +#include <linux/tick.h> > > > > #include "internal.h" > > > > @@ -194,21 +195,57 @@ void fold_vm_numa_events(void) > > #endif > > > > #ifdef CONFIG_SMP > > -static DEFINE_PER_CPU_ALIGNED(bool, vmstat_dirty); > > + > > +struct vmstat_dirty { > > + bool dirty; > > +#ifdef CONFIG_FLUSH_WORK_ON_RESUME_USER > > + bool cpu_offline; > > +#endif > > +}; > > + > > +static DEFINE_PER_CPU_ALIGNED(struct vmstat_dirty, vmstat_dirty_pcpu); > > +static DEFINE_PER_CPU(struct delayed_work, vmstat_work); > > +int sysctl_stat_interval __read_mostly = HZ; > > + > > +#ifdef CONFIG_FLUSH_WORK_ON_RESUME_USER > > +static inline void vmstat_queue_local_work(void) > > +{ > > + bool vmstat_dirty = this_cpu_read(vmstat_dirty_pcpu.dirty); > > + bool cpu_offline = this_cpu_read(vmstat_dirty_pcpu.cpu_offline); > > + int cpu = smp_processor_id(); > > + > > + if (tick_nohz_full_cpu(cpu) && !vmstat_dirty) { > > + struct delayed_work *dw; > > + > > + dw = this_cpu_ptr(&vmstat_work); > > + if (!delayed_work_pending(dw) && !cpu_offline) { > > + unsigned long delay; > > + > > + delay = round_jiffies_relative(sysctl_stat_interval); > > + queue_delayed_work_on(cpu, mm_percpu_wq, dw, delay); > > Regression wrt V12 if timer is added on the CPU that is not doing HK_TYPE_TIMER? Before this change, the timer was managed (and queued on an isolated CPU) by vmstat_shepherd. Now it is managed (and queued) by the local CPU, so there is no regression. Thanks.
On 6 Jan 2023 09:51:00 -0300 Marcelo Tosatti <mtosatti@redhat.com> > On Fri, Jan 06, 2023 at 08:12:44AM +0800, Hillf Danton wrote: > > > > Regression wrt V12 if timer is added on the CPU that is not doing HK_TYPE_TIMER? > > Before this change, the timer was managed (and queued on an isolated > CPU) by vmstat_shepherd. Now it is managed (and queued) by the local > CPU, so there is no regression. Given vm stats folded when returning to userspace, queuing the delayed work barely makes sense in the first place. If it can be canceled, queuing it burns cycles with nothing earned. Otherwise vm stats got folded already. Nor does shepherd even without delay. And the right thing is only make shepherd leave isolated CPUs intact.
On Fri, Jan 06, 2023 at 11:01:54PM +0800, Hillf Danton wrote: > On 6 Jan 2023 09:51:00 -0300 Marcelo Tosatti <mtosatti@redhat.com> > > On Fri, Jan 06, 2023 at 08:12:44AM +0800, Hillf Danton wrote: > > > > > > Regression wrt V12 if timer is added on the CPU that is not doing HK_TYPE_TIMER? > > > > Before this change, the timer was managed (and queued on an isolated > > CPU) by vmstat_shepherd. Now it is managed (and queued) by the local > > CPU, so there is no regression. > > Given vm stats folded when returning to userspace, queuing the delayed work > barely makes sense in the first place. If it can be canceled, queuing it burns > cycles with nothing earned. Otherwise vm stats got folded already. Agree, but you can't know whether return to userspace will occur before the timer is fired. So queueing the timer is to _ensure_ that eventually vmstats will be synced (which maintains the current timing behaviour wrt vmstat syncs). Also don't think the queueing cost is significant: it only happens for the first vmstat dirty item. > Nor does shepherd even without delay. And the right thing is only make shepherd > leave isolated CPUs intact. > >
On 6 Jan 2023 15:16:23 -0300 Marcelo Tosatti <mtosatti@redhat.com> > On Fri, Jan 06, 2023 at 11:01:54PM +0800, Hillf Danton wrote: > > On 6 Jan 2023 09:51:00 -0300 Marcelo Tosatti <mtosatti@redhat.com> > > > On Fri, Jan 06, 2023 at 08:12:44AM +0800, Hillf Danton wrote: > > > > > > > > Regression wrt V12 if timer is added on the CPU that is not doing HK_TYPE_TIMER? > > > > > > Before this change, the timer was managed (and queued on an isolated > > > CPU) by vmstat_shepherd. Now it is managed (and queued) by the local > > > CPU, so there is no regression. > > > > Given vm stats folded when returning to userspace, queuing the delayed work > > barely makes sense in the first place. If it can be canceled, queuing it burns > > cycles with nothing earned. Otherwise vm stats got folded already. > > Agree, but you can't know whether return to userspace will occur > before the timer is fired. No way to predict a random timer expiration, no? > > So queueing the timer is to _ensure_ that eventually vmstats will be > synced (which maintains the current timing behaviour wrt vmstat syncs). After this change, > > > > > @@ -1988,13 +2022,19 @@ void quiet_vmstat(void) > > > > > if (!is_vmstat_dirty()) > > > > > return; > > > > > it is only ensured eventually by this check instead. > > > > > + refresh_cpu_vm_stats(false); > > > > > + > > > > > + if (!IS_ENABLED(CONFIG_FLUSH_WORK_ON_RESUME_USER)) > > > > > + return; > > > > > + > > > > > + if (!user) > > > > > + return; > Also don't think the queueing cost is significant: it only happens > for the first vmstat dirty item. Cost is considered only if it is needed. > > > Nor does shepherd even without delay. And the right thing is only make shepherd > > leave isolated CPUs intact.
Hi Hillf, On Sat, Jan 07, 2023 at 08:15:29AM +0800, Hillf Danton wrote: > On 6 Jan 2023 15:16:23 -0300 Marcelo Tosatti <mtosatti@redhat.com> > > On Fri, Jan 06, 2023 at 11:01:54PM +0800, Hillf Danton wrote: > > > On 6 Jan 2023 09:51:00 -0300 Marcelo Tosatti <mtosatti@redhat.com> > > > > On Fri, Jan 06, 2023 at 08:12:44AM +0800, Hillf Danton wrote: > > > > > > > > > > Regression wrt V12 if timer is added on the CPU that is not doing HK_TYPE_TIMER? > > > > > > > > Before this change, the timer was managed (and queued on an isolated > > > > CPU) by vmstat_shepherd. Now it is managed (and queued) by the local > > > > CPU, so there is no regression. > > > > > > Given vm stats folded when returning to userspace, queuing the delayed work > > > barely makes sense in the first place. If it can be canceled, queuing it burns > > > cycles with nothing earned. Otherwise vm stats got folded already. > > > > Agree, but you can't know whether return to userspace will occur > > before the timer is fired. > > No way to predict a random timer expiration, no? Right. > > > > So queueing the timer is to _ensure_ that eventually vmstats will be > > synced (which maintains the current timing behaviour wrt vmstat syncs). > > After this change, > > > > > > > @@ -1988,13 +2022,19 @@ void quiet_vmstat(void) > > > > > > if (!is_vmstat_dirty()) > > > > > > return; > > > > > > > > it is only ensured eventually by this check instead. Yes, but if you do not return to userspace, then the per-CPU vm statistics can be dirty indefinitely. > > > > > > + refresh_cpu_vm_stats(false); > > > > > > + > > > > > > + if (!IS_ENABLED(CONFIG_FLUSH_WORK_ON_RESUME_USER)) > > > > > > + return; > > > > > > + > > > > > > + if (!user) > > > > > > + return; > > > > Also don't think the queueing cost is significant: it only happens > > for the first vmstat dirty item. > > Cost is considered only if it is needed. Not sure i understand what you mean (or whether there is any alternative to the timer).
On 9 Jan 2023 11:12:49 -0300 Marcelo Tosatti <mtosatti@redhat.com> > > Yes, but if you do not return to userspace, then the per-CPU vm > statistics can be dirty indefinitely. Could you specify the reasons for failing to return to userspace, given it is undesired intereference for the shepherd to queue work on the isolated CPUs.
On Tue, Jan 10, 2023 at 10:43:56AM +0800, Hillf Danton wrote: > On 9 Jan 2023 11:12:49 -0300 Marcelo Tosatti <mtosatti@redhat.com> > > > > Yes, but if you do not return to userspace, then the per-CPU vm > > statistics can be dirty indefinitely. > > Could you specify the reasons for failing to return to userspace, > given it is undesired intereference for the shepherd to queue work > on the isolated CPUs. Any system call that takes longer than the threshold to sync vmstats. Or a long running kernel thread, for example: https://stackoverflow.com/questions/65111483/long-running-kthread-and-synchronize-net
On Tue, 10 Jan 2023 08:50:28 -0300 Marcelo Tosatti <mtosatti@redhat.com> > On Tue, Jan 10, 2023 at 10:43:56AM +0800, Hillf Danton wrote: > > On 9 Jan 2023 11:12:49 -0300 Marcelo Tosatti <mtosatti@redhat.com> > > > > > > Yes, but if you do not return to userspace, then the per-CPU vm > > > statistics can be dirty indefinitely. > > > > Could you specify the reasons for failing to return to userspace, > > given it is undesired intereference for the shepherd to queue work > > on the isolated CPUs. > > Any system call that takes longer than the threshold to sync vmstats. Which ones? If schedule() occurs during syscall because of acquiring mutex for instance then anything on the isolated runqueue, including workqueue worker shepherd wakes up, can burn CPU cycles without undesired intereference produced. > > Or a long running kernel thread, for example: It is a buggyyyy example. > > https://stackoverflow.com/questions/65111483/long-running-kthread-and-synchronize-net
On Tue, Jan 10, 2023 at 11:19:01PM +0800, Hillf Danton wrote: > On Tue, 10 Jan 2023 08:50:28 -0300 Marcelo Tosatti <mtosatti@redhat.com> > > On Tue, Jan 10, 2023 at 10:43:56AM +0800, Hillf Danton wrote: > > > On 9 Jan 2023 11:12:49 -0300 Marcelo Tosatti <mtosatti@redhat.com> > > > > > > > > Yes, but if you do not return to userspace, then the per-CPU vm > > > > statistics can be dirty indefinitely. > > > > > > Could you specify the reasons for failing to return to userspace, > > > given it is undesired intereference for the shepherd to queue work > > > on the isolated CPUs. > > > > Any system call that takes longer than the threshold to sync vmstats. > > Which ones? > > If schedule() occurs during syscall because of acquiring mutex for instance > then anything on the isolated runqueue, including workqueue worker shepherd > wakes up, can burn CPU cycles without undesired intereference produced. The above confuses me. How others tasks would help with syscalls that take too long too service? > > > > Or a long running kernel thread, for example: > > It is a buggyyyy example. > > > > https://stackoverflow.com/questions/65111483/long-running-kthread-and-synchronize-net I can imagine a CPU spending most of its time processing networking packets through interrupts/softirq within ksoftirqd/NAPI while another CPU process these packets in userspace. In this case the CPU handling the kernel part can theoretically never go to idle/user. nohz_full isn't optimized toward such job but there is nothing to prevent it from doing such job. Thanks.
On 10 Jan 2023 17:12:22 +0100 Frederic Weisbecker <frederic@kernel.org> > On Tue, Jan 10, 2023 at 11:19:01PM +0800, Hillf Danton wrote: > > On Tue, 10 Jan 2023 08:50:28 -0300 Marcelo Tosatti <mtosatti@redhat.com> > > > On Tue, Jan 10, 2023 at 10:43:56AM +0800, Hillf Danton wrote: > > > > On 9 Jan 2023 11:12:49 -0300 Marcelo Tosatti <mtosatti@redhat.com> > > > > > > > > > > Yes, but if you do not return to userspace, then the per-CPU vm > > > > > statistics can be dirty indefinitely. > > > > > > > > Could you specify the reasons for failing to return to userspace, > > > > given it is undesired intereference for the shepherd to queue work > > > > on the isolated CPUs. > > > > > > Any system call that takes longer than the threshold to sync vmstats. > > > > Which ones? > > > > If schedule() occurs during syscall because of acquiring mutex for instance > > then anything on the isolated runqueue, including workqueue worker shepherd > > wakes up, can burn CPU cycles without undesired interference produced. > > The above confuses me. How others tasks would help with syscalls that take too long too > service? Given no scheduling in userspace, no chance for other tasks to interfere after returning to userspace, on one hand. Upon scheduling during syscall on the other hand, it is the right time to sync vmstats for example. But no vmstats can be updated without works queued by shepherd. In a nutshell, no interference could happen without scheduling, and how work is queued does not matter. So the current shepherd behavior is prefered. > > > > > > > Or a long running kernel thread, for example: > > > > It is a buggyyyy example. > > > > > > https://stackoverflow.com/questions/65111483/long-running-kthread-and-synchronize-net > > I can imagine a CPU spending most of its time processing networking packets > through interrupts/softirq within ksoftirqd/NAPI while another CPU process > these packets in userspace. > > In this case the CPU handling the kernel part can theoretically never go to > idle/user. nohz_full isn't optimized toward such job but there is nothing > to prevent it from doing such job. A simple FIFO task launched by an administrator can get a CPU out of scheduler's control for a week, regardless of isolation.
On Wed, Jan 11, 2023 at 07:58:22AM +0800, Hillf Danton wrote: > On 10 Jan 2023 17:12:22 +0100 Frederic Weisbecker <frederic@kernel.org> > > On Tue, Jan 10, 2023 at 11:19:01PM +0800, Hillf Danton wrote: > > > On Tue, 10 Jan 2023 08:50:28 -0300 Marcelo Tosatti <mtosatti@redhat.com> > > > > On Tue, Jan 10, 2023 at 10:43:56AM +0800, Hillf Danton wrote: > > > > > On 9 Jan 2023 11:12:49 -0300 Marcelo Tosatti <mtosatti@redhat.com> > > > > > > > > > > > > Yes, but if you do not return to userspace, then the per-CPU vm > > > > > > statistics can be dirty indefinitely. > > > > > > > > > > Could you specify the reasons for failing to return to userspace, > > > > > given it is undesired intereference for the shepherd to queue work > > > > > on the isolated CPUs. > > > > > > > > Any system call that takes longer than the threshold to sync vmstats. > > > > > > Which ones? > > > > > > If schedule() occurs during syscall because of acquiring mutex for instance > > > then anything on the isolated runqueue, including workqueue worker shepherd > > > wakes up, can burn CPU cycles without undesired interference produced. > > > > The above confuses me. How others tasks would help with syscalls that take too long too > > service? > > Given no scheduling in userspace, no chance for other tasks to interfere > after returning to userspace, on one hand. > > Upon scheduling during syscall on the other hand, it is the right time > to sync vmstats for example. But no vmstats can be updated without works > queued by shepherd. > > In a nutshell, no interference could happen without scheduling, and how > work is queued does not matter. So the current shepherd behavior is prefered. I'm still confused... We want to avoid the shepherd because it may queue the vmstat work while the task wants to run noise-free in userspace. > > > > > > > > > > Or a long running kernel thread, for example: > > > > > > It is a buggyyyy example. > > > > > > > > https://stackoverflow.com/questions/65111483/long-running-kthread-and-synchronize-net > > > > I can imagine a CPU spending most of its time processing networking packets > > through interrupts/softirq within ksoftirqd/NAPI while another CPU process > > these packets in userspace. > > > > In this case the CPU handling the kernel part can theoretically never go to > > idle/user. nohz_full isn't optimized toward such job but there is nothing > > to prevent it from doing such job. > > A simple FIFO task launched by an administrator can get a CPU out of scheduler's > control for a week, regardless of isolation. Sure. But, what do you mean by that exactly? Thanks.
This patch series addresses the following two problems: 1. A customer provided some evidence which indicates that the idle tick was stopped; albeit, CPU-specific vmstat counters still remained populated. Thus one can only assume quiet_vmstat() was not invoked on return to the idle loop. If I understand correctly, I suspect this divergence might erroneously prevent a reclaim attempt by kswapd. If the number of zone specific free pages are below their per-cpu drift value then zone_page_state_snapshot() is used to compute a more accurate view of the aforementioned statistic. Thus any task blocked on the NUMA node specific pfmemalloc_wait queue will be unable to make significant progress via direct reclaim unless it is killed after being woken up by kswapd (see throttle_direct_reclaim()) 2. With a SCHED_FIFO task that busy loops on a given CPU, and kworker for that CPU at SCHED_OTHER priority, queuing work to sync per-vmstats will either cause that work to never execute, or stalld (i.e. stall daemon) boosts kworker priority which causes a latency violation As seen previously, the trivial test program (i.e. attached at the end of this cover letter) executed inside a KVM VM, was used to determine the somewhat impact under vanilla and with the proposed changes. Firstly, the mlock(2) and munlock(2) system calls was used solely to modify vmstat item 'NR_MLOCK'. In another scenario, the nanosleep(2) system call was used several times to suspend execution for a period of time to approximately compute the number of CPU-cycles in the idle code path. The following is an average count of CPU-cycles across the aforementioned system calls and the idle loop, respectively. I believe these results are negligible: Vanilla Modified Cycles per idle loop 151858 153258 (+1.0%) Cycles per syscall 8461 8690 (+2.6%) Any feedback would be appreciated. Thanks. Changes since v12 [1]: - Protect vmstat cmpxchg and vmstat dirty bit write by disabling preemption (Frederic Weisbecker) Changes since v11 [2]: - Switch back to this_cpu_write/read when appropriate (Frederic Weisbecker) - Avoid ifdeffery in the middle of functions (Frederic Weisbecker) - Clarify down_prep callback comment (Frederic Weisbecker) - Move new Kconfig option close to CPU_ISOLATION option (Frederic Weisbecker) Changes since v10 [3]: - Close cpu hotplug race with nohz_full CPUs (Frederic Weisbecker) Changes since v9 [4]: - Add config to enable/disable syncing when returning to userspace (Frederic Weisbecker) - Add missing signed-off-by (Frederic Weisbecker) - Use proper CPU value when skipping nohz_full CPUs (Frederic Weisbecker) - Use this_cpu_ptr when appropriate (Frederic Weisbecker) - Improve changelogs (Frederic Weisbecker) - For stat_refresh sysfs file: avoid queueing work on CPU if stats are clean Changes since v8 [5]: - For nohz_full CPUs, manage per-CPU vmstat flushing from CPU context (Frederic Weisbecker) Changes since v7 [6]: - Added trivial helpers for modification and testing (Andrew Morton) - Modified comment since we do now cancel any delayed work if the tick is stopped in quiet_vmstat() - Moved check to ensure vmstat differentials do not remain if the tick is stopped on exiting to user-mode into a separate patch (Frederic Weisbecker) Changes since v6 [7]: - Clean vmstat_dirty before differential sync loop - Cancel pending work if tick stopped - Do not queue work to remote CPU if tick stopped Changes since v5 [8]: - Introduced __tick_nohz_user_enter_prepare() - Switched to EXPORT_SYMBOL_GPL() Changes since v4 [9]: - Moved vmstat_dirty specific changes into a separate patch (Marcelo Tosatti) Changes since v3 [10]: - Used EXPORT_SYMBOL() on tick_nohz_user_enter_prepare() - Replaced need_update() - Introduced CPU-specific variable namely vmstat_dirty and mark_vmstat_dirty() [1]: https://lore.kernel.org/linux-mm/20230104133459.5yaflf3yicpmhbbh@ava.usersys.com/T/ [2]: https://lore.kernel.org/lkml/20221223144150.GA79369@lothringen/T/ [3]: https://lore.kernel.org/linux-mm/20221216194904.075275493@redhat.com/T/ [4]: https://lore.kernel.org/lkml/20221214131839.GE1930067@lothringen/t/ [5]: https://lore.kernel.org/linux-mm/20220924152227.819815-1-atomlin@redhat.com/ [6]: https://lore.kernel.org/lkml/20220817191346.287594886@redhat.com/ [7]: https://lore.kernel.org/linux-mm/20220808194820.676246-1-atomlin@redhat.com/ [8]: https://lore.kernel.org/lkml/20220801234258.134609-1-atomlin@redhat.com/ [9]: https://lore.kernel.org/lkml/20220621172207.1501641-1-atomlin@redhat.com/ [10]: https://lore.kernel.org/lkml/20220422193647.3808657-1-atomlin@redhat.com/ Aaron Tomlin (4): mm/vmstat: Add CPU-specific variable to track a vmstat discrepancy mm/vmstat: Use vmstat_dirty to track CPU-specific vmstat discrepancies tick/nohz_full: Ensure quiet_vmstat() is called on exit to user-mode when the idle tick is stopped tick/sched: Ensure quiet_vmstat() is called when the idle tick was stopped too Marcelo Tosatti (2): mm/vmstat: Do not queue vmstat_update if tick is stopped mm/vmstat: avoid queueing work item if cpu stats are clean include/linux/tick.h | 5 +- include/linux/vmstat.h | 4 - init/Kconfig | 13 +++++ kernel/time/tick-sched.c | 20 ++++++++- mm/vmstat.c | 223 ++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++--------------------- 5 files changed, 214 insertions(+), 51 deletions(-)