Re: [PATCH v1 1/6] memcg-v1: fully deprecate move_charge_at_immigrate

[Michal Hocko <mhocko@suse.com>]

On Thu 24-10-24 18:22:58, Shakeel Butt wrote:
> Proceed with the complete deprecation of memcg v1's charge moving
> feature. The deprecation warning has been in the kernel for almost two
> years and has been ported to all stable kernel since. Now is the time to
> fully deprecate this feature.
> 
> Signed-off-by: Shakeel Butt <shakeel.butt@linux.dev>
> Reviewed-by: Roman Gushchin <roman.gushchin@linux.dev>

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

> ---
> 
> Changes since RFC:
> - Writing 0 to memory.move_charge_at_immigrate is allowed.
> 
>  .../admin-guide/cgroup-v1/memory.rst          | 82 +------------------
>  mm/memcontrol-v1.c                            | 14 +---
>  2 files changed, 5 insertions(+), 91 deletions(-)
> 
> diff --git a/Documentation/admin-guide/cgroup-v1/memory.rst b/Documentation/admin-guide/cgroup-v1/memory.rst
> index 270501db9f4e..286d16fc22eb 100644
> --- a/Documentation/admin-guide/cgroup-v1/memory.rst
> +++ b/Documentation/admin-guide/cgroup-v1/memory.rst
> @@ -90,9 +90,7 @@ Brief summary of control files.
>                                       used.
>   memory.swappiness		     set/show swappiness parameter of vmscan
>  				     (See sysctl's vm.swappiness)
> - memory.move_charge_at_immigrate     set/show controls of moving charges
> -                                     This knob is deprecated and shouldn't be
> -                                     used.
> + memory.move_charge_at_immigrate     This knob is deprecated.
>   memory.oom_control		     set/show oom controls.
>                                       This knob is deprecated and shouldn't be
>                                       used.
> @@ -243,10 +241,6 @@ behind this approach is that a cgroup that aggressively uses a shared
>  page will eventually get charged for it (once it is uncharged from
>  the cgroup that brought it in -- this will happen on memory pressure).
>  
> -But see :ref:`section 8.2 <cgroup-v1-memory-movable-charges>` when moving a
> -task to another cgroup, its pages may be recharged to the new cgroup, if
> -move_charge_at_immigrate has been chosen.
> -
>  2.4 Swap Extension
>  --------------------------------------
>  
> @@ -756,78 +750,8 @@ If we want to change this to 1G, we can at any time use::
>  
>  THIS IS DEPRECATED!
>  
> -It's expensive and unreliable! It's better practice to launch workload
> -tasks directly from inside their target cgroup. Use dedicated workload
> -cgroups to allow fine-grained policy adjustments without having to
> -move physical pages between control domains.
> -
> -Users can move charges associated with a task along with task migration, that
> -is, uncharge task's pages from the old cgroup and charge them to the new cgroup.
> -This feature is not supported in !CONFIG_MMU environments because of lack of
> -page tables.
> -
> -8.1 Interface
> --------------
> -
> -This feature is disabled by default. It can be enabled (and disabled again) by
> -writing to memory.move_charge_at_immigrate of the destination cgroup.
> -
> -If you want to enable it::
> -
> -	# echo (some positive value) > memory.move_charge_at_immigrate
> -
> -.. note::
> -      Each bits of move_charge_at_immigrate has its own meaning about what type
> -      of charges should be moved. See :ref:`section 8.2
> -      <cgroup-v1-memory-movable-charges>` for details.
> -
> -.. note::
> -      Charges are moved only when you move mm->owner, in other words,
> -      a leader of a thread group.
> -
> -.. note::
> -      If we cannot find enough space for the task in the destination cgroup, we
> -      try to make space by reclaiming memory. Task migration may fail if we
> -      cannot make enough space.
> -
> -.. note::
> -      It can take several seconds if you move charges much.
> -
> -And if you want disable it again::
> -
> -	# echo 0 > memory.move_charge_at_immigrate
> -
> -.. _cgroup-v1-memory-movable-charges:
> -
> -8.2 Type of charges which can be moved
> ---------------------------------------
> -
> -Each bit in move_charge_at_immigrate has its own meaning about what type of
> -charges should be moved. But in any case, it must be noted that an account of
> -a page or a swap can be moved only when it is charged to the task's current
> -(old) memory cgroup.
> -
> -+---+--------------------------------------------------------------------------+
> -|bit| what type of charges would be moved ?                                    |
> -+===+==========================================================================+
> -| 0 | A charge of an anonymous page (or swap of it) used by the target task.   |
> -|   | You must enable Swap Extension (see 2.4) to enable move of swap charges. |
> -+---+--------------------------------------------------------------------------+
> -| 1 | A charge of file pages (normal file, tmpfs file (e.g. ipc shared memory) |
> -|   | and swaps of tmpfs file) mmapped by the target task. Unlike the case of  |
> -|   | anonymous pages, file pages (and swaps) in the range mmapped by the task |
> -|   | will be moved even if the task hasn't done page fault, i.e. they might   |
> -|   | not be the task's "RSS", but other task's "RSS" that maps the same file. |
> -|   | The mapcount of the page is ignored (the page can be moved independent   |
> -|   | of the mapcount). You must enable Swap Extension (see 2.4) to            |
> -|   | enable move of swap charges.                                             |
> -+---+--------------------------------------------------------------------------+
> -
> -8.3 TODO
> ---------
> -
> -- All of moving charge operations are done under cgroup_mutex. It's not good
> -  behavior to hold the mutex too long, so we may need some trick.
> +Reading memory.move_charge_at_immigrate will always return 0 and writing
> +to it will always return -EINVAL.
>  
>  9. Memory thresholds
>  ====================
> diff --git a/mm/memcontrol-v1.c b/mm/memcontrol-v1.c
> index 81d8819f13cd..9b3b1a446c65 100644
> --- a/mm/memcontrol-v1.c
> +++ b/mm/memcontrol-v1.c
> @@ -593,29 +593,19 @@ static inline int mem_cgroup_move_swap_account(swp_entry_t entry,
>  static u64 mem_cgroup_move_charge_read(struct cgroup_subsys_state *css,
>  				struct cftype *cft)
>  {
> -	return mem_cgroup_from_css(css)->move_charge_at_immigrate;
> +	return 0;
>  }
>  
>  #ifdef CONFIG_MMU
>  static int mem_cgroup_move_charge_write(struct cgroup_subsys_state *css,
>  				 struct cftype *cft, u64 val)
>  {
> -	struct mem_cgroup *memcg = mem_cgroup_from_css(css);
> -
>  	pr_warn_once("Cgroup memory moving (move_charge_at_immigrate) is deprecated. "
>  		     "Please report your usecase to linux-mm@kvack.org if you "
>  		     "depend on this functionality.\n");
>  
> -	if (val & ~MOVE_MASK)
> +	if (val != 0)
>  		return -EINVAL;
> -
> -	/*
> -	 * No kind of locking is needed in here, because ->can_attach() will
> -	 * check this value once in the beginning of the process, and then carry
> -	 * on with stale data. This means that changes to this value will only
> -	 * affect task migrations starting after the change.
> -	 */
> -	memcg->move_charge_at_immigrate = val;
>  	return 0;
>  }
>  #else
> -- 
> 2.43.5

-- 
Michal Hocko
SUSE Labs