linux-kernel - Re: [PATCH v3 3/7] mm/swap: always account swapped in page into current memcg

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <87sf2fgxi5.fsf@yhuang6-desk2.ccr.corp.intel.com>
Date: Tue, 30 Jan 2024 14:12:50 +0800
From: "Huang, Ying" <ying.huang@...el.com>
To: Kairui Song <ryncsn@...il.com>
Cc: linux-mm@...ck.org,  Kairui Song <kasong@...cent.com>,  Andrew Morton
 <akpm@...ux-foundation.org>,  Chris Li <chrisl@...nel.org>,  Hugh Dickins
 <hughd@...gle.com>,  Johannes Weiner <hannes@...xchg.org>,  Matthew Wilcox
 <willy@...radead.org>,  Michal Hocko <mhocko@...e.com>,  Yosry Ahmed
 <yosryahmed@...gle.com>,  David Hildenbrand <david@...hat.com>,
  linux-kernel@...r.kernel.org
Subject: Re: [PATCH v3 3/7] mm/swap: always account swapped in page into
 current memcg

Kairui Song <ryncsn@...il.com> writes:

> From: Kairui Song <kasong@...cent.com>
>
> Currently, mem_cgroup_swapin_charge_folio is always called with
> mm == NULL, except in swapin_direct.
>
> swapin_direct is only used when swapin should skip readahead
> and swapcache (SWP_SYNCHRONOUS_IO). All other callers of
> mem_cgroup_swapin_charge_folio are for swapin that should
> not skip readahead and cache.
>
> This could cause swapin charging to behave differently depending
> on swap device, which is unexpected.
>
> This is currently not happening because the only caller of
> swapin_direct is the direct anon page fault path, where mm always
> equals to current->mm, but will no longer be true if swapin_direct
> is shared and have other callers (eg, swapoff) to share the
> readahead skipping logic.
>
> So make swapin_direct also pass NULL for mm, so swpain charge
> will behave consistently and not effected by type of swapin device
> or readahead policy.
>
> After this, the second param of mem_cgroup_swapin_charge_folio is
> never used now, so it can be safely dropped.
>
> Signed-off-by: Kairui Song <kasong@...cent.com>
> ---
>  include/linux/memcontrol.h | 4 ++--
>  mm/memcontrol.c            | 5 ++---
>  mm/swap_state.c            | 7 +++----
>  3 files changed, 7 insertions(+), 9 deletions(-)
>
> diff --git a/include/linux/memcontrol.h b/include/linux/memcontrol.h
> index 20ff87f8e001..540590d80958 100644
> --- a/include/linux/memcontrol.h
> +++ b/include/linux/memcontrol.h
> @@ -693,7 +693,7 @@ static inline int mem_cgroup_charge(struct folio *folio, struct mm_struct *mm,
>  int mem_cgroup_hugetlb_try_charge(struct mem_cgroup *memcg, gfp_t gfp,
>  		long nr_pages);
>  
> -int mem_cgroup_swapin_charge_folio(struct folio *folio, struct mm_struct *mm,
> +int mem_cgroup_swapin_charge_folio(struct folio *folio,
>  				  gfp_t gfp, swp_entry_t entry);
>  void mem_cgroup_swapin_uncharge_swap(swp_entry_t entry);
>  
> @@ -1281,7 +1281,7 @@ static inline int mem_cgroup_hugetlb_try_charge(struct mem_cgroup *memcg,
>  }
>  
>  static inline int mem_cgroup_swapin_charge_folio(struct folio *folio,
> -			struct mm_struct *mm, gfp_t gfp, swp_entry_t entry)
> +		gfp_t gfp, swp_entry_t entry)
>  {
>  	return 0;
>  }
> diff --git a/mm/memcontrol.c b/mm/memcontrol.c
> index e4c8735e7c85..5852742df958 100644
> --- a/mm/memcontrol.c
> +++ b/mm/memcontrol.c
> @@ -7306,8 +7306,7 @@ int mem_cgroup_hugetlb_try_charge(struct mem_cgroup *memcg, gfp_t gfp,
>   *
>   * Returns 0 on success. Otherwise, an error code is returned.
>   */
> -int mem_cgroup_swapin_charge_folio(struct folio *folio, struct mm_struct *mm,
> -				  gfp_t gfp, swp_entry_t entry)
> +int mem_cgroup_swapin_charge_folio(struct folio *folio, gfp_t gfp, swp_entry_t entry)
>  {
>  	struct mem_cgroup *memcg;
>  	unsigned short id;
> @@ -7320,7 +7319,7 @@ int mem_cgroup_swapin_charge_folio(struct folio *folio, struct mm_struct *mm,
>  	rcu_read_lock();
>  	memcg = mem_cgroup_from_id(id);
>  	if (!memcg || !css_tryget_online(&memcg->css))
> -		memcg = get_mem_cgroup_from_mm(mm);
> +		memcg = get_mem_cgroup_from_current();

The behavior of get_mem_cgroup_from_mm(NULL) and
get_mem_cgroup_from_current() isn't same exactly.  Are you sure that
this is OK?

--
Best Regards,
Huang, Ying


>  	rcu_read_unlock();
>  
>  	ret = charge_memcg(folio, memcg, gfp);
> diff --git a/mm/swap_state.c b/mm/swap_state.c
> index 645f5bcad123..a450d09fc0db 100644
> --- a/mm/swap_state.c
> +++ b/mm/swap_state.c
> @@ -495,7 +495,7 @@ struct folio *__read_swap_cache_async(swp_entry_t entry, gfp_t gfp_mask,
>  	__folio_set_locked(folio);
>  	__folio_set_swapbacked(folio);
>  
> -	if (mem_cgroup_swapin_charge_folio(folio, NULL, gfp_mask, entry))
> +	if (mem_cgroup_swapin_charge_folio(folio, gfp_mask, entry))
>  		goto fail_unlock;
>  
>  	/* May fail (-ENOMEM) if XArray node allocation failed. */
> @@ -884,9 +884,8 @@ struct folio *swapin_direct(swp_entry_t entry, gfp_t gfp_mask,
>  		__folio_set_locked(folio);
>  		__folio_set_swapbacked(folio);
>  
> -		if (mem_cgroup_swapin_charge_folio(folio,
> -					vma->vm_mm, GFP_KERNEL,
> -					entry)) {
> +		if (mem_cgroup_swapin_charge_folio(folio, GFP_KERNEL,
> +						   entry)) {
>  			folio_unlock(folio);
>  			folio_put(folio);
>  			return NULL;