Re: [patch v8 03/10] task isolation: sync vmstats on return to userspace

[Marcelo Tosatti <mtosatti@redhat.com>]

On Fri, Jan 21, 2022 at 01:06:10PM +0100, Frederic Weisbecker wrote:
> On Wed, Dec 08, 2021 at 01:09:09PM -0300, Marcelo Tosatti wrote:
> > 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
> > @@ -40,8 +40,19 @@ int prctl_task_isolation_activate_set(un
> >  
> >  int __copy_task_isolation(struct task_struct *tsk);
> >  
> > +void isolation_exit_to_user_mode(void);
> > +
> > +static inline int task_isol_has_work(void)
> > +{
> > +	return 0;
> > +}
> > +
> >  #else
> >  
> > +static void isolation_exit_to_user_mode(void)
> > +{
> > +}
> > +
> >  static inline void tsk_isol_free(struct task_struct *tsk)
> >  {
> >  }
> > @@ -86,6 +97,11 @@ static inline int prctl_task_isolation_a
> >  	return -EOPNOTSUPP;
> >  }
> >  
> > +static inline int task_isol_has_work(void)
> > +{
> > +	return 0;
> > +}
> > +
> 
> It would be nice to have a coherent greppable task_isol_*() namespace instead
> of random scattered tsk_*(), isolation_*() stuff...
> 
> task_isol_exit_to_user_mode()
> task_isol_free()
> task_isol_copy_process()
> task_isol_had_work()
> ...
> 
> > @@ -149,13 +150,14 @@ static void handle_signal_work(struct pt
> >  }
> >  
> >  static unsigned long exit_to_user_mode_loop(struct pt_regs *regs,
> > -					    unsigned long ti_work)
> > +					    unsigned long ti_work,
> > +					    unsigned long tsk_isol_work)
> >  {
> >  	/*
> >  	 * Before returning to user space ensure that all pending work
> >  	 * items have been completed.
> >  	 */
> > -	while (ti_work & EXIT_TO_USER_MODE_WORK) {
> > +	while ((ti_work & EXIT_TO_USER_MODE_WORK) || tsk_isol_work) {
> 
> So there is a dependency on CONFIG_GENERIC_ENTRY. Then you need to split that
> from CONFIG_CPU_ISOLATION:
> 
> config TASK_ISOLATION
>        bool "Task isolation prctl()"
>        depends on GENERIC_ENTRY
>        help "...."
> 
> >  
> >  		local_irq_enable_exit_to_user(ti_work);
> >  
> > @@ -177,6 +179,9 @@ static unsigned long exit_to_user_mode_l
> >  		/* Architecture specific TIF work */
> >  		arch_exit_to_user_mode_work(regs, ti_work);
> >  
> > +		if (tsk_isol_work)
> > +			isolation_exit_to_user_mode();
> > +
> >  		/*
> >  		 * Disable interrupts and reevaluate the work flags as they
> >  		 * might have changed while interrupts and preemption was
> > @@ -188,6 +193,7 @@ static unsigned long exit_to_user_mode_l
> >  		tick_nohz_user_enter_prepare();
> >  
> >  		ti_work = READ_ONCE(current_thread_info()->flags);
> > +		tsk_isol_work = task_isol_has_work();
> 
> Shouldn't it be a TIF_FLAG part of EXIT_TO_USER_MODE_WORK instead?
> 
> Thanks.

static inline int task_isol_has_work(void)
{
       int cpu, ret;
       struct isol_info *i;

       if (likely(current->task_isol_info == NULL))
               return 0;

       i = current->task_isol_info;
       if (i->active_mask != ISOL_F_QUIESCE)
               return 0;

       if (!(i->quiesce_mask & ISOL_F_QUIESCE_VMSTATS))
               return 0;

       cpu = get_cpu();
       ret = per_cpu(vmstat_dirty, cpu);
       put_cpu();

       return ret;
}

Well, whether its necessary to call task_isol_exit_to_user_mode depends
on the state of the enabled/disabled masks _and_ vmstat dirty bit
information.

It seems awkward, to me, to condense all that information in a single bit.

Addressed all other comments, thanks.