Add swap accounting to cgroup2

In the legacy hierarchy we charge memsw, which is dubious, because:

- memsw.limit must be >= memory.limit, so it is impossible to limit swap usage less than memory usage. Taking into account the fact that the primary limiting mechanism in the unified hierarchy is memory.high while memory.limit is either left unset or set to a very large value, moving memsw.limit knob to the unified hierarchy would effectively make it impossible to limit swap usage according to the user preference.

- memsw.usage != memory.usage + swap.usage, because a page occupying both swap entry and a swap cache page is charged only once to memsw counter. As a result, it is possible to effectively eat up to memory.limit of memory pages *and* memsw.limit of swap entries, which looks unexpected.

That said, we should provide a different swap limiting mechanism for cgroup2.

This patch adds mem_cgroup->swap counter, which charges the actual number of swap entries used by a cgroup. It is only charged in the unified hierarchy, while the legacy hierarchy memsw logic is left intact.

The swap usage can be monitored using new memory.swap.current file and limited using memory.swap.max.

Signed-off-by Vladimir Davydov <[email protected]>

---
 include/linux/memcontrol.h |   1 +
 include/linux/swap.h       |   5 ++
 mm/memcontrol.c            | 123 +++++++++++++++++++++++++++++++++++++++++----
 mm/shmem.c                 |   4 ++
 mm/swap_state.c            |   5 ++
 5 files changed, 129 insertions(+), 9 deletions(-)

include/linux/memcontrol.h +0 -0

index c6a5ed2f2744..993c9a26b637 100644

include/linux/swap.h +1 -1

@@ -1248,12 +1248,15 @@ static unsigned long mem_cgroup_get_limit(struct mem_cgroup *memcg)
 {
 	unsigned long limit;
 
-	limit = memcg->memory.limit;
+	limit = READ_ONCE(memcg->memory.limit);
 	if (mem_cgroup_swappiness(memcg)) {
 		unsigned long memsw_limit;

include/linux/swap.h +23 -0

@@ -5754,26 +5760,66 @@ void mem_cgroup_swapout(struct page *page, swp_entry_t entry)
 	memcg_check_events(memcg, page);
 }
 
+/*
+ * mem_cgroup_charge_swap - charge a swap entry
+ * @page: page being added to swap
+ * @entry: swap entry to charge
+ *
+ * Try to charge @entry to the memcg that @page belongs to.
+ *
+ * Returns 0 on success, -ENOMEM on failure.
+ */
+int mem_cgroup_charge_swap(struct page *page, swp_entry_t entry)
+{
+	struct mem_cgroup *memcg;
+	struct page_counter *counter;
+	unsigned short oldid;
+
+	if (!cgroup_subsys_on_dfl(memory_cgrp_subsys) || !do_swap_account)
+		return 0;
+
+	memcg = page->mem_cgroup;
+
+	/* Readahead page, never charged */
+	if (!memcg)
+		return 0;

On Thu, Dec 10, 2015 at 02:39:14PM +0300, Vladimir Davydov wrote:

Quoted 6 lines

include/linux/memcontrol.h +0 -0

index c6a5ed2f2744..993c9a26b637 100644
@@ -169,6 +169,7 @@ struct mem_cgroup {
 

include/linux/memcontrol.h struct mem_cgroup { · line 174

Quoted 5 lines

include/linux/memcontrol.h +1 -0

 	/* Accounted resources */
 	struct page_counter memory;
+	struct page_counter swap;
 	struct page_counter memsw;
 	struct page_counter kmem;

We should probably separate this to differentiate the new counters from the old ones. Only memory and swap are actual resources, the memsw and kmem counters are counting consumer-oriented.

include/linux/swap.h static inline int mem_cgroup_swappiness(struct mem_cgroup *mem) · line 371

Quoted 10 lines

include/linux/swap.h +1 -0

index 457181844b6e..f4b3ccdcba91 100644
@@ -368,11 +368,16 @@ static inline int mem_cgroup_swappiness(struct mem_cgroup *mem)
 #endif
 #ifdef CONFIG_MEMCG_SWAP
 extern void mem_cgroup_swapout(struct page *page, swp_entry_t entry);
+extern int mem_cgroup_charge_swap(struct page *page, swp_entry_t entry);

Should this be mem_cgroup_try_swap() to keep in line with the page counter terminology? So it's clear this is not forcing a charge.

Quoted 11 lines

include/linux/swap.h +2 -1

@@ -1248,12 +1248,15 @@ static unsigned long mem_cgroup_get_limit(struct mem_cgroup *memcg)
 {
 	unsigned long limit;
 
-	limit = memcg->memory.limit;
+	limit = READ_ONCE(memcg->memory.limit);
 	if (mem_cgroup_swappiness(memcg)) {
 		unsigned long memsw_limit;
+		unsigned long swap_limit;
 

include/linux/swap.h static unsigned long mem_cgroup_get_limit(struct mem_cgroup *memcg) · line 1261

Quoted 8 lines

include/linux/swap.h +4 -2

-		memsw_limit = memcg->memsw.limit;
-		limit = min(limit + total_swap_pages, memsw_limit);
+		memsw_limit = READ_ONCE(memcg->memsw.limit);
+		swap_limit = min(READ_ONCE(memcg->swap.limit),
+				 (unsigned long)total_swap_pages);
+		limit = min(limit + swap_limit, memsw_limit);
 	}
 	return limit;

This is taking a racy snapshot, so we don't rely on 100% accuracy. Can we do without the READ_ONCE()?

Quoted 34 lines

include/linux/swap.h +29 -0

@@ -5754,26 +5760,66 @@ void mem_cgroup_swapout(struct page *page, swp_entry_t entry)
 	memcg_check_events(memcg, page);
 }
 
+/*
+ * mem_cgroup_charge_swap - charge a swap entry
+ * @page: page being added to swap
+ * @entry: swap entry to charge
+ *
+ * Try to charge @entry to the memcg that @page belongs to.
+ *
+ * Returns 0 on success, -ENOMEM on failure.
+ */
+int mem_cgroup_charge_swap(struct page *page, swp_entry_t entry)
+{
+	struct mem_cgroup *memcg;
+	struct page_counter *counter;
+	unsigned short oldid;
+
+	if (!cgroup_subsys_on_dfl(memory_cgrp_subsys) || !do_swap_account)
+		return 0;
+
+	memcg = page->mem_cgroup;
+
+	/* Readahead page, never charged */
+	if (!memcg)
+		return 0;
+
+	if (!mem_cgroup_is_root(memcg) &&
+	    !page_counter_try_charge(&memcg->swap, 1, &counter))
+		return -ENOMEM;
+
+	oldid = swap_cgroup_record(entry, mem_cgroup_id(memcg));

include/linux/swap.h void mem_cgroup_swapout(struct page *page, swp_entry_t entry) · line 5795

Quoted 4 lines

include/linux/swap.h +4 -0

+	VM_BUG_ON_PAGE(oldid, page);
+	mem_cgroup_swap_statistics(memcg, true);
+
+	css_get(&memcg->css);

I think we don't have to duplicate the swap record code. Both cgroup1 and cgroup2 could run this function to handle the swapout record and statistics, and then mem_cgroup_swapout() would simply uncharge memsw.

include/linux/swap.h static int __init mem_cgroup_swap_init(void) · line 5937

Quoted 9 lines

include/linux/swap.h +2 -0

@@ -5828,6 +5931,8 @@ static int __init mem_cgroup_swap_init(void)
 {
 	if (!mem_cgroup_disabled() && really_do_swap_account) {
 		do_swap_account = 1;
+		WARN_ON(cgroup_add_dfl_cftypes(&memory_cgrp_subsys,
+					       swap_files));
 		WARN_ON(cgroup_add_legacy_cftypes(&memory_cgrp_subsys,
 						  memsw_cgroup_files));

I guess we could also support cgroup.memory=noswap.

In the legacy hierarchy we charge memsw, which is dubious, because:

- memsw.limit must be >= memory.limit, so it is impossible to limit swap usage less than memory usage. Taking into account the fact that the primary limiting mechanism in the unified hierarchy is memory.high while memory.limit is either left unset or set to a very large value, moving memsw.limit knob to the unified hierarchy would effectively make it impossible to limit swap usage according to the user preference.

- memsw.usage != memory.usage + swap.usage, because a page occupying both swap entry and a swap cache page is charged only once to memsw counter. As a result, it is possible to effectively eat up to memory.limit of memory pages *and* memsw.limit of swap entries, which looks unexpected.

That said, we should provide a different swap limiting mechanism for cgroup2.

This patch adds mem_cgroup->swap counter, which charges the actual number of swap entries used by a cgroup. It is only charged in the unified hierarchy, while the legacy hierarchy memsw logic is left intact.

The swap usage can be monitored using new memory.swap.current file and limited using memory.swap.max.

Signed-off-by Vladimir Davydov <[email protected]>

---
  include/linux/memcontrol.h |   1 +
  include/linux/swap.h       |   5 ++
  mm/memcontrol.c            | 123 +++++++++++++++++++++++++++++++++++++++++----
  mm/shmem.c                 |   4 ++
  mm/swap_state.c            |   5 ++
  5 files changed, 129 insertions(+), 9 deletions(-)

include/linux/memcontrol.h +1 -0

index c6a5ed2f2744..993c9a26b637 100644
@@ -169,6 +169,7 @@ struct mem_cgroup {
 
  	/* Accounted resources */
  	struct page_counter memory;
+	struct page_counter swap;
  	struct page_counter memsw;
  	struct page_counter kmem;
 

include/linux/swap.h +5 -0

index 457181844b6e..f4b3ccdcba91 100644
@@ -368,11 +368,16 @@ static inline int mem_cgroup_swappiness(struct mem_cgroup *mem)
  #endif
  #ifdef CONFIG_MEMCG_SWAP
  extern void mem_cgroup_swapout(struct page *page, swp_entry_t entry);
+extern int mem_cgroup_charge_swap(struct page *page, swp_entry_t entry);
  extern void mem_cgroup_uncharge_swap(swp_entry_t entry);
  #else
  static inline void mem_cgroup_swapout(struct page *page, swp_entry_t entry)
  {
  }
+static inline int mem_cgroup_charge_swap(struct page *page, swp_entry_t entry)
+{
+	return 0;
+}
  static inline void mem_cgroup_uncharge_swap(swp_entry_t entry)
  {
  }

mm/memcontrol.c +114 -9

index 7f5c6abf5421..9d10e2819ec4 100644
@@ -1212,7 +1212,7 @@ void mem_cgroup_print_oom_info(struct mem_cgroup *memcg, struct task_struct *p)
  		pr_cont(":");
 
  		for (i = 0; i < MEM_CGROUP_STAT_NSTATS; i++) {
-			if (i == MEM_CGROUP_STAT_SWAP && !do_memsw_account())
+			if (i == MEM_CGROUP_STAT_SWAP && !do_swap_account)
  				continue;
  			pr_cont(" %s:%luKB", mem_cgroup_stat_names[i],
  				K(mem_cgroup_read_stat(iter, i)));

@@ -1248,12 +1248,15 @@ static unsigned long mem_cgroup_get_limit(struct mem_cgroup *memcg)
  {
  	unsigned long limit;
 
-	limit = memcg->memory.limit;
+	limit = READ_ONCE(memcg->memory.limit);
  	if (mem_cgroup_swappiness(memcg)) {
  		unsigned long memsw_limit;
+		unsigned long swap_limit;
 
-		memsw_limit = memcg->memsw.limit;
-		limit = min(limit + total_swap_pages, memsw_limit);
+		memsw_limit = READ_ONCE(memcg->memsw.limit);
+		swap_limit = min(READ_ONCE(memcg->swap.limit),
+				 (unsigned long)total_swap_pages);
+		limit = min(limit + swap_limit, memsw_limit);
  	}
  	return limit;
  }

@@ -4226,6 +4229,7 @@ mem_cgroup_css_alloc(struct cgroup_subsys_state *parent_css)
  		page_counter_init(&memcg->memory, NULL);
  		memcg->high = PAGE_COUNTER_MAX;
  		memcg->soft_limit = PAGE_COUNTER_MAX;
+		page_counter_init(&memcg->swap, NULL);
  		page_counter_init(&memcg->memsw, NULL);
  		page_counter_init(&memcg->kmem, NULL);
  	}

@@ -4276,6 +4280,7 @@ mem_cgroup_css_online(struct cgroup_subsys_state *css)
  		page_counter_init(&memcg->memory, &parent->memory);
  		memcg->high = PAGE_COUNTER_MAX;
  		memcg->soft_limit = PAGE_COUNTER_MAX;
+		page_counter_init(&memcg->swap, &parent->swap);
  		page_counter_init(&memcg->memsw, &parent->memsw);
  		page_counter_init(&memcg->kmem, &parent->kmem);
  #if defined(CONFIG_MEMCG_LEGACY_KMEM) && defined(CONFIG_INET)

@@ -4291,6 +4296,7 @@ mem_cgroup_css_online(struct cgroup_subsys_state *css)
  		page_counter_init(&memcg->memory, NULL);
  		memcg->high = PAGE_COUNTER_MAX;
  		memcg->soft_limit = PAGE_COUNTER_MAX;
+		page_counter_init(&memcg->swap, NULL);
  		page_counter_init(&memcg->memsw, NULL);
  		page_counter_init(&memcg->kmem, NULL);
  #if defined(CONFIG_MEMCG_LEGACY_KMEM) && defined(CONFIG_INET)

@@ -5291,7 +5297,7 @@ int mem_cgroup_try_charge(struct page *page, struct mm_struct *mm,
  		if (page->mem_cgroup)
  			goto out;
 
-		if (do_memsw_account()) {
+		if (do_swap_account) {
  			swp_entry_t ent = { .val = page_private(page), };
  			unsigned short id = lookup_swap_cgroup_id(ent);
 

@@ -5754,26 +5760,66 @@ void mem_cgroup_swapout(struct page *page, swp_entry_t entry)
  	memcg_check_events(memcg, page);
  }
 
+/*
+ * mem_cgroup_charge_swap - charge a swap entry
+ * @page: page being added to swap
+ * @entry: swap entry to charge
+ *
+ * Try to charge @entry to the memcg that @page belongs to.
+ *
+ * Returns 0 on success, -ENOMEM on failure.
+ */
+int mem_cgroup_charge_swap(struct page *page, swp_entry_t entry)
+{
+	struct mem_cgroup *memcg;
+	struct page_counter *counter;
+	unsigned short oldid;
+
+	if (!cgroup_subsys_on_dfl(memory_cgrp_subsys) || !do_swap_account)
+		return 0;
+
+	memcg = page->mem_cgroup;
+
+	/* Readahead page, never charged */
+	if (!memcg)
+		return 0;
+
+	if (!mem_cgroup_is_root(memcg) &&
+	    !page_counter_try_charge(&memcg->swap, 1, &counter))
+		return -ENOMEM;
+
+	oldid = swap_cgroup_record(entry, mem_cgroup_id(memcg));
+	VM_BUG_ON_PAGE(oldid, page);
+	mem_cgroup_swap_statistics(memcg, true);
+
+	css_get(&memcg->css);
+	return 0;
+}
+
  /**
   * mem_cgroup_uncharge_swap - uncharge a swap entry
   * @entry: swap entry to uncharge
   *
- * Drop the memsw charge associated with @entry.
+ * Drop the swap charge associated with @entry.
   */
  void mem_cgroup_uncharge_swap(swp_entry_t entry)
  {
  	struct mem_cgroup *memcg;
  	unsigned short id;
 
-	if (!do_memsw_account())
+	if (!do_swap_account)
  		return;
 
  	id = swap_cgroup_record(entry, 0);
  	rcu_read_lock();
  	memcg = mem_cgroup_from_id(id);
  	if (memcg) {
-		if (!mem_cgroup_is_root(memcg))
-			page_counter_uncharge(&memcg->memsw, 1);
+		if (!mem_cgroup_is_root(memcg)) {
+			if (cgroup_subsys_on_dfl(memory_cgrp_subsys))
+				page_counter_uncharge(&memcg->swap, 1);
+			else
+				page_counter_uncharge(&memcg->memsw, 1);
+		}
  		mem_cgroup_swap_statistics(memcg, false);
  		css_put(&memcg->css);
  	}

@@ -5797,6 +5843,63 @@ static int __init enable_swap_account(char *s)
  }
  __setup("swapaccount=", enable_swap_account);
 
+static u64 swap_current_read(struct cgroup_subsys_state *css,
+			     struct cftype *cft)
+{
+	struct mem_cgroup *memcg = mem_cgroup_from_css(css);
+
+	return (u64)page_counter_read(&memcg->swap) * PAGE_SIZE;
+}
+
+static int swap_max_show(struct seq_file *m, void *v)
+{
+	struct mem_cgroup *memcg = mem_cgroup_from_css(seq_css(m));
+	unsigned long max = READ_ONCE(memcg->swap.limit);
+
+	if (max == PAGE_COUNTER_MAX)
+		seq_puts(m, "max\n");
+	else
+		seq_printf(m, "%llu\n", (u64)max * PAGE_SIZE);
+
+	return 0;
+}
+
+static ssize_t swap_max_write(struct kernfs_open_file *of,
+			      char *buf, size_t nbytes, loff_t off)
+{
+	struct mem_cgroup *memcg = mem_cgroup_from_css(of_css(of));
+	unsigned long max;
+	int err;
+
+	buf = strstrip(buf);
+	err = page_counter_memparse(buf, "max", &max);
+	if (err)
+		return err;
+
+	mutex_lock(&memcg_limit_mutex);
+	err = page_counter_limit(&memcg->swap, max);
+	mutex_unlock(&memcg_limit_mutex);
+	if (err)
+		return err;
+
+	return nbytes;
+}
+
+static struct cftype swap_files[] = {
+	{
+		.name = "swap.current",
+		.flags = CFTYPE_NOT_ON_ROOT,
+		.read_u64 = swap_current_read,
+	},
+	{
+		.name = "swap.max",
+		.flags = CFTYPE_NOT_ON_ROOT,
+		.seq_show = swap_max_show,
+		.write = swap_max_write,
+	},
+	{ }	/* terminate */
+};
+
  static struct cftype memsw_cgroup_files[] = {
  	{
  		.name = "memsw.usage_in_bytes",

@@ -5828,6 +5931,8 @@ static int __init mem_cgroup_swap_init(void)
  {
  	if (!mem_cgroup_disabled() && really_do_swap_account) {
  		do_swap_account = 1;
+		WARN_ON(cgroup_add_dfl_cftypes(&memory_cgrp_subsys,
+					       swap_files));
  		WARN_ON(cgroup_add_legacy_cftypes(&memory_cgrp_subsys,
  						  memsw_cgroup_files));
  	}

mm/shmem.c +4 -0

index 9b051115a100..659a90d8305c 100644
@@ -912,6 +912,9 @@ static int shmem_writepage(struct page *page, struct writeback_control *wbc)
  	if (!swap.val)
  		goto redirty;
 
+	if (mem_cgroup_charge_swap(page, swap))
+		goto free_swap;
+
  	/*
  	 * Add inode to shmem_unuse()'s list of swapped-out inodes,
  	 * if it's not already there.  Do it now before the page is

@@ -940,6 +943,7 @@ static int shmem_writepage(struct page *page, struct writeback_control *wbc)
  	}
 
  	mutex_unlock(&shmem_swaplist_mutex);
+free_swap:
  	swapcache_free(swap);
  redirty:
  	set_page_dirty(page);

mm/swap_state.c +0 -0

index d783872d746c..dea39cb03967 100644
@@ -170,6 +170,11 @@ int add_to_swap(struct page *page, struct list_head *list)
  	if (!entry.val)

Quoted 24 lines · Vladimir Davydov

In the legacy hierarchy we charge memsw, which is dubious, because:

- memsw.limit must be >= memory.limit, so it is impossible to limit swap usage less than memory usage. Taking into account the fact that the primary limiting mechanism in the unified hierarchy is memory.high while memory.limit is either left unset or set to a very large value, moving memsw.limit knob to the unified hierarchy would effectively make it impossible to limit swap usage according to the user preference.

- memsw.usage != memory.usage + swap.usage, because a page occupying both swap entry and a swap cache page is charged only once to memsw counter. As a result, it is possible to effectively eat up to memory.limit of memory pages *and* memsw.limit of swap entries, which looks unexpected.

That said, we should provide a different swap limiting mechanism for cgroup2.

This patch adds mem_cgroup->swap counter, which charges the actual number of swap entries used by a cgroup. It is only charged in the unified hierarchy, while the legacy hierarchy memsw logic is left intact.

Quoted 4 lines

The swap usage can be monitored using new memory.swap.current file and limited using memory.swap.max.

Signed-off-by Vladimir Davydov <[email protected]>

setting swap.max=0 will work like mlock ?

In the legacy hierarchy we charge memsw, which is dubious, because:

- memsw.limit must be >= memory.limit, so it is impossible to limit swap usage less than memory usage. Taking into account the fact that the primary limiting mechanism in the unified hierarchy is memory.high while memory.limit is either left unset or set to a very large value, moving memsw.limit knob to the unified hierarchy would effectively make it impossible to limit swap usage according to the user preference.

- memsw.usage != memory.usage + swap.usage, because a page occupying both swap entry and a swap cache page is charged only once to memsw counter. As a result, it is possible to effectively eat up to memory.limit of memory pages *and* memsw.limit of swap entries, which looks unexpected.

That said, we should provide a different swap limiting mechanism for cgroup2.
This patch adds mem_cgroup->swap counter, which charges the actual number of swap entries used by a cgroup. It is only charged in the unified hierarchy, while the legacy hierarchy memsw logic is left intact.

The swap usage can be monitored using new memory.swap.current file and limited using memory.swap.max.

Signed-off-by Vladimir Davydov <[email protected]>

---
 include/linux/memcontrol.h |   1 +
 include/linux/swap.h       |   5 ++
 mm/memcontrol.c            | 123 +++++++++++++++++++++++++++++++++++++++++----
 mm/shmem.c                 |   4 ++
 mm/swap_state.c            |   5 ++
 5 files changed, 129 insertions(+), 9 deletions(-)

Quoted 16 lines · Vladimir Davydov

In the legacy hierarchy we charge memsw, which is dubious, because:

- memsw.limit must be >= memory.limit, so it is impossible to limit swap usage less than memory usage. Taking into account the fact that the primary limiting mechanism in the unified hierarchy is memory.high while memory.limit is either left unset or set to a very large value, moving memsw.limit knob to the unified hierarchy would effectively make it impossible to limit swap usage according to the user preference.

- memsw.usage != memory.usage + swap.usage, because a page occupying both swap entry and a swap cache page is charged only once to memsw counter. As a result, it is possible to effectively eat up to memory.limit of memory pages *and* memsw.limit of swap entries, which looks unexpected.

Quoted 6 lines

That said, we should provide a different swap limiting mechanism for cgroup2.
This patch adds mem_cgroup->swap counter, which charges the actual number of swap entries used by a cgroup. It is only charged in the unified hierarchy, while the legacy hierarchy memsw logic is left intact.

I agree that the previous semantic was awkward. The problem I can see with this approach is that once the swap limit is reached the anon memory pressure might spill over to other and unrelated memcgs during the global memory pressure. I guess this is what Kame referred to as anon would become mlocked basically. This would be even more of an issue with resource delegation to sub-hierarchies because nobody will prevent setting the swap amount to a small value and use that as an anon memory protection.

Quoted 16 lines · Vladimir Davydov

In the legacy hierarchy we charge memsw, which is dubious, because:

- memsw.limit must be >= memory.limit, so it is impossible to limit swap usage less than memory usage. Taking into account the fact that the primary limiting mechanism in the unified hierarchy is memory.high while memory.limit is either left unset or set to a very large value, moving memsw.limit knob to the unified hierarchy would effectively make it impossible to limit swap usage according to the user preference.

- memsw.usage != memory.usage + swap.usage, because a page occupying both swap entry and a swap cache page is charged only once to memsw counter. As a result, it is possible to effectively eat up to memory.limit of memory pages *and* memsw.limit of swap entries, which looks unexpected.

Quoted 6 lines

That said, we should provide a different swap limiting mechanism for cgroup2.
This patch adds mem_cgroup->swap counter, which charges the actual number of swap entries used by a cgroup. It is only charged in the unified hierarchy, while the legacy hierarchy memsw logic is left intact.

I agree that the previous semantic was awkward. The problem I can see with this approach is that once the swap limit is reached the anon memory pressure might spill over to other and unrelated memcgs during the global memory pressure. I guess this is what Kame referred to as anon would become mlocked basically. This would be even more of an issue with resource delegation to sub-hierarchies because nobody will prevent setting the swap amount to a small value and use that as an anon memory protection.

My question now is. Is the knob usable/useful even without additional heuristics? Do we want to protect swap space so rigidly that a swap limited memcg can cause bigger problems than without the swap limit globally?

Anyway, if you don't trust a container you'd better set the hard memory limit so that it can't hurt others no matter what it runs and how it tweaks its sub-tree knobs.

Quoted 24 lines · Michal Hocko

Quoted 16 lines · Vladimir Davydov

In the legacy hierarchy we charge memsw, which is dubious, because:

- memsw.limit must be >= memory.limit, so it is impossible to limit swap usage less than memory usage. Taking into account the fact that the primary limiting mechanism in the unified hierarchy is memory.high while memory.limit is either left unset or set to a very large value, moving memsw.limit knob to the unified hierarchy would effectively make it impossible to limit swap usage according to the user preference.

- memsw.usage != memory.usage + swap.usage, because a page occupying both swap entry and a swap cache page is charged only once to memsw counter. As a result, it is possible to effectively eat up to memory.limit of memory pages *and* memsw.limit of swap entries, which looks unexpected.

Quoted 6 lines

That said, we should provide a different swap limiting mechanism for cgroup2.
This patch adds mem_cgroup->swap counter, which charges the actual number of swap entries used by a cgroup. It is only charged in the unified hierarchy, while the legacy hierarchy memsw logic is left intact.

Quoted 8 lines

I agree that the previous semantic was awkward. The problem I can see with this approach is that once the swap limit is reached the anon memory pressure might spill over to other and unrelated memcgs during the global memory pressure. I guess this is what Kame referred to as anon would become mlocked basically. This would be even more of an issue with resource delegation to sub-hierarchies because nobody will prevent setting the swap amount to a small value and use that as an anon memory protection.

AFAICS such anon memory protection has a side-effect: real-life workloads need page cache to run smoothly (at least for mapping executables). Disabling swapping would switch pressure to page caches, resulting in performance degradation. So, I don't think per memcg swap limit can be abused to boost your workload on an overcommitted system.

If you mean malicious users, well, they already have plenty ways to eat all available memory up to the hard limit by creating unreclaimable kernel objects.

Anyway, if you don't trust a container you'd better set the hard memory limit so that it can't hurt others no matter what it runs and how it tweaks its sub-tree knobs.

Quoted 40 lines · Vladimir Davydov

Quoted 24 lines · Michal Hocko

Quoted 16 lines · Vladimir Davydov

In the legacy hierarchy we charge memsw, which is dubious, because:

- memsw.limit must be >= memory.limit, so it is impossible to limit swap usage less than memory usage. Taking into account the fact that the primary limiting mechanism in the unified hierarchy is memory.high while memory.limit is either left unset or set to a very large value, moving memsw.limit knob to the unified hierarchy would effectively make it impossible to limit swap usage according to the user preference.

- memsw.usage != memory.usage + swap.usage, because a page occupying both swap entry and a swap cache page is charged only once to memsw counter. As a result, it is possible to effectively eat up to memory.limit of memory pages *and* memsw.limit of swap entries, which looks unexpected.

Quoted 6 lines

That said, we should provide a different swap limiting mechanism for cgroup2.
This patch adds mem_cgroup->swap counter, which charges the actual number of swap entries used by a cgroup. It is only charged in the unified hierarchy, while the legacy hierarchy memsw logic is left intact.

Quoted 8 lines

I agree that the previous semantic was awkward. The problem I can see with this approach is that once the swap limit is reached the anon memory pressure might spill over to other and unrelated memcgs during the global memory pressure. I guess this is what Kame referred to as anon would become mlocked basically. This would be even more of an issue with resource delegation to sub-hierarchies because nobody will prevent setting the swap amount to a small value and use that as an anon memory protection.

AFAICS such anon memory protection has a side-effect: real-life workloads need page cache to run smoothly (at least for mapping executables). Disabling swapping would switch pressure to page caches, resulting in performance degradation. So, I don't think per memcg swap limit can be abused to boost your workload on an overcommitted system.

Quoted 4 lines

If you mean malicious users, well, they already have plenty ways to eat all available memory up to the hard limit by creating unreclaimable kernel objects.

"protect anon" user's malicious degree is far lower than such cracker like users.

Quoted 5 lines

Anyway, if you don't trust a container you'd better set the hard memory limit so that it can't hurt others no matter what it runs and how it tweaks its sub-tree knobs.

Limiting swap can easily cause "OOM-Killer even while there are available swap" with easy mistake.

Can't you add "swap excess" switch to sysctl to allow global memory reclaim can ignore swap limitation ?

Quoted 41 lines · Kamezawa Hiroyuki

Quoted 40 lines · Vladimir Davydov

Quoted 24 lines · Michal Hocko

Quoted 16 lines · Vladimir Davydov

In the legacy hierarchy we charge memsw, which is dubious, because:

- memsw.limit must be >= memory.limit, so it is impossible to limit swap usage less than memory usage. Taking into account the fact that the primary limiting mechanism in the unified hierarchy is memory.high while memory.limit is either left unset or set to a very large value, moving memsw.limit knob to the unified hierarchy would effectively make it impossible to limit swap usage according to the user preference.

- memsw.usage != memory.usage + swap.usage, because a page occupying both swap entry and a swap cache page is charged only once to memsw counter. As a result, it is possible to effectively eat up to memory.limit of memory pages *and* memsw.limit of swap entries, which looks unexpected.

Quoted 6 lines

That said, we should provide a different swap limiting mechanism for cgroup2.
This patch adds mem_cgroup->swap counter, which charges the actual number of swap entries used by a cgroup. It is only charged in the unified hierarchy, while the legacy hierarchy memsw logic is left intact.

Quoted 8 lines

I agree that the previous semantic was awkward. The problem I can see with this approach is that once the swap limit is reached the anon memory pressure might spill over to other and unrelated memcgs during the global memory pressure. I guess this is what Kame referred to as anon would become mlocked basically. This would be even more of an issue with resource delegation to sub-hierarchies because nobody will prevent setting the swap amount to a small value and use that as an anon memory protection.

AFAICS such anon memory protection has a side-effect: real-life workloads need page cache to run smoothly (at least for mapping executables). Disabling swapping would switch pressure to page caches, resulting in performance degradation. So, I don't think per memcg swap limit can be abused to boost your workload on an overcommitted system.

Quoted 5 lines

Quoted 4 lines

If you mean malicious users, well, they already have plenty ways to eat all available memory up to the hard limit by creating unreclaimable kernel objects.

"protect anon" user's malicious degree is far lower than such cracker like users.

What do you mean by "malicious degree"? What is such a user trying to achieve? Killing the system? Well, there are much more effective ways to do so. Or does it want to exploit a system specific feature to get benefit for itself? If so, it will hardly win by mlocking all anonymous memory, because this will result in higher pressure exerted upon its page cache and dcache, which normal workloads just can't get along without.

Quoted 8 lines

Quoted 5 lines

Anyway, if you don't trust a container you'd better set the hard memory limit so that it can't hurt others no matter what it runs and how it tweaks its sub-tree knobs.

Limiting swap can easily cause "OOM-Killer even while there are available swap" with easy mistake.

What do you mean by "easy mistake"? Misconfiguration? If so, it's a lame excuse IMO. Admin should take system configuration seriously. If the host is not overcommitted, it's trivial. Otherwise, there's always a chance that things will go south, so it's not going to be easy. It's up to admin to analyze risks and set limits accordingly. Exporting knobs with clear meaning is the best we can do here. swap.max is one such knob It defines maximal usage of swap resource. Allowing to breach it just does not add up.

Quoted 2 lines

Can't you add "swap excess" switch to sysctl to allow global memory reclaim can ignore swap limitation ?

I'd be opposed to it, because this would obscure the user API. OTOH, a kind of swap soft limit (swap.high?) might be considered. I'm not sure if it's really necessary though, because all arguments for it do not look convincing to me for now. So, personally, I would refrain from implementing it until it is really called for by users of cgroup v2.

Thanks,
Vladimir
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to [email protected]
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/

Quoted 3 lines · Vladimir Davydov

Anyway, if you don't trust a container you'd better set the hard memory limit so that it can't hurt others no matter what it runs and how it tweaks its sub-tree knobs.

Limiting swap can easily cause "OOM-Killer even while there are available swap" with easy mistake. Can't you add "swap excess" switch to sysctl to allow global memory reclaim can ignore swap limitation ?

Quoted 8 lines · Kamezawa Hiroyuki

Quoted 3 lines · Vladimir Davydov

Anyway, if you don't trust a container you'd better set the hard memory limit so that it can't hurt others no matter what it runs and how it tweaks its sub-tree knobs.

Limiting swap can easily cause "OOM-Killer even while there are available swap" with easy mistake. Can't you add "swap excess" switch to sysctl to allow global memory reclaim can ignore swap limitation ?

That never worked with a combined memory+swap limit, either. How could it? The parent might swap you out under pressure, but simply touching a few of your anon pages causes them to get swapped back in, thrashing with whatever the parent was trying to do. Your ability to swap it out is simply no protection against a group touching its pages.

Allowing the parent to exceed swap with separate counters makes even less sense, because every page swapped out frees up a page of memory that the child can reuse. For every swap page that exceeds the limit, the child gets a free memory page! The child doesn't even have to cause swapin, it can just steal whatever the parent tried to free up, and meanwhile its combined memory & swap footprint explodes.

The answer is and always should have been: don't overcommit untrusted cgroups. Think of swap as a resource you distribute, not as breathing room for the parents to rely on. Because it can't and could never.

And the new separate swap counter makes this explicit.

Hmm, my requests are
- set the same capabilities as mlock() to set swap.limit=0

- swap-full notification via vmpressure or something mechanism.

- OOM-Killer's available memory calculation may be corrupted, please check.

- force swap-in at reducing swap.limit

Quoted 2 lines · Kamezawa Hiroyuki

Hmm, my requests are
- set the same capabilities as mlock() to set swap.limit=0

Setting swap.max is already privileged operation.

Quoted 1 line

- swap-full notification via vmpressure or something mechanism.

Why?

Quoted 1 line

- OOM-Killer's available memory calculation may be corrupted, please check.

Vladimir updated mem_cgroup_get_limit().

Quoted 1 line

- force swap-in at reducing swap.limit

Why?

Quoted 5 lines · Johannes Weiner

Quoted 1 line · Kamezawa Hiroyuki

- swap-full notification via vmpressure or something mechanism.

Why?

I think it's a sign of unhealthy condition, starting file cache drop rate to rise. But I forgot that there are resource threshold notifier already. Does the notifier work for swap.usage ?

Quoted 4 lines

Quoted 1 line

- force swap-in at reducing swap.limit

Why?

If full, swap.limit cannot be reduced even if there are available memory in a cgroup. Another cgroup cannot make use of the swap resource while it's occupied by other cgroup. The job scheduler should have a chance to fix the situation.

Quoted 7 lines · Kamezawa Hiroyuki

Quoted 5 lines · Johannes Weiner

Quoted 1 line · Kamezawa Hiroyuki

- swap-full notification via vmpressure or something mechanism.

Why?

Quoted 3 lines

I think it's a sign of unhealthy condition, starting file cache drop rate to rise. But I forgot that there are resource threshold notifier already. Does the notifier work for swap.usage ?

That will be reflected in vmpressure or other distress mechanisms. I'm not convinced "ran out of swap space" needs special casing in any way.

Quoted 7 lines

Quoted 4 lines

Quoted 1 line

- force swap-in at reducing swap.limit

Why?

If full, swap.limit cannot be reduced even if there are available memory in a cgroup. Another cgroup cannot make use of the swap resource while it's occupied by other cgroup. The job scheduler should have a chance to fix the situation.

I don't see why swap space allowance would need to be as dynamically adjustable as the memory allowance. There is usually no need to be as tight with swap space as with memory, and the performance penalty of swapping, even with flash drives, is high enough that swap space acts as an overflow vessel rather than be part of the regularly backing of the anonymous/shmem working set. It really is NOT obvious that swap space would need to be adjusted on the fly, and that it's important that reducing the limit will be reflected in consumption right away.

We shouldn't be adding hundreds of lines of likely terrible heuristics code* on speculation that somebody MIGHT find this useful in real life. We should wait until we are presented with a real usecase that applies to a whole class of users, and then see what the true requirements are.

* If a group has 200M swapped out and the swap limit is reduced by 10M below the current consumption, which pages would you swap in? There is no LRU list for swap space.

Quoted 24 lines · Michal Hocko

Quoted 16 lines · Vladimir Davydov

In the legacy hierarchy we charge memsw, which is dubious, because:

- memsw.limit must be >= memory.limit, so it is impossible to limit swap usage less than memory usage. Taking into account the fact that the primary limiting mechanism in the unified hierarchy is memory.high while memory.limit is either left unset or set to a very large value, moving memsw.limit knob to the unified hierarchy would effectively make it impossible to limit swap usage according to the user preference.

- memsw.usage != memory.usage + swap.usage, because a page occupying both swap entry and a swap cache page is charged only once to memsw counter. As a result, it is possible to effectively eat up to memory.limit of memory pages *and* memsw.limit of swap entries, which looks unexpected.

Quoted 6 lines

That said, we should provide a different swap limiting mechanism for cgroup2.
This patch adds mem_cgroup->swap counter, which charges the actual number of swap entries used by a cgroup. It is only charged in the unified hierarchy, while the legacy hierarchy memsw logic is left intact.

Quoted 8 lines

I agree that the previous semantic was awkward. The problem I can see with this approach is that once the swap limit is reached the anon memory pressure might spill over to other and unrelated memcgs during the global memory pressure. I guess this is what Kame referred to as anon would become mlocked basically. This would be even more of an issue with resource delegation to sub-hierarchies because nobody will prevent setting the swap amount to a small value and use that as an anon memory protection.

AFAICS such anon memory protection has a side-effect: real-life workloads need page cache to run smoothly (at least for mapping executables). Disabling swapping would switch pressure to page caches, resulting in performance degradation. So, I don't think per memcg swap limit can be abused to boost your workload on an overcommitted system.

If you mean malicious users, well, they already have plenty ways to eat all available memory up to the hard limit by creating unreclaimable kernel objects.

Anyway, if you don't trust a container you'd better set the hard memory limit so that it can't hurt others no matter what it runs and how it tweaks its sub-tree knobs.

...

Quoted 4 lines

My question now is. Is the knob usable/useful even without additional heuristics? Do we want to protect swap space so rigidly that a swap limited memcg can cause bigger problems than without the swap limit globally?

Hmm, I don't see why problems might get bigger with per memcg swap limit than w/o it.

W/o swap limit, a memcg can eat all swap space on the host and disable swapping for everyone, not just for itself alone.

Quoted 5 lines

AFAICS such anon memory protection has a side-effect: real-life workloads need page cache to run smoothly (at least for mapping executables). Disabling swapping would switch pressure to page caches, resulting in performance degradation. So, I don't think per memcg swap limit can be abused to boost your workload on an overcommitted system.

Well, you can trash on the page cache which could slow down the workload but the executable pages get an additional protection so this might be not sufficient and still trigger a massive disruption on the global level.

Just to make it clear. I am not against the new way of the swap accounting. It is much more clear then the previous one. I am just worried it allows for an easy misconfiguration and we do not have any measures to help the global system healthiness. I am OK with the patch if we document the risk for now. I still think we will end up doing some heuristic to throttle for a large unreclaimable high limit excess in the future but I agree this shouldn't be the prerequisite.

I completely agree that malicious/untrusted users absolutely have to be capped by the hard limit. Then the separate swap limit would work for sure. But I am less convinced about usefulness of the rigid (to the global memory pressure) swap limit without the hard limit. All the memory that could have been swapped out will make a memory pressure to the rest of the system without being punished for it too much. Memcg is allowed to grow over the high limit (in the current implementation) without any way to shrink back in other words.

My understanding was that the primary use case for the swap limit is to handle potential (not only malicious but also unexpectedly misbehaving application) anon memory consumption runaways more gracefully without the massive disruption on the global level. I simply didn't see swap space partitioning as important enough because an alternative to swap usage is to consume primary memory which is a more precious resource IMO. Swap storage is really cheap and runtime expandable resource which is not the case for the primary memory in general. Maybe there are other use cases I am not aware of, though. Do you want to guarantee the swap availability?

Quoted 16 lines · Vladimir Davydov

In the legacy hierarchy we charge memsw, which is dubious, because:

- memsw.limit must be >= memory.limit, so it is impossible to limit swap usage less than memory usage. Taking into account the fact that the primary limiting mechanism in the unified hierarchy is memory.high while memory.limit is either left unset or set to a very large value, moving memsw.limit knob to the unified hierarchy would effectively make it impossible to limit swap usage according to the user preference.

- memsw.usage != memory.usage + swap.usage, because a page occupying both swap entry and a swap cache page is charged only once to memsw counter. As a result, it is possible to effectively eat up to memory.limit of memory pages *and* memsw.limit of swap entries, which looks unexpected.

Quoted 6 lines

That said, we should provide a different swap limiting mechanism for cgroup2.
This patch adds mem_cgroup->swap counter, which charges the actual number of swap entries used by a cgroup. It is only charged in the unified hierarchy, while the legacy hierarchy memsw logic is left intact.

I agree that the previous semantic was awkward. The problem I can see with this approach is that once the swap limit is reached the anon memory pressure might spill over to other and unrelated memcgs during the global memory pressure. I guess this is what Kame referred to as anon would become mlocked basically. This would be even more of an issue with resource delegation to sub-hierarchies because nobody will prevent setting the swap amount to a small value and use that as an anon memory protection.

I guess this was the reason why this approach hasn't been chosen before

but I think we can come up with a way to stop the run away consumption even when the swap is accounted separately. All of them are quite nasty but let me try.

We could allow charges to fail even for the high limit if the excess is way above the amount of reclaimable memory in the given memcg/hierarchy. A runaway load would be stopped before it can cause a considerable damage outside of its hierarchy this way even when the swap limit is configured small.
Now that goes against the high limit semantic which should only throttle the consumer and shouldn't cause any functional failures but maybe this is acceptable for the overall system stability. An alternative would be to throttle in the high limit reclaim context proportionally to the excess. This is normally done by the reclaim itself but with no reclaimable memory this wouldn't work that way.

Another option would be to ignore the swap limit during the global reclaim. This wouldn't stop the runaway loads but they would at least see their fair share of the reclaim. The swap excess could be then used as a "handicap" for a more aggressive throttling during high limit reclaim or to trigger hard limit sooner.

Or we could teach the global OOM killer to select abusive anon memory users with restricted swap. That would require to iterate through all memcgs and checks whether their anon consumption is in a large excess to their swap limit and fallback to the memcg OOM victim selection if that is the case. This adds more complexity to the OOM killer path so I am not sure this is generally acceptable, though.

My question now is. Is the knob usable/useful even without additional heuristics? Do we want to protect swap space so rigidly that a swap limited memcg can cause bigger problems than without the swap limit globally?

Quoted 11 lines

The swap usage can be monitored using new memory.swap.current file and limited using memory.swap.max.

Signed-off-by Vladimir Davydov <[email protected]>

---
  include/linux/memcontrol.h |   1 +
  include/linux/swap.h       |   5 ++
  mm/memcontrol.c            | 123 +++++++++++++++++++++++++++++++++++++++++----
  mm/shmem.c                 |   4 ++
  mm/swap_state.c            |   5 ++
  5 files changed, 129 insertions(+), 9 deletions(-)

[...]

Quoted 33 lines · Michal Hocko

Quoted 16 lines · Vladimir Davydov

In the legacy hierarchy we charge memsw, which is dubious, because:

- memsw.limit must be >= memory.limit, so it is impossible to limit swap usage less than memory usage. Taking into account the fact that the primary limiting mechanism in the unified hierarchy is memory.high while memory.limit is either left unset or set to a very large value, moving memsw.limit knob to the unified hierarchy would effectively make it impossible to limit swap usage according to the user preference.

- memsw.usage != memory.usage + swap.usage, because a page occupying both swap entry and a swap cache page is charged only once to memsw counter. As a result, it is possible to effectively eat up to memory.limit of memory pages *and* memsw.limit of swap entries, which looks unexpected.

Quoted 6 lines

That said, we should provide a different swap limiting mechanism for cgroup2.
This patch adds mem_cgroup->swap counter, which charges the actual number of swap entries used by a cgroup. It is only charged in the unified hierarchy, while the legacy hierarchy memsw logic is left intact.

I agree that the previous semantic was awkward. The problem I can see with this approach is that once the swap limit is reached the anon memory pressure might spill over to other and unrelated memcgs during the global memory pressure. I guess this is what Kame referred to as anon would become mlocked basically. This would be even more of an issue with resource delegation to sub-hierarchies because nobody will prevent setting the swap amount to a small value and use that as an anon memory protection.

Quoted 1 line

I guess this was the reason why this approach hasn't been chosen before

Yes. At that age, "never break global VM" was the policy. And "mlock" can be used for attacking system.

Quoted 36 lines · Kamezawa Hiroyuki

Quoted 34 lines · Michal Hocko

Quoted 16 lines · Vladimir Davydov

In the legacy hierarchy we charge memsw, which is dubious, because:

- memsw.limit must be >= memory.limit, so it is impossible to limit swap usage less than memory usage. Taking into account the fact that the primary limiting mechanism in the unified hierarchy is memory.high while memory.limit is either left unset or set to a very large value, moving memsw.limit knob to the unified hierarchy would effectively make it impossible to limit swap usage according to the user preference.

- memsw.usage != memory.usage + swap.usage, because a page occupying both swap entry and a swap cache page is charged only once to memsw counter. As a result, it is possible to effectively eat up to memory.limit of memory pages *and* memsw.limit of swap entries, which looks unexpected.

Quoted 6 lines

That said, we should provide a different swap limiting mechanism for cgroup2.
This patch adds mem_cgroup->swap counter, which charges the actual number of swap entries used by a cgroup. It is only charged in the unified hierarchy, while the legacy hierarchy memsw logic is left intact.

I agree that the previous semantic was awkward. The problem I can see with this approach is that once the swap limit is reached the anon memory pressure might spill over to other and unrelated memcgs during the global memory pressure. I guess this is what Kame referred to as anon would become mlocked basically. This would be even more of an issue with resource delegation to sub-hierarchies because nobody will prevent setting the swap amount to a small value and use that as an anon memory protection.

I guess this was the reason why this approach hasn't been chosen before

Quoted 2 lines

Yes. At that age, "never break global VM" was the policy. And "mlock" can be used for attacking system.

If we are talking about "attacking system" from inside a container, there are much easier and disruptive ways, e.g. running a fork-bomb or creating pipes - such memory can't be reclaimed and global OOM killer won't help.

memcg will come in handy in get_scan_count(). It can already be used for getting swappiness immediately in get_scan_count() instead of passing it around. The following patches will add more memcg-related values, which will be used there.

Signed-off-by Vladimir Davydov <[email protected]>

mem_cgroup_lruvec_online() takes lruvec, but it only needs memcg. Since get_scan_count(), which is the only user of this function, now possesses pointer to memcg, let's pass memcg directly to mem_cgroup_online() instead of picking it out of lruvec and rename the function accordingly.

Signed-off-by Vladimir Davydov <[email protected]>

The following patches will add more functions to the memcg section of include/linux/swap.h. Some of them will need values defined below the current location of the section. So let's move the section to the end of the file. No functional changes intended.

Signed-off-by Vladimir Davydov <[email protected]>

We don't scan anonymous memory if we ran out of swap, neither should we do it in case memcg swap limit is hit, because swap out is impossible anyway.

Signed-off-by Vladimir Davydov <[email protected]>

Swap cache pages are freed aggressively if swap is nearly full (>50% currently), because otherwise we are likely to stop scanning anonymous when we near the swap limit even if there is plenty of freeable swap cache pages. We should follow the same trend in case of memory cgroup, which has its own swap limit.

Signed-off-by Vladimir Davydov <[email protected]>

mm/memcontrol.c +0 -0

@@ -5839,6 +5839,29 @@ long mem_cgroup_get_nr_swap_pages(struct mem_cgroup *memcg)
 	return nr_swap_pages;
 }
 

Quoted 7 lines · Vladimir Davydov

Swap cache pages are freed aggressively if swap is nearly full (>50% currently), because otherwise we are likely to stop scanning anonymous when we near the swap limit even if there is plenty of freeable swap cache pages. We should follow the same trend in case of memory cgroup, which has its own swap limit.

Signed-off-by Vladimir Davydov <[email protected]>

Acked-by Johannes Weiner <[email protected]>

One note:

Quoted 4 lines

mm/memcontrol.c +0 -0

@@ -5839,6 +5839,29 @@ long mem_cgroup_get_nr_swap_pages(struct mem_cgroup *memcg)
 	return nr_swap_pages;
 }
 

mm/memcontrol.c long mem_cgroup_get_nr_swap_pages(struct mem_cgroup *memcg) · line 5851

Quoted 10 lines

mm/memcontrol.c +10 -0

+bool mem_cgroup_swap_full(struct page *page)
+{
+	struct mem_cgroup *memcg;
+
+	VM_BUG_ON_PAGE(!PageLocked(page), page);
+
+	if (vm_swap_full())
+		return true;
+	if (!do_swap_account || !PageSwapCache(page))
+		return false;

The callers establish PageSwapCache() under the page lock, which makes sense since they only inquire about the swap state when deciding what to do with a swapcache page at hand. So this check seems unnecessary.

Signed-off-by Vladimir Davydov <[email protected]>

Quoted 1 line · Vladimir Davydov

Signed-off-by Vladimir Davydov <[email protected]>

Acked-by Johannes Weiner <[email protected]>

Can we include a blurb for R-5-1 of cgroups.txt as well to explain why cgroup2 has a new swap interface? I had already written something up in the past, pasted below, feel free to use it if you like. Otherwise, you had pretty good reasons in your swap controller changelog as well.