mbox series

[0/7] memcontrol code cleanup and simplification

Message ID 20210413065153.63431-1-songmuchun@bytedance.com (mailing list archive)
Headers show
Series memcontrol code cleanup and simplification | expand

Message

Muchun Song April 13, 2021, 6:51 a.m. UTC 
This patch series is part of [1] patch series. Because those patches are
code cleanup or simplification. I gather those patches into a separate
series to make it easier to review.

[1] https://lore.kernel.org/linux-mm/20210409122959.82264-1-songmuchun@bytedance.com/

Muchun Song (7):
  mm: memcontrol: fix page charging in page replacement
  mm: memcontrol: bail out early when !mm in get_mem_cgroup_from_mm
  mm: memcontrol: remove the pgdata parameter of mem_cgroup_page_lruvec
  mm: memcontrol: simplify lruvec_holds_page_lru_lock
  mm: memcontrol: simplify the logic of objcg pinning memcg
  mm: memcontrol: move obj_cgroup_uncharge_pages() out of css_set_lock
  mm: vmscan: remove noinline_for_stack

 include/linux/memcontrol.h | 39 +++++++++--------------------
 mm/compaction.c            |  2 +-
 mm/memcontrol.c            | 61 ++++++++++++++++++++--------------------------
 mm/swap.c                  |  2 +-
 mm/vmscan.c                |  6 ++---
 mm/workingset.c            |  2 +-
 6 files changed, 43 insertions(+), 69 deletions(-)

Comments

Shakeel Butt April 13, 2021, 1:12 p.m. UTC | #1 
On Mon, Apr 12, 2021 at 11:57 PM Muchun Song <songmuchun@bytedance.com> wrote:
>
> All the callers of mem_cgroup_page_lruvec() just pass page_pgdat(page)
> as the 2nd parameter to it (except isolate_migratepages_block()). But
> for isolate_migratepages_block(), the page_pgdat(page) is also equal
> to the local variable of @pgdat. So mem_cgroup_page_lruvec() do not
> need the pgdat parameter. Just remove it to simplify the code.
>
> Signed-off-by: Muchun Song <songmuchun@bytedance.com>
> Acked-by: Johannes Weiner <hannes@cmpxchg.org>

Reviewed-by: Shakeel Butt <shakeelb@google.com>
Roman Gushchin April 13, 2021, 5:48 p.m. UTC | #2 
On Tue, Apr 13, 2021 at 02:51:49PM +0800, Muchun Song wrote:
> All the callers of mem_cgroup_page_lruvec() just pass page_pgdat(page)
> as the 2nd parameter to it (except isolate_migratepages_block()). But
> for isolate_migratepages_block(), the page_pgdat(page) is also equal
> to the local variable of @pgdat. So mem_cgroup_page_lruvec() do not
> need the pgdat parameter. Just remove it to simplify the code.
> 
> Signed-off-by: Muchun Song <songmuchun@bytedance.com>
> Acked-by: Johannes Weiner <hannes@cmpxchg.org>

Acked-by: Roman Gushchin <guro@fb.com>

> ---
>  include/linux/memcontrol.h | 10 +++++-----
>  mm/compaction.c            |  2 +-
>  mm/memcontrol.c            |  9 +++------
>  mm/swap.c                  |  2 +-
>  mm/workingset.c            |  2 +-
>  5 files changed, 11 insertions(+), 14 deletions(-)
> 
> diff --git a/include/linux/memcontrol.h b/include/linux/memcontrol.h
> index c960fd49c3e8..4f49865c9958 100644
> --- a/include/linux/memcontrol.h
> +++ b/include/linux/memcontrol.h
> @@ -743,13 +743,12 @@ static inline struct lruvec *mem_cgroup_lruvec(struct mem_cgroup *memcg,
>  /**
>   * mem_cgroup_page_lruvec - return lruvec for isolating/putting an LRU page
>   * @page: the page
> - * @pgdat: pgdat of the page
>   *
>   * This function relies on page->mem_cgroup being stable.
>   */
> -static inline struct lruvec *mem_cgroup_page_lruvec(struct page *page,
> -						struct pglist_data *pgdat)
> +static inline struct lruvec *mem_cgroup_page_lruvec(struct page *page)
>  {
> +	pg_data_t *pgdat = page_pgdat(page);
>  	struct mem_cgroup *memcg = page_memcg(page);
>  
>  	VM_WARN_ON_ONCE_PAGE(!memcg && !mem_cgroup_disabled(), page);
> @@ -1223,9 +1222,10 @@ static inline struct lruvec *mem_cgroup_lruvec(struct mem_cgroup *memcg,
>  	return &pgdat->__lruvec;
>  }
>  
> -static inline struct lruvec *mem_cgroup_page_lruvec(struct page *page,
> -						    struct pglist_data *pgdat)
> +static inline struct lruvec *mem_cgroup_page_lruvec(struct page *page)
>  {
> +	pg_data_t *pgdat = page_pgdat(page);
> +
>  	return &pgdat->__lruvec;
>  }
>  
> diff --git a/mm/compaction.c b/mm/compaction.c
> index caa4c36c1db3..e7da342003dd 100644
> --- a/mm/compaction.c
> +++ b/mm/compaction.c
> @@ -1033,7 +1033,7 @@ isolate_migratepages_block(struct compact_control *cc, unsigned long low_pfn,
>  		if (!TestClearPageLRU(page))
>  			goto isolate_fail_put;
>  
> -		lruvec = mem_cgroup_page_lruvec(page, pgdat);
> +		lruvec = mem_cgroup_page_lruvec(page);
>  
>  		/* If we already hold the lock, we can skip some rechecking */
>  		if (lruvec != locked) {
> diff --git a/mm/memcontrol.c b/mm/memcontrol.c
> index 9cbfff59b171..1f807448233e 100644
> --- a/mm/memcontrol.c
> +++ b/mm/memcontrol.c
> @@ -1177,9 +1177,8 @@ void lruvec_memcg_debug(struct lruvec *lruvec, struct page *page)
>  struct lruvec *lock_page_lruvec(struct page *page)
>  {
>  	struct lruvec *lruvec;
> -	struct pglist_data *pgdat = page_pgdat(page);
>  
> -	lruvec = mem_cgroup_page_lruvec(page, pgdat);
> +	lruvec = mem_cgroup_page_lruvec(page);
>  	spin_lock(&lruvec->lru_lock);
>  
>  	lruvec_memcg_debug(lruvec, page);
> @@ -1190,9 +1189,8 @@ struct lruvec *lock_page_lruvec(struct page *page)
>  struct lruvec *lock_page_lruvec_irq(struct page *page)
>  {
>  	struct lruvec *lruvec;
> -	struct pglist_data *pgdat = page_pgdat(page);
>  
> -	lruvec = mem_cgroup_page_lruvec(page, pgdat);
> +	lruvec = mem_cgroup_page_lruvec(page);
>  	spin_lock_irq(&lruvec->lru_lock);
>  
>  	lruvec_memcg_debug(lruvec, page);
> @@ -1203,9 +1201,8 @@ struct lruvec *lock_page_lruvec_irq(struct page *page)
>  struct lruvec *lock_page_lruvec_irqsave(struct page *page, unsigned long *flags)
>  {
>  	struct lruvec *lruvec;
> -	struct pglist_data *pgdat = page_pgdat(page);
>  
> -	lruvec = mem_cgroup_page_lruvec(page, pgdat);
> +	lruvec = mem_cgroup_page_lruvec(page);
>  	spin_lock_irqsave(&lruvec->lru_lock, *flags);
>  
>  	lruvec_memcg_debug(lruvec, page);
> diff --git a/mm/swap.c b/mm/swap.c
> index a75a8265302b..e0d5699213cc 100644
> --- a/mm/swap.c
> +++ b/mm/swap.c
> @@ -313,7 +313,7 @@ void lru_note_cost(struct lruvec *lruvec, bool file, unsigned int nr_pages)
>  
>  void lru_note_cost_page(struct page *page)
>  {
> -	lru_note_cost(mem_cgroup_page_lruvec(page, page_pgdat(page)),
> +	lru_note_cost(mem_cgroup_page_lruvec(page),
>  		      page_is_file_lru(page), thp_nr_pages(page));
>  }
>  
> diff --git a/mm/workingset.c b/mm/workingset.c
> index b7cdeca5a76d..4f7a306ce75a 100644
> --- a/mm/workingset.c
> +++ b/mm/workingset.c
> @@ -408,7 +408,7 @@ void workingset_activation(struct page *page)
>  	memcg = page_memcg_rcu(page);
>  	if (!mem_cgroup_disabled() && !memcg)
>  		goto out;
> -	lruvec = mem_cgroup_page_lruvec(page, page_pgdat(page));
> +	lruvec = mem_cgroup_page_lruvec(page);
>  	workingset_age_nonresident(lruvec, thp_nr_pages(page));
>  out:
>  	rcu_read_unlock();
> -- 
> 2.11.0
>
Michal Hocko April 14, 2021, 9:27 a.m. UTC | #3 
On Tue 13-04-21 14:51:49, Muchun Song wrote:
> All the callers of mem_cgroup_page_lruvec() just pass page_pgdat(page)
> as the 2nd parameter to it (except isolate_migratepages_block()). But
> for isolate_migratepages_block(), the page_pgdat(page) is also equal
> to the local variable of @pgdat. So mem_cgroup_page_lruvec() do not
> need the pgdat parameter. Just remove it to simplify the code.
> 
> Signed-off-by: Muchun Song <songmuchun@bytedance.com>
> Acked-by: Johannes Weiner <hannes@cmpxchg.org>

I like this. Two arguments where one can be directly inferred from the
first one can just lead to subtle bugs. In this case it even doesn't
give any advantage for most callers.

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

> ---
>  include/linux/memcontrol.h | 10 +++++-----
>  mm/compaction.c            |  2 +-
>  mm/memcontrol.c            |  9 +++------
>  mm/swap.c                  |  2 +-
>  mm/workingset.c            |  2 +-
>  5 files changed, 11 insertions(+), 14 deletions(-)
> 
> diff --git a/include/linux/memcontrol.h b/include/linux/memcontrol.h
> index c960fd49c3e8..4f49865c9958 100644
> --- a/include/linux/memcontrol.h
> +++ b/include/linux/memcontrol.h
> @@ -743,13 +743,12 @@ static inline struct lruvec *mem_cgroup_lruvec(struct mem_cgroup *memcg,
>  /**
>   * mem_cgroup_page_lruvec - return lruvec for isolating/putting an LRU page
>   * @page: the page
> - * @pgdat: pgdat of the page
>   *
>   * This function relies on page->mem_cgroup being stable.
>   */
> -static inline struct lruvec *mem_cgroup_page_lruvec(struct page *page,
> -						struct pglist_data *pgdat)
> +static inline struct lruvec *mem_cgroup_page_lruvec(struct page *page)
>  {
> +	pg_data_t *pgdat = page_pgdat(page);
>  	struct mem_cgroup *memcg = page_memcg(page);
>  
>  	VM_WARN_ON_ONCE_PAGE(!memcg && !mem_cgroup_disabled(), page);
> @@ -1223,9 +1222,10 @@ static inline struct lruvec *mem_cgroup_lruvec(struct mem_cgroup *memcg,
>  	return &pgdat->__lruvec;
>  }
>  
> -static inline struct lruvec *mem_cgroup_page_lruvec(struct page *page,
> -						    struct pglist_data *pgdat)
> +static inline struct lruvec *mem_cgroup_page_lruvec(struct page *page)
>  {
> +	pg_data_t *pgdat = page_pgdat(page);
> +
>  	return &pgdat->__lruvec;
>  }
>  
> diff --git a/mm/compaction.c b/mm/compaction.c
> index caa4c36c1db3..e7da342003dd 100644
> --- a/mm/compaction.c
> +++ b/mm/compaction.c
> @@ -1033,7 +1033,7 @@ isolate_migratepages_block(struct compact_control *cc, unsigned long low_pfn,
>  		if (!TestClearPageLRU(page))
>  			goto isolate_fail_put;
>  
> -		lruvec = mem_cgroup_page_lruvec(page, pgdat);
> +		lruvec = mem_cgroup_page_lruvec(page);
>  
>  		/* If we already hold the lock, we can skip some rechecking */
>  		if (lruvec != locked) {
> diff --git a/mm/memcontrol.c b/mm/memcontrol.c
> index 9cbfff59b171..1f807448233e 100644
> --- a/mm/memcontrol.c
> +++ b/mm/memcontrol.c
> @@ -1177,9 +1177,8 @@ void lruvec_memcg_debug(struct lruvec *lruvec, struct page *page)
>  struct lruvec *lock_page_lruvec(struct page *page)
>  {
>  	struct lruvec *lruvec;
> -	struct pglist_data *pgdat = page_pgdat(page);
>  
> -	lruvec = mem_cgroup_page_lruvec(page, pgdat);
> +	lruvec = mem_cgroup_page_lruvec(page);
>  	spin_lock(&lruvec->lru_lock);
>  
>  	lruvec_memcg_debug(lruvec, page);
> @@ -1190,9 +1189,8 @@ struct lruvec *lock_page_lruvec(struct page *page)
>  struct lruvec *lock_page_lruvec_irq(struct page *page)
>  {
>  	struct lruvec *lruvec;
> -	struct pglist_data *pgdat = page_pgdat(page);
>  
> -	lruvec = mem_cgroup_page_lruvec(page, pgdat);
> +	lruvec = mem_cgroup_page_lruvec(page);
>  	spin_lock_irq(&lruvec->lru_lock);
>  
>  	lruvec_memcg_debug(lruvec, page);
> @@ -1203,9 +1201,8 @@ struct lruvec *lock_page_lruvec_irq(struct page *page)
>  struct lruvec *lock_page_lruvec_irqsave(struct page *page, unsigned long *flags)
>  {
>  	struct lruvec *lruvec;
> -	struct pglist_data *pgdat = page_pgdat(page);
>  
> -	lruvec = mem_cgroup_page_lruvec(page, pgdat);
> +	lruvec = mem_cgroup_page_lruvec(page);
>  	spin_lock_irqsave(&lruvec->lru_lock, *flags);
>  
>  	lruvec_memcg_debug(lruvec, page);
> diff --git a/mm/swap.c b/mm/swap.c
> index a75a8265302b..e0d5699213cc 100644
> --- a/mm/swap.c
> +++ b/mm/swap.c
> @@ -313,7 +313,7 @@ void lru_note_cost(struct lruvec *lruvec, bool file, unsigned int nr_pages)
>  
>  void lru_note_cost_page(struct page *page)
>  {
> -	lru_note_cost(mem_cgroup_page_lruvec(page, page_pgdat(page)),
> +	lru_note_cost(mem_cgroup_page_lruvec(page),
>  		      page_is_file_lru(page), thp_nr_pages(page));
>  }
>  
> diff --git a/mm/workingset.c b/mm/workingset.c
> index b7cdeca5a76d..4f7a306ce75a 100644
> --- a/mm/workingset.c
> +++ b/mm/workingset.c
> @@ -408,7 +408,7 @@ void workingset_activation(struct page *page)
>  	memcg = page_memcg_rcu(page);
>  	if (!mem_cgroup_disabled() && !memcg)
>  		goto out;
> -	lruvec = mem_cgroup_page_lruvec(page, page_pgdat(page));
> +	lruvec = mem_cgroup_page_lruvec(page);
>  	workingset_age_nonresident(lruvec, thp_nr_pages(page));
>  out:
>  	rcu_read_unlock();
> -- 
> 2.11.0