The logic to disable vmstat worker thread, when entering
nohz full, does not cover all scenarios. For example, it is possible
for the following to happen:
1) enter nohz_full, which calls refresh_cpu_vm_stats, syncing the stats.
2) app runs mlock, which increases counters for mlock'ed pages.
3) start -RT loop
Since refresh_cpu_vm_stats from nohz_full logic can happen _before_
the mlock, vmstat shepherd can restart vmstat worker thread on
the CPU in question.
To fix this, use the task isolation prctl interface to quiesce
deferred actions when returning to userspace.
Signed-off-by: Marcelo Tosatti <[email protected]>
Index: linux-2.6/include/linux/task_isolation.h
===================================================================
--- linux-2.6.orig/include/linux/task_isolation.h
+++ linux-2.6/include/linux/task_isolation.h
@@ -32,8 +32,20 @@ int prctl_task_isolation_ctrl_get(unsign
int prctl_task_isolation_ctrl_set(unsigned long arg2, unsigned long arg3,
unsigned long arg4, unsigned long arg5);
+void __isolation_exit_to_user_mode_prepare(void);
+
+static inline void isolation_exit_to_user_mode_prepare(void)
+{
+ if (current->isol_info != NULL)
+ __isolation_exit_to_user_mode_prepare();
+}
+
#else
+static void isolation_exit_to_user_mode_prepare(void)
+{
+}
+
static inline void tsk_isol_exit(struct task_struct *tsk)
{
}
Index: linux-2.6/include/linux/vmstat.h
===================================================================
--- linux-2.6.orig/include/linux/vmstat.h
+++ linux-2.6/include/linux/vmstat.h
@@ -21,6 +21,14 @@ int sysctl_vm_numa_stat_handler(struct c
void *buffer, size_t *length, loff_t *ppos);
#endif
+#ifdef CONFIG_SMP
+void sync_vmstat(void);
+#else
+static inline void sync_vmstat(void)
+{
+}
+#endif
+
struct reclaim_stat {
unsigned nr_dirty;
unsigned nr_unqueued_dirty;
Index: linux-2.6/kernel/entry/common.c
===================================================================
--- linux-2.6.orig/kernel/entry/common.c
+++ linux-2.6/kernel/entry/common.c
@@ -6,6 +6,7 @@
#include <linux/livepatch.h>
#include <linux/audit.h>
#include <linux/tick.h>
+#include <linux/task_isolation.h>
#include "common.h"
@@ -287,6 +288,7 @@ static void syscall_exit_to_user_mode_pr
static __always_inline void __syscall_exit_to_user_mode_work(struct pt_regs *regs)
{
syscall_exit_to_user_mode_prepare(regs);
+ isolation_exit_to_user_mode_prepare();
local_irq_disable_exit_to_user();
exit_to_user_mode_prepare(regs);
}
Index: linux-2.6/kernel/task_isolation.c
===================================================================
--- linux-2.6.orig/kernel/task_isolation.c
+++ linux-2.6/kernel/task_isolation.c
@@ -17,6 +17,8 @@
#include <linux/string.h>
#include <linux/sysfs.h>
#include <linux/init.h>
+#include <linux/mm.h>
+#include <linux/vmstat.h>
static unsigned long default_quiesce_mask;
@@ -145,6 +147,17 @@ int prctl_task_isolation_ctrl_get(unsign
return current->isol_info->active_mask;
}
+void __isolation_exit_to_user_mode_prepare(void)
+{
+ struct isol_info *i = current->isol_info;
+
+ if (i->active_mask != ISOL_F_QUIESCE)
+ return;
+
+ if (i->quiesce_mask & ISOL_F_QUIESCE_VMSTATS)
+ sync_vmstat();
+}
+
struct qoptions {
unsigned long mask;
char *name;
Index: linux-2.6/mm/vmstat.c
===================================================================
--- linux-2.6.orig/mm/vmstat.c
+++ linux-2.6/mm/vmstat.c
@@ -1964,6 +1964,27 @@ static void vmstat_shepherd(struct work_
round_jiffies_relative(sysctl_stat_interval));
}
+void sync_vmstat(void)
+{
+ int cpu;
+
+ cpu = get_cpu();
+
+ refresh_cpu_vm_stats(false);
+ put_cpu();
+
+ /*
+ * If task is migrated to another CPU between put_cpu
+ * and cancel_delayed_work_sync, the code below might
+ * cancel vmstat_update work for a different cpu
+ * (than the one from which the vmstats were flushed).
+ *
+ * However, vmstat shepherd will re-enable it later,
+ * so its harmless.
+ */
+ cancel_delayed_work_sync(&per_cpu(vmstat_work, cpu));
+}
+
static void __init start_shepherd_timer(void)
{
int cpu;
On Fri, 2021-07-30 at 17:18 -0300, Marcelo Tosatti wrote:
> The logic to disable vmstat worker thread, when entering
> nohz full, does not cover all scenarios. For example, it is possible
> for the following to happen:
>
> 1) enter nohz_full, which calls refresh_cpu_vm_stats, syncing the stats.
> 2) app runs mlock, which increases counters for mlock'ed pages.
> 3) start -RT loop
>
> Since refresh_cpu_vm_stats from nohz_full logic can happen _before_
> the mlock, vmstat shepherd can restart vmstat worker thread on
> the CPU in question.
>
> To fix this, use the task isolation prctl interface to quiesce
> deferred actions when returning to userspace.
Even though this is mostly targeted at nohz_full users, I believe I haven't
seen anything in this series that forces the feature to be run on nohz_full
CPUs (this is a good thing IMO). So, I'd suggest to reword the patch
description so it doesn't imply nohz_full is necessary to use this.
--
Nicolás Sáenz
On Tue, Aug 03, 2021 at 05:13:03PM +0200, [email protected] wrote:
> On Fri, 2021-07-30 at 17:18 -0300, Marcelo Tosatti wrote:
> > The logic to disable vmstat worker thread, when entering
> > nohz full, does not cover all scenarios. For example, it is possible
> > for the following to happen:
> >
> > 1) enter nohz_full, which calls refresh_cpu_vm_stats, syncing the stats.
> > 2) app runs mlock, which increases counters for mlock'ed pages.
> > 3) start -RT loop
> >
> > Since refresh_cpu_vm_stats from nohz_full logic can happen _before_
> > the mlock, vmstat shepherd can restart vmstat worker thread on
> > the CPU in question.
> >
> > To fix this, use the task isolation prctl interface to quiesce
> > deferred actions when returning to userspace.
>
> Even though this is mostly targeted at nohz_full users, I believe I haven't
> seen anything in this series that forces the feature to be run on nohz_full
> CPUs (this is a good thing IMO).
I don't think there is such a dependency either.
> So, I'd suggest to reword the patch
> description so it doesn't imply nohz_full is necessary to use this.
Its describing a fact from nohz_full where it can't guarantee entering
userspace with vmstat turned off (which is a reply to Christopher's
earlier comment that "this should just work with nohz_full and
logic to shutdown the vmstat delayed work timer").
Will add a comment to make it explicit that the series does not depend
on nohz_full.
Thanks.