Re: [PATCH 5/9] memcg: remove the soft limit rbtree

On Tue 11-08-26 13:31:59, Shakeel Butt wrote:
> With soft limit reclaim gone, the per-node rbtree of cgroups in excess
> has no readers left. Remove the tree, the helpers maintaining it, and
> the subsys_initcall that existed only to allocate it. memcg1_check_events()
> no longer needs to feed it, which also drops the last caller of
> lru_gen_soft_reclaim().
> 
> Signed-off-by: Shakeel Butt <[email protected]>

Acked-by: Michal Hocko <[email protected]>
Thanks!

> ---
>  mm/memcontrol-v1.c | 176 +--------------------------------------------
>  mm/memcontrol-v1.h |   2 -
>  mm/memcontrol.c    |   1 -
>  3 files changed, 2 insertions(+), 177 deletions(-)
> 
> diff --git a/mm/memcontrol-v1.c b/mm/memcontrol-v1.c
> index b38b8d0f7f51..475f998b7643 100644
> --- a/mm/memcontrol-v1.c
> +++ b/mm/memcontrol-v1.c
> @@ -17,23 +17,6 @@
>  #include "swap_table.h"
>  #include "memcontrol-v1.h"
>  
> -/*
> - * Cgroups above their limits are maintained in a RB-Tree, independent of
> - * their hierarchy representation
> - */
> -
> -struct mem_cgroup_tree_per_node {
> -	struct rb_root rb_root;
> -	struct rb_node *rb_rightmost;
> -	spinlock_t lock;
> -};
> -
> -struct mem_cgroup_tree {
> -	struct mem_cgroup_tree_per_node *rb_tree_per_node[MAX_NUMNODES];
> -};
> -
> -static struct mem_cgroup_tree soft_limit_tree __read_mostly;
> -
>  /* for OOM */
>  struct mem_cgroup_eventfd_list {
>  	struct list_head list;
> @@ -99,133 +82,6 @@ static struct lockdep_map memcg_oom_lock_dep_map = {
>  
>  DEFINE_SPINLOCK(memcg_oom_lock);
>  
> -static void __mem_cgroup_insert_exceeded(struct mem_cgroup_per_node *mz,
> -					 struct mem_cgroup_tree_per_node *mctz,
> -					 unsigned long new_usage_in_excess)
> -{
> -	struct rb_node **p = &mctz->rb_root.rb_node;
> -	struct rb_node *parent = NULL;
> -	struct mem_cgroup_per_node *mz_node;
> -	bool rightmost = true;
> -
> -	if (mz->on_tree)
> -		return;
> -
> -	mz->usage_in_excess = new_usage_in_excess;
> -	if (!mz->usage_in_excess)
> -		return;
> -	while (*p) {
> -		parent = *p;
> -		mz_node = rb_entry(parent, struct mem_cgroup_per_node,
> -					tree_node);
> -		if (mz->usage_in_excess < mz_node->usage_in_excess) {
> -			p = &(*p)->rb_left;
> -			rightmost = false;
> -		} else {
> -			p = &(*p)->rb_right;
> -		}
> -	}
> -
> -	if (rightmost)
> -		mctz->rb_rightmost = &mz->tree_node;
> -
> -	rb_link_node(&mz->tree_node, parent, p);
> -	rb_insert_color(&mz->tree_node, &mctz->rb_root);
> -	mz->on_tree = true;
> -}
> -
> -static void __mem_cgroup_remove_exceeded(struct mem_cgroup_per_node *mz,
> -					 struct mem_cgroup_tree_per_node *mctz)
> -{
> -	if (!mz->on_tree)
> -		return;
> -
> -	if (&mz->tree_node == mctz->rb_rightmost)
> -		mctz->rb_rightmost = rb_prev(&mz->tree_node);
> -
> -	rb_erase(&mz->tree_node, &mctz->rb_root);
> -	mz->on_tree = false;
> -}
> -
> -static void mem_cgroup_remove_exceeded(struct mem_cgroup_per_node *mz,
> -				       struct mem_cgroup_tree_per_node *mctz)
> -{
> -	unsigned long flags;
> -
> -	spin_lock_irqsave(&mctz->lock, flags);
> -	__mem_cgroup_remove_exceeded(mz, mctz);
> -	spin_unlock_irqrestore(&mctz->lock, flags);
> -}
> -
> -static unsigned long soft_limit_excess(struct mem_cgroup *memcg)
> -{
> -	unsigned long nr_pages = page_counter_read(&memcg->memory);
> -	unsigned long soft_limit = READ_ONCE(memcg->soft_limit);
> -	unsigned long excess = 0;
> -
> -	if (nr_pages > soft_limit)
> -		excess = nr_pages - soft_limit;
> -
> -	return excess;
> -}
> -
> -static void memcg1_update_tree(struct mem_cgroup *memcg, int nid)
> -{
> -	unsigned long excess;
> -	struct mem_cgroup_per_node *mz;
> -	struct mem_cgroup_tree_per_node *mctz;
> -
> -	if (lru_gen_enabled()) {
> -		if (soft_limit_excess(memcg))
> -			lru_gen_soft_reclaim(memcg, nid);
> -		return;
> -	}
> -
> -	mctz = soft_limit_tree.rb_tree_per_node[nid];
> -	if (!mctz)
> -		return;
> -	/*
> -	 * Necessary to update all ancestors when hierarchy is used.
> -	 * because their event counter is not touched.
> -	 */
> -	for (; memcg; memcg = parent_mem_cgroup(memcg)) {
> -		mz = memcg->nodeinfo[nid];
> -		excess = soft_limit_excess(memcg);
> -		/*
> -		 * We have to update the tree if mz is on RB-tree or
> -		 * mem is over its softlimit.
> -		 */
> -		if (excess || mz->on_tree) {
> -			unsigned long flags;
> -
> -			spin_lock_irqsave(&mctz->lock, flags);
> -			/* if on-tree, remove it */
> -			if (mz->on_tree)
> -				__mem_cgroup_remove_exceeded(mz, mctz);
> -			/*
> -			 * Insert again. mz->usage_in_excess will be updated.
> -			 * If excess is 0, no tree ops.
> -			 */
> -			__mem_cgroup_insert_exceeded(mz, mctz, excess);
> -			spin_unlock_irqrestore(&mctz->lock, flags);
> -		}
> -	}
> -}
> -
> -void memcg1_remove_from_trees(struct mem_cgroup *memcg)
> -{
> -	struct mem_cgroup_tree_per_node *mctz;
> -	struct mem_cgroup_per_node *mz;
> -	int nid;
> -
> -	for_each_node(nid) {
> -		mz = memcg->nodeinfo[nid];
> -		mctz = soft_limit_tree.rb_tree_per_node[nid];
> -		if (mctz)
> -			mem_cgroup_remove_exceeded(mz, mctz);
> -	}
> -}
> -
>  static u64 mem_cgroup_move_charge_read(struct cgroup_subsys_state *css,
>  				struct cftype *cft)
>  {
> @@ -336,7 +192,7 @@ static void mem_cgroup_threshold(struct mem_cgroup *memcg)
>  	}
>  }
>  
> -/* Cgroup1: threshold notifications & softlimit tree updates */
> +/* Cgroup1: threshold notifications */
>  
>  /*
>   * Per memcg event counter is incremented at every pagein/pageout. With THP,
> @@ -405,17 +261,8 @@ static void memcg1_check_events(struct mem_cgroup *memcg, int nid)
>  	if (IS_ENABLED(CONFIG_PREEMPT_RT))
>  		return;
>  
> -	/* threshold event is triggered in finer grain than soft limit */
> -	if (unlikely(memcg1_event_ratelimit(memcg,
> -						MEM_CGROUP_TARGET_THRESH))) {
> -		bool do_softlimit;
> -
> -		do_softlimit = memcg1_event_ratelimit(memcg,
> -						MEM_CGROUP_TARGET_SOFTLIMIT);
> +	if (unlikely(memcg1_event_ratelimit(memcg, MEM_CGROUP_TARGET_THRESH)))
>  		mem_cgroup_threshold(memcg);
> -		if (unlikely(do_softlimit))
> -			memcg1_update_tree(memcg, nid);
> -	}
>  }
>  
>  void memcg1_commit_charge(struct folio *folio, struct mem_cgroup *memcg)
> @@ -2391,22 +2238,3 @@ void memcg1_free_events(struct mem_cgroup *memcg)
>  {
>  	free_percpu(memcg->events_percpu);
>  }
> -
> -static int __init memcg1_init(void)
> -{
> -	int node;
> -
> -	for_each_node(node) {
> -		struct mem_cgroup_tree_per_node *rtpn;
> -
> -		rtpn = kzalloc_node(sizeof(*rtpn), GFP_KERNEL, node);
> -
> -		rtpn->rb_root = RB_ROOT;
> -		rtpn->rb_rightmost = NULL;
> -		spin_lock_init(&rtpn->lock);
> -		soft_limit_tree.rb_tree_per_node[node] = rtpn;
> -	}
> -
> -	return 0;
> -}
> -subsys_initcall(memcg1_init);
> diff --git a/mm/memcontrol-v1.h b/mm/memcontrol-v1.h
> index 1e394269c613..fd611e66859a 100644
> --- a/mm/memcontrol-v1.h
> +++ b/mm/memcontrol-v1.h
> @@ -41,7 +41,6 @@ bool memcg1_alloc_events(struct mem_cgroup *memcg);
>  void memcg1_free_events(struct mem_cgroup *memcg);
>  
>  void memcg1_memcg_init(struct mem_cgroup *memcg);
> -void memcg1_remove_from_trees(struct mem_cgroup *memcg);
>  
>  static inline void memcg1_soft_limit_reset(struct mem_cgroup *memcg)
>  {
> @@ -98,7 +97,6 @@ static inline bool memcg1_alloc_events(struct mem_cgroup *memcg) { return true;
>  static inline void memcg1_free_events(struct mem_cgroup *memcg) {}
>  
>  static inline void memcg1_memcg_init(struct mem_cgroup *memcg) {}
> -static inline void memcg1_remove_from_trees(struct mem_cgroup *memcg) {}
>  static inline void memcg1_soft_limit_reset(struct mem_cgroup *memcg) {}
>  static inline void memcg1_css_offline(struct mem_cgroup *memcg) {}
>  
> diff --git a/mm/memcontrol.c b/mm/memcontrol.c
> index 1d3339520809..b68f1f16ae54 100644
> --- a/mm/memcontrol.c
> +++ b/mm/memcontrol.c
> @@ -4394,7 +4394,6 @@ static void mem_cgroup_css_free(struct cgroup_subsys_state *css)
>  
>  	vmpressure_cleanup(&memcg->vmpressure);
>  	cancel_work_sync(&memcg->high_work);
> -	memcg1_remove_from_trees(memcg);
>  	free_shrinker_info(memcg);
>  	mem_cgroup_free(memcg);
>  }
> -- 
> 2.53.0-Meta

-- 
Michal Hocko
SUSE Labs