Re: [PATCH 9/9] memcg: simplify v1 event ratelimiting

[Michal Hocko <mhocko@suse.com>]

On Tue 11-08-26 13:32:03, Shakeel Butt wrote:
> Thresholds are the only periodic v1 event left, so the target enum, the
> per-cpu target array and the switch in memcg1_event_ratelimit() all
> collapse to a single counter.
> 
> memcg1_check_events() no longer needs a node id either, which lets
> memcg1_uncharge_batch() drop its nid argument and struct
> uncharge_gather drop the field feeding it.
> 
> Signed-off-by: Shakeel Butt <shakeel.butt@linux.dev>

Acked-by: Michal Hocko <mhocko@suse.com>
Thanks!

> ---
>  mm/memcontrol-v1.c | 43 +++++++++++--------------------------------
>  mm/memcontrol-v1.h |  4 ++--
>  mm/memcontrol.c    |  4 +---
>  3 files changed, 14 insertions(+), 37 deletions(-)
> 
> diff --git a/mm/memcontrol-v1.c b/mm/memcontrol-v1.c
> index 475f998b7643..bf2c7d53b01b 100644
> --- a/mm/memcontrol-v1.c
> +++ b/mm/memcontrol-v1.c
> @@ -200,15 +200,9 @@ static void mem_cgroup_threshold(struct mem_cgroup *memcg)
>   * to trigger some periodic events. This is straightforward and better
>   * than using jiffies etc. to handle periodic memcg event.
>   */
> -enum mem_cgroup_events_target {
> -	MEM_CGROUP_TARGET_THRESH,
> -	MEM_CGROUP_TARGET_SOFTLIMIT,
> -	MEM_CGROUP_NTARGETS,
> -};
> -
>  struct memcg1_events_percpu {
>  	unsigned long nr_page_events;
> -	unsigned long targets[MEM_CGROUP_NTARGETS];
> +	unsigned long threshold_target;
>  };
>  
>  static void memcg1_charge_statistics(struct mem_cgroup *memcg, int nr_pages)
> @@ -225,43 +219,28 @@ static void memcg1_charge_statistics(struct mem_cgroup *memcg, int nr_pages)
>  }
>  
>  #define THRESHOLDS_EVENTS_TARGET 128
> -#define SOFTLIMIT_EVENTS_TARGET 1024
>  
> -static bool memcg1_event_ratelimit(struct mem_cgroup *memcg,
> -				enum mem_cgroup_events_target target)
> +static bool memcg1_event_ratelimit(struct mem_cgroup *memcg)
>  {
>  	unsigned long val, next;
>  
>  	val = __this_cpu_read(memcg->events_percpu->nr_page_events);
> -	next = __this_cpu_read(memcg->events_percpu->targets[target]);
> +	next = __this_cpu_read(memcg->events_percpu->threshold_target);
>  	/* from time_after() in jiffies.h */
>  	if ((long)(next - val) < 0) {
> -		switch (target) {
> -		case MEM_CGROUP_TARGET_THRESH:
> -			next = val + THRESHOLDS_EVENTS_TARGET;
> -			break;
> -		case MEM_CGROUP_TARGET_SOFTLIMIT:
> -			next = val + SOFTLIMIT_EVENTS_TARGET;
> -			break;
> -		default:
> -			break;
> -		}
> -		__this_cpu_write(memcg->events_percpu->targets[target], next);
> +		__this_cpu_write(memcg->events_percpu->threshold_target,
> +				 val + THRESHOLDS_EVENTS_TARGET);
>  		return true;
>  	}
>  	return false;
>  }
>  
> -/*
> - * Check events in order.
> - *
> - */
> -static void memcg1_check_events(struct mem_cgroup *memcg, int nid)
> +static void memcg1_check_events(struct mem_cgroup *memcg)
>  {
>  	if (IS_ENABLED(CONFIG_PREEMPT_RT))
>  		return;
>  
> -	if (unlikely(memcg1_event_ratelimit(memcg, MEM_CGROUP_TARGET_THRESH)))
> +	if (unlikely(memcg1_event_ratelimit(memcg)))
>  		mem_cgroup_threshold(memcg);
>  }
>  
> @@ -271,7 +250,7 @@ void memcg1_commit_charge(struct folio *folio, struct mem_cgroup *memcg)
>  
>  	local_irq_save(flags);
>  	memcg1_charge_statistics(memcg, folio_nr_pages(folio));
> -	memcg1_check_events(memcg, folio_nid(folio));
> +	memcg1_check_events(memcg);
>  	local_irq_restore(flags);
>  }
>  
> @@ -344,7 +323,7 @@ void __memcg1_swapout(struct folio *folio, struct swap_cluster_info *ci)
>  	VM_WARN_ON_IRQS_ENABLED();
>  	memcg1_charge_statistics(memcg, -folio_nr_pages(folio));
>  	preempt_enable_nested();
> -	memcg1_check_events(memcg, folio_nid(folio));
> +	memcg1_check_events(memcg);
>  
>  	rcu_read_unlock();
>  	obj_cgroup_put(objcg);
> @@ -398,14 +377,14 @@ void memcg1_swapin(struct folio *folio)
>  #endif
>  
>  void memcg1_uncharge_batch(struct mem_cgroup *memcg, unsigned long pgpgout,
> -			   unsigned long nr_memory, int nid)
> +			   unsigned long nr_memory)
>  {
>  	unsigned long flags;
>  
>  	local_irq_save(flags);
>  	count_memcg_events(memcg, PGPGOUT, pgpgout);
>  	__this_cpu_add(memcg->events_percpu->nr_page_events, nr_memory);
> -	memcg1_check_events(memcg, nid);
> +	memcg1_check_events(memcg);
>  	local_irq_restore(flags);
>  }
>  
> diff --git a/mm/memcontrol-v1.h b/mm/memcontrol-v1.h
> index f48d0e22e615..b9a21f0fd2c3 100644
> --- a/mm/memcontrol-v1.h
> +++ b/mm/memcontrol-v1.h
> @@ -59,7 +59,7 @@ void memcg1_oom_recover(struct mem_cgroup *memcg);
>  
>  void memcg1_commit_charge(struct folio *folio, struct mem_cgroup *memcg);
>  void memcg1_uncharge_batch(struct mem_cgroup *memcg, unsigned long pgpgout,
> -			   unsigned long nr_memory, int nid);
> +			   unsigned long nr_memory);
>  
>  void memcg1_stat_format(struct mem_cgroup *memcg, struct seq_buf *s);
>  void reparent_memcg1_state_local(struct mem_cgroup *memcg, struct mem_cgroup *parent);
> @@ -107,7 +107,7 @@ static inline void memcg1_commit_charge(struct folio *folio,
>  
>  static inline void memcg1_uncharge_batch(struct mem_cgroup *memcg,
>  					 unsigned long pgpgout,
> -					 unsigned long nr_memory, int nid) {}
> +					 unsigned long nr_memory) {}
>  
>  static inline void memcg1_stat_format(struct mem_cgroup *memcg, struct seq_buf *s) {}
>  
> diff --git a/mm/memcontrol.c b/mm/memcontrol.c
> index ba3ef821553d..44ef376d657b 100644
> --- a/mm/memcontrol.c
> +++ b/mm/memcontrol.c
> @@ -5293,7 +5293,6 @@ struct uncharge_gather {
>  	unsigned long nr_memory;
>  	unsigned long pgpgout;
>  	unsigned long nr_kmem;
> -	int nid;
>  };
>  
>  static inline void uncharge_gather_clear(struct uncharge_gather *ug)
> @@ -5316,7 +5315,7 @@ static void uncharge_batch(const struct uncharge_gather *ug)
>  		memcg1_oom_recover(memcg);
>  	}
>  
> -	memcg1_uncharge_batch(memcg, ug->pgpgout, ug->nr_memory, ug->nid);
> +	memcg1_uncharge_batch(memcg, ug->pgpgout, ug->nr_memory);
>  	rcu_read_unlock();
>  
>  	/* drop reference from uncharge_folio */
> @@ -5345,7 +5344,6 @@ static void uncharge_folio(struct folio *folio, struct uncharge_gather *ug)
>  			uncharge_gather_clear(ug);
>  		}
>  		ug->objcg = objcg;
> -		ug->nid = folio_nid(folio);
>  
>  		/* pairs with obj_cgroup_put in uncharge_batch */
>  		obj_cgroup_get(objcg);
> -- 
> 2.53.0-Meta

-- 
Michal Hocko
SUSE Labs