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@xxxxxxxxx>
> Reviewed-by: Roman Gushchin <roman.gushchin@xxxxxxxxx>
Acked-by: Michal Hocko <mhocko@xxxxxxxx>
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@xxxxxxxxx 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
