Re: [patch v12 07/13] task isolation: sync vmstats conditional on changes

[Frederic Weisbecker <frederic@kernel.org>]

On Tue, Mar 15, 2022 at 12:31:39PM -0300, Marcelo Tosatti wrote:
> Rather than syncing VM-stats on every return to userspace
> (or VM-entry), keep track of changes through a per-CPU bool.
> 
> This improves performance when enabling task isolated
> for vcpu VMs.
> 
> Signed-off-by: Marcelo Tosatti <mtosatti@redhat.com>
> 
> ---
> v11:
> - Add TIF_TASK_ISOL bit to thread info flags and use it
>   to decide whether to perform task isolation work on
>   return to userspace     
> 
>  include/linux/vmstat.h |   13 ++++++++++++-
>  mm/vmstat.c            |   29 ++++++++++++++++++++++++++++-
>  2 files changed, 40 insertions(+), 2 deletions(-)
> 
> Index: linux-2.6/include/linux/vmstat.h
> ===================================================================
> --- linux-2.6.orig/include/linux/vmstat.h
> +++ linux-2.6/include/linux/vmstat.h
> @@ -22,7 +22,18 @@ int sysctl_vm_numa_stat_handler(struct c
>  #endif
>  
>  #if defined(CONFIG_SMP) && defined(CONFIG_TASK_ISOLATION)
> -void sync_vmstat(void);
> +DECLARE_PER_CPU_ALIGNED(bool, vmstat_dirty);
> +
> +extern struct static_key vmstat_sync_enabled;
> +
> +void __sync_vmstat(void);
> +static inline void sync_vmstat(void)
> +{
> +	if (static_key_false(&vmstat_sync_enabled))
> +		__sync_vmstat();
> +}
> +
> +void init_sync_vmstat(void);
>  #else
>  static inline void sync_vmstat(void)
>  {
> Index: linux-2.6/mm/vmstat.c
> ===================================================================
> --- linux-2.6.orig/mm/vmstat.c
> +++ linux-2.6/mm/vmstat.c
> @@ -334,6 +334,31 @@ void set_pgdat_percpu_threshold(pg_data_
>  	}
>  }
>  
> +#ifdef CONFIG_TASK_ISOLATION
> +struct static_key vmstat_sync_enabled;
> +DEFINE_PER_CPU_ALIGNED(bool, vmstat_dirty);
> +
> +static inline void mark_vmstat_dirty(void)
> +{
> +	if (!static_key_false(&vmstat_sync_enabled))
> +		return;
> +
> +	raw_cpu_write(vmstat_dirty, true);
> +	set_thread_flag(TIF_TASK_ISOL);
> +}
> +
> +void init_sync_vmstat(void)
> +{
> +	raw_cpu_write(vmstat_dirty, true);
> +	set_thread_flag(TIF_TASK_ISOL);
> +}
> +EXPORT_SYMBOL_GPL(vmstat_dirty);
> +#else
> +static inline void mark_vmstat_dirty(void)
> +{
> +}
> +#endif
> +
>  /*
>   * For use when we know that interrupts are disabled,
>   * or when we know that preemption is disabled and that
> @@ -366,6 +391,7 @@ void __mod_zone_page_state(struct zone *
>  		x = 0;
>  	}
>  	__this_cpu_write(*p, x);
> +	mark_vmstat_dirty();
>  
>  	if (IS_ENABLED(CONFIG_PREEMPT_RT))
>  		preempt_enable();
> @@ -404,6 +430,7 @@ void __mod_node_page_state(struct pglist
>  		x = 0;
>  	}
>  	__this_cpu_write(*p, x);
> +	mark_vmstat_dirty();
>  
>  	if (IS_ENABLED(CONFIG_PREEMPT_RT))
>  		preempt_enable();
> @@ -602,6 +629,7 @@ static inline void mod_zone_state(struct
>  
>  	if (z)
>  		zone_page_state_add(z, zone, item);
> +	mark_vmstat_dirty();
>  }
>  
>  void mod_zone_page_state(struct zone *zone, enum zone_stat_item item,
> @@ -670,6 +698,7 @@ static inline void mod_node_state(struct
>  
>  	if (z)
>  		node_page_state_add(z, pgdat, item);
> +	mark_vmstat_dirty();
>  }
>  
>  void mod_node_page_state(struct pglist_data *pgdat, enum node_stat_item item,
> @@ -1087,6 +1116,7 @@ static void fill_contig_page_info(struct
>  			info->free_blocks_suitable += blocks <<
>  						(order - suitable_order);
>  	}
> +	mark_vmstat_dirty();
>  }
>  
>  /*
> @@ -1443,6 +1473,7 @@ static void walk_zones_in_node(struct se
>  		if (!nolock)
>  			spin_unlock_irqrestore(&zone->lock, flags);
>  	}
> +	mark_vmstat_dirty();
>  }
>  #endif
>  
> @@ -1512,6 +1543,7 @@ static void pagetypeinfo_showfree_print(
>  		}
>  		seq_putc(m, '\n');
>  	}
> +	mark_vmstat_dirty();
>  }
>  
>  /* Print out the free pages at each order for each migatetype */
> @@ -1932,6 +1964,7 @@ static void vmstat_update(struct work_st
>  				this_cpu_ptr(&vmstat_work),
>  				round_jiffies_relative(sysctl_stat_interval));
>  	}
> +	mark_vmstat_dirty();
>  }
>  
>  /*
> @@ -2019,13 +2052,14 @@ static void vmstat_shepherd(struct work_
>  }
>  
>  #ifdef CONFIG_TASK_ISOLATION
> -void sync_vmstat(void)
> +void __sync_vmstat(void)
>  {
>  	int cpu;
>  
>  	cpu = get_cpu();
>  
>  	refresh_cpu_vm_stats(false);
> +	raw_cpu_write(vmstat_dirty, false);

I still see a few raw_cpu_write() here, especially that one should
clearly become __this_cpu_write().

>  	put_cpu();
>  
>  	/*
> 
>