Re: [patch v11 05/13] task isolation: sync vmstats on return to userspace

[Marcelo Tosatti]

On Mon, Feb 07, 2022 at 03:57:18PM +0100, Frederic Weisbecker wrote:
> On Fri, Feb 04, 2022 at 02:35:42PM -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.
> > 
> > This patch adds hooks to fork and exit code paths.
> > 
> > Signed-off-by: Marcelo Tosatti <mtosatti@xxxxxxxxxx>
> > 
> > ---
> > v11: fold patch to add task_isol_exit hooks (Frederic)
> >      Use _TIF_TASK_ISOL bit on thread flags (Frederic)
> >      
> > v6: modify exit_to_user_mode_loop to cover exceptions and interrupts
> > v5: no changes
> > v4: add oneshot mode support
> > 
> >  include/linux/task_isolation.h |   16 ++++++++++++++++
> >  include/linux/vmstat.h         |    8 ++++++++
> >  kernel/entry/common.c          |   15 +++++++++++----
> >  kernel/task_isolation.c        |   21 +++++++++++++++++++++
> >  mm/vmstat.c                    |   21 +++++++++++++++++++++
> >  5 files changed, 77 insertions(+), 4 deletions(-)
> > 
> > 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
> > @@ -27,6 +27,13 @@ static inline void task_isol_free(struct
> >  		__task_isol_free(tsk);
> >  }
> >  
> > +void __task_isol_exit(struct task_struct *tsk);
> > +static inline void task_isol_exit(struct task_struct *tsk)
> > +{
> > +	if (tsk->task_isol_info)
> > +		__task_isol_exit(tsk);
> > +}
> > +
> >  int prctl_task_isol_feat_get(unsigned long arg2, unsigned long arg3,
> >  			     unsigned long arg4, unsigned long arg5);
> >  int prctl_task_isol_cfg_get(unsigned long arg2, unsigned long arg3,
> > @@ -40,12 +47,22 @@ int prctl_task_isol_activate_set(unsigne
> >  
> >  int __copy_task_isol(struct task_struct *tsk);
> >  
> > +void task_isol_exit_to_user_mode(void);
> > +
> >  #else
> >  
> > +static inline void task_isol_exit_to_user_mode(void)
> > +{
> > +}
> > +
> >  static inline void task_isol_free(struct task_struct *tsk)
> >  {
> >  }
> >  
> > +static inline void task_isol_exit(struct task_struct *tsk)
> > +{
> > +}
> > +
> >  static inline int prctl_task_isol_feat_get(unsigned long arg2,
> >  					   unsigned long arg3,
> >  					   unsigned long arg4,
> > 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
> >  
> > +#if defined(CONFIG_SMP) && defined(CONFIG_TASK_ISOLATION)
> > +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"
> >  
> > @@ -174,6 +175,9 @@ static unsigned long exit_to_user_mode_l
> >  		if (ti_work & _TIF_NOTIFY_RESUME)
> >  			tracehook_notify_resume(regs);
> >  
> > +		if (ti_work & _TIF_TASK_ISOL)
> > +			task_isol_exit_to_user_mode();
> > +
> >  		/* Architecture specific TIF work */
> >  		arch_exit_to_user_mode_work(regs, ti_work);
> >  
> > Index: linux-2.6/kernel/task_isolation.c
> > ===================================================================
> > --- linux-2.6.orig/kernel/task_isolation.c
> > +++ linux-2.6/kernel/task_isolation.c
> > @@ -18,6 +18,12 @@
> >  #include <linux/sysfs.h>
> >  #include <linux/init.h>
> >  #include <linux/sched/task.h>
> > +#include <linux/mm.h>
> > +#include <linux/vmstat.h>
> > +
> > +void __task_isol_exit(struct task_struct *tsk)
> > +{
> > +}
> >  
> >  void __task_isol_free(struct task_struct *tsk)
> >  {
> > @@ -251,6 +257,9 @@ static int cfg_feat_quiesce_set(unsigned
> >  	task_isol_info->quiesce_mask = i_qctrl->quiesce_mask;
> >  	task_isol_info->oneshot_mask = i_qctrl->quiesce_oneshot_mask;
> >  	task_isol_info->conf_mask |= ISOL_F_QUIESCE;
> > +	if (task_isol_info->quiesce_mask & ISOL_F_QUIESCE_VMSTATS)
> > +		set_thread_flag(TIF_TASK_ISOL);
> 
> Should you check if (i->active_mask == ISOL_F_QUIESCE) before setting the
> flag?
> 
> > +
> >  	ret = 0;
> >  
> >  out_free:
> > @@ -303,6 +312,7 @@ int __copy_task_isol(struct task_struct
> >  		new_info->active_mask = info->active_mask;
> >  
> >  	tsk->task_isol_info = new_info;
> > +	set_ti_thread_flag(task_thread_info(tsk), TIF_TASK_ISOL);
> 
> Same here?

Yes, should fix that.

Will wait for more comments before resending -v12.