Groups | Search | Server Info | Keyboard shortcuts | Login | Register [http] [https] [nntp] [nntps]

Groups > linux.kernel > #1288457 > unrolled thread

[PATCH 0/7] Add swap accounting to cgroup2

Back to article view | Back to linux.kernel

Contents

  [PATCH 0/7] Add swap accounting to cgroup2 Vladimir Davydov <vdavydov@virtuozzo.com> - 2015-12-10 12:40 +0100
    [PATCH 4/7] swap.h: move memcg related stuff to the end of the file Vladimir Davydov <vdavydov@virtuozzo.com> - 2015-12-10 12:40 +0100
      Re: [PATCH 4/7] swap.h: move memcg related stuff to the end of the  file Johannes Weiner <hannes@cmpxchg.org> - 2015-12-11 20:30 +0100
    [PATCH 3/7] mm: memcontrol: replace mem_cgroup_lruvec_online with mem_cgroup_online Vladimir Davydov <vdavydov@virtuozzo.com> - 2015-12-10 12:40 +0100
      Re: [PATCH 3/7] mm: memcontrol: replace mem_cgroup_lruvec_online  with mem_cgroup_online Johannes Weiner <hannes@cmpxchg.org> - 2015-12-11 20:30 +0100
    [PATCH 2/7] mm: vmscan: pass memcg to get_scan_count() Vladimir Davydov <vdavydov@virtuozzo.com> - 2015-12-10 12:40 +0100
      Re: [PATCH 2/7] mm: vmscan: pass memcg to get_scan_count() Johannes Weiner <hannes@cmpxchg.org> - 2015-12-11 20:30 +0100
    [PATCH 1/7] mm: memcontrol: charge swap to cgroup2 Vladimir Davydov <vdavydov@virtuozzo.com> - 2015-12-10 12:40 +0100
      Re: [PATCH 1/7] mm: memcontrol: charge swap to cgroup2 Johannes Weiner <hannes@cmpxchg.org> - 2015-12-10 17:10 +0100
        Re: [PATCH 1/7] mm: memcontrol: charge swap to cgroup2 Vladimir Davydov <vdavydov@virtuozzo.com> - 2015-12-10 18:10 +0100
      Re: [PATCH 1/7] mm: memcontrol: charge swap to cgroup2 Kamezawa Hiroyuki <kamezawa.hiroyu@jp.fujitsu.com> - 2015-12-11 04:00 +0100
        Re: [PATCH 1/7] mm: memcontrol: charge swap to cgroup2 Vladimir Davydov <vdavydov@virtuozzo.com> - 2015-12-11 08:50 +0100
      Re: [PATCH 1/7] mm: memcontrol: charge swap to cgroup2 Michal Hocko <mhocko@kernel.org> - 2015-12-14 16:40 +0100
        Re: [PATCH 1/7] mm: memcontrol: charge swap to cgroup2 Johannes Weiner <hannes@cmpxchg.org> - 2015-12-14 16:50 +0100
        Re: [PATCH 1/7] mm: memcontrol: charge swap to cgroup2 Vladimir Davydov <vdavydov@virtuozzo.com> - 2015-12-14 20:50 +0100
          Re: [PATCH 1/7] mm: memcontrol: charge swap to cgroup2 One Thousand Gnomes <gnomes@lxorguk.ukuu.org.uk> - 2015-12-14 21:00 +0100
          Re: [PATCH 1/7] mm: memcontrol: charge swap to cgroup2 Kamezawa Hiroyuki <kamezawa.hiroyu@jp.fujitsu.com> - 2015-12-15 04:30 +0100
            Re: [PATCH 1/7] mm: memcontrol: charge swap to cgroup2 Vladimir Davydov <vdavydov@virtuozzo.com> - 2015-12-15 12:10 +0100
              Re: [PATCH 1/7] mm: memcontrol: charge swap to cgroup2 Kamezawa Hiroyuki <kamezawa.hiroyu@jp.fujitsu.com> - 2015-12-16 03:50 +0100
            Re: [PATCH 1/7] mm: memcontrol: charge swap to cgroup2 Johannes Weiner <hannes@cmpxchg.org> - 2015-12-15 16:00 +0100
              Re: [PATCH 1/7] mm: memcontrol: charge swap to cgroup2 Kamezawa Hiroyuki <kamezawa.hiroyu@jp.fujitsu.com> - 2015-12-16 04:20 +0100
                Re: [PATCH 1/7] mm: memcontrol: charge swap to cgroup2 Johannes Weiner <hannes@cmpxchg.org> - 2015-12-16 12:10 +0100
                  Re: [PATCH 1/7] mm: memcontrol: charge swap to cgroup2 Kamezawa Hiroyuki <kamezawa.hiroyu@jp.fujitsu.com> - 2015-12-17 03:50 +0100
                    Re: [PATCH 1/7] mm: memcontrol: charge swap to cgroup2 Johannes Weiner <hannes@cmpxchg.org> - 2015-12-17 04:40 +0100
                      Re: [PATCH 1/7] mm: memcontrol: charge swap to cgroup2 Kamezawa Hiroyuki <kamezawa.hiroyu@jp.fujitsu.com> - 2015-12-17 05:40 +0100
          Re: [PATCH 1/7] mm: memcontrol: charge swap to cgroup2 Michal Hocko <mhocko@kernel.org> - 2015-12-15 18:30 +0100
            Re: [PATCH 1/7] mm: memcontrol: charge swap to cgroup2 Johannes Weiner <hannes@cmpxchg.org> - 2015-12-15 21:30 +0100
            Re: [PATCH 1/7] mm: memcontrol: charge swap to cgroup2 Kamezawa Hiroyuki <kamezawa.hiroyu@jp.fujitsu.com> - 2015-12-16 05:00 +0100
        Re: [PATCH 1/7] mm: memcontrol: charge swap to cgroup2 Kamezawa Hiroyuki <kamezawa.hiroyu@jp.fujitsu.com> - 2015-12-15 04:30 +0100
          Re: [PATCH 1/7] mm: memcontrol: charge swap to cgroup2 Vladimir Davydov <vdavydov@virtuozzo.com> - 2015-12-15 09:40 +0100
            Re: [PATCH 1/7] mm: memcontrol: charge swap to cgroup2 Kamezawa Hiroyuki <kamezawa.hiroyu@jp.fujitsu.com> - 2015-12-15 10:40 +0100
    [PATCH 7/7] Documentation: cgroup: add memory.swap.{current,max} description Vladimir Davydov <vdavydov@virtuozzo.com> - 2015-12-10 12:50 +0100
      Re: [PATCH 7/7] Documentation: cgroup: add memory.swap.{current,max}  description Johannes Weiner <hannes@cmpxchg.org> - 2015-12-11 20:50 +0100
        Re: [PATCH 7/7] Documentation: cgroup: add memory.swap.{current,max}  description Vladimir Davydov <vdavydov@virtuozzo.com> - 2015-12-12 17:20 +0100
    [PATCH 6/7] mm: free swap cache aggressively if memcg swap is full Vladimir Davydov <vdavydov@virtuozzo.com> - 2015-12-10 12:50 +0100
      Re: [PATCH 6/7] mm: free swap cache aggressively if memcg swap is  full Johannes Weiner <hannes@cmpxchg.org> - 2015-12-11 20:40 +0100
        Re: [PATCH 6/7] mm: free swap cache aggressively if memcg swap is  full Vladimir Davydov <vdavydov@virtuozzo.com> - 2015-12-12 17:20 +0100

Page 1 of 2  [1] 2  Next page →

#1288457 — [PATCH 0/7] Add swap accounting to cgroup2

Hi,

This patch set adds swap accounting to cgroup2. In contrast to the
legacy hierarchy, actual swap usage is accounted. It can be controlled
and monitored using new files, memory.swap.current and memory.swap.max.
For more details, please see patch 1 of the series, which introduces the
new counter. Patches 2-6 make memcg reclaim follow the heuristics used
on global reclaim for handling anon/swap. Patch 7 updates documentation.

Thanks,

Vladimir Davydov (7):
  mm: memcontrol: charge swap to cgroup2
  mm: vmscan: pass memcg to get_scan_count()
  mm: memcontrol: replace mem_cgroup_lruvec_online with
    mem_cgroup_online
  swap.h: move memcg related stuff to the end of the file
  mm: vmscan: do not scan anon pages if memcg swap limit is hit
  mm: free swap cache aggressively if memcg swap is full
  Documentation: cgroup: add memory.swap.{current,max} description

 Documentation/cgroup.txt   |  16 +++++
 include/linux/memcontrol.h |  28 ++++----
 include/linux/swap.h       |  75 +++++++++++++--------
 mm/memcontrol.c            | 159 ++++++++++++++++++++++++++++++++++++++++++---
 mm/memory.c                |   3 +-
 mm/shmem.c                 |   4 ++
 mm/swap_state.c            |   5 ++
 mm/swapfile.c              |   2 +-
 mm/vmscan.c                |  26 ++++----
 9 files changed, 249 insertions(+), 69 deletions(-)

-- 
2.1.4

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

[toc] | [next] | [standalone]

#1288458 — [PATCH 4/7] swap.h: move memcg related stuff to the end of the file

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 <vdavydov@virtuozzo.com>
---
 include/linux/swap.h | 68 ++++++++++++++++++++++++++++------------------------
 1 file changed, 37 insertions(+), 31 deletions(-)

diff --git a/include/linux/swap.h b/include/linux/swap.h
index f4b3ccdcba91..66ea62cf256d 100644
--- a/include/linux/swap.h
+++ b/include/linux/swap.h
@@ -350,38 +350,7 @@ extern void check_move_unevictable_pages(struct page **, int nr_pages);
 
 extern int kswapd_run(int nid);
 extern void kswapd_stop(int nid);
-#ifdef CONFIG_MEMCG
-static inline int mem_cgroup_swappiness(struct mem_cgroup *memcg)
-{
-	/* root ? */
-	if (mem_cgroup_disabled() || !memcg->css.parent)
-		return vm_swappiness;
-
-	return memcg->swappiness;
-}
 
-#else
-static inline int mem_cgroup_swappiness(struct mem_cgroup *mem)
-{
-	return vm_swappiness;
-}
-#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)
-{
-}
-#endif
 #ifdef CONFIG_SWAP
 /* linux/mm/page_io.c */
 extern int swap_readpage(struct page *);
@@ -560,5 +529,42 @@ static inline swp_entry_t get_swap_page(void)
 }
 
 #endif /* CONFIG_SWAP */
+
+#ifdef CONFIG_MEMCG
+static inline int mem_cgroup_swappiness(struct mem_cgroup *memcg)
+{
+	/* root ? */
+	if (mem_cgroup_disabled() || !memcg->css.parent)
+		return vm_swappiness;
+
+	return memcg->swappiness;
+}
+
+#else
+static inline int mem_cgroup_swappiness(struct mem_cgroup *mem)
+{
+	return vm_swappiness;
+}
+#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)
+{
+}
+#endif
+
 #endif /* __KERNEL__*/
 #endif /* _LINUX_SWAP_H */
-- 
2.1.4

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

[toc] | [prev] | [next] | [standalone]

#1289843 — Re: [PATCH 4/7] swap.h: move memcg related stuff to the end of the file

On Thu, Dec 10, 2015 at 02:39:17PM +0300, Vladimir Davydov wrote:
> 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 <vdavydov@virtuozzo.com>

Acked-by: Johannes Weiner <hannes@cmpxchg.org>
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[toc] | [prev] | [next] | [standalone]

#1288459 — [PATCH 3/7] mm: memcontrol: replace mem_cgroup_lruvec_online with mem_cgroup_online

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 <vdavydov@virtuozzo.com>
---
 include/linux/memcontrol.h | 27 ++++++++++-----------------
 mm/vmscan.c                |  2 +-
 2 files changed, 11 insertions(+), 18 deletions(-)

diff --git a/include/linux/memcontrol.h b/include/linux/memcontrol.h
index 993c9a26b637..c9a14e1eab62 100644
--- a/include/linux/memcontrol.h
+++ b/include/linux/memcontrol.h
@@ -361,6 +361,13 @@ static inline bool mem_cgroup_disabled(void)
 	return !cgroup_subsys_enabled(memory_cgrp_subsys);
 }
 
+static inline bool mem_cgroup_online(struct mem_cgroup *memcg)
+{
+	if (mem_cgroup_disabled())
+		return true;
+	return !!(memcg->css.flags & CSS_ONLINE);
+}
+
 /*
  * For memory reclaim.
  */
@@ -369,20 +376,6 @@ int mem_cgroup_select_victim_node(struct mem_cgroup *memcg);
 void mem_cgroup_update_lru_size(struct lruvec *lruvec, enum lru_list lru,
 		int nr_pages);
 
-static inline bool mem_cgroup_lruvec_online(struct lruvec *lruvec)
-{
-	struct mem_cgroup_per_zone *mz;
-	struct mem_cgroup *memcg;
-
-	if (mem_cgroup_disabled())
-		return true;
-
-	mz = container_of(lruvec, struct mem_cgroup_per_zone, lruvec);
-	memcg = mz->memcg;
-
-	return !!(memcg->css.flags & CSS_ONLINE);
-}
-
 static inline
 unsigned long mem_cgroup_get_lru_size(struct lruvec *lruvec, enum lru_list lru)
 {
@@ -706,13 +699,13 @@ static inline bool mem_cgroup_disabled(void)
 	return true;
 }
 
-static inline bool
-mem_cgroup_inactive_anon_is_low(struct lruvec *lruvec)
+static inline bool mem_cgroup_online(struct mem_cgroup *memcg)
 {
 	return true;
 }
 
-static inline bool mem_cgroup_lruvec_online(struct lruvec *lruvec)
+static inline bool
+mem_cgroup_inactive_anon_is_low(struct lruvec *lruvec)
 {
 	return true;
 }
diff --git a/mm/vmscan.c b/mm/vmscan.c
index acc6bff84e26..b220e6cda25d 100644
--- a/mm/vmscan.c
+++ b/mm/vmscan.c
@@ -1988,7 +1988,7 @@ static void get_scan_count(struct lruvec *lruvec, struct mem_cgroup *memcg,
 	if (current_is_kswapd()) {
 		if (!zone_reclaimable(zone))
 			force_scan = true;
-		if (!mem_cgroup_lruvec_online(lruvec))
+		if (!mem_cgroup_online(memcg))
 			force_scan = true;
 	}
 	if (!global_reclaim(sc))
-- 
2.1.4

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

[toc] | [prev] | [next] | [standalone]

#1289851 — Re: [PATCH 3/7] mm: memcontrol: replace mem_cgroup_lruvec_online with mem_cgroup_online

On Thu, Dec 10, 2015 at 02:39:16PM +0300, Vladimir Davydov wrote:
> 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 <vdavydov@virtuozzo.com>

Acked-by: Johannes Weiner <hannes@cmpxchg.org>
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[toc] | [prev] | [next] | [standalone]

#1288460 — [PATCH 2/7] mm: vmscan: pass memcg to get_scan_count()

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 <vdavydov@virtuozzo.com>
---
 mm/vmscan.c | 20 ++++++++------------
 1 file changed, 8 insertions(+), 12 deletions(-)

diff --git a/mm/vmscan.c b/mm/vmscan.c
index bb01b04154ad..acc6bff84e26 100644
--- a/mm/vmscan.c
+++ b/mm/vmscan.c
@@ -1957,10 +1957,11 @@ enum scan_balance {
  * nr[0] = anon inactive pages to scan; nr[1] = anon active pages to scan
  * nr[2] = file inactive pages to scan; nr[3] = file active pages to scan
  */
-static void get_scan_count(struct lruvec *lruvec, int swappiness,
+static void get_scan_count(struct lruvec *lruvec, struct mem_cgroup *memcg,
 			   struct scan_control *sc, unsigned long *nr,
 			   unsigned long *lru_pages)
 {
+	int swappiness = mem_cgroup_swappiness(memcg);
 	struct zone_reclaim_stat *reclaim_stat = &lruvec->reclaim_stat;
 	u64 fraction[2];
 	u64 denominator = 0;	/* gcc */
@@ -2184,9 +2185,10 @@ static inline void init_tlb_ubc(void)
 /*
  * This is a basic per-zone page freer.  Used by both kswapd and direct reclaim.
  */
-static void shrink_lruvec(struct lruvec *lruvec, int swappiness,
-			  struct scan_control *sc, unsigned long *lru_pages)
+static void shrink_zone_memcg(struct zone *zone, struct mem_cgroup *memcg,
+			      struct scan_control *sc, unsigned long *lru_pages)
 {
+	struct lruvec *lruvec = mem_cgroup_zone_lruvec(zone, memcg);
 	unsigned long nr[NR_LRU_LISTS];
 	unsigned long targets[NR_LRU_LISTS];
 	unsigned long nr_to_scan;
@@ -2196,7 +2198,7 @@ static void shrink_lruvec(struct lruvec *lruvec, int swappiness,
 	struct blk_plug plug;
 	bool scan_adjusted;
 
-	get_scan_count(lruvec, swappiness, sc, nr, lru_pages);
+	get_scan_count(lruvec, memcg, sc, nr, lru_pages);
 
 	/* Record the original scan target for proportional adjustments later */
 	memcpy(targets, nr, sizeof(nr));
@@ -2400,8 +2402,6 @@ static bool shrink_zone(struct zone *zone, struct scan_control *sc,
 			unsigned long lru_pages;
 			unsigned long reclaimed;
 			unsigned long scanned;
-			struct lruvec *lruvec;
-			int swappiness;
 
 			if (mem_cgroup_low(root, memcg)) {
 				if (!sc->may_thrash)
@@ -2409,12 +2409,10 @@ static bool shrink_zone(struct zone *zone, struct scan_control *sc,
 				mem_cgroup_events(memcg, MEMCG_LOW, 1);
 			}
 
-			lruvec = mem_cgroup_zone_lruvec(zone, memcg);
-			swappiness = mem_cgroup_swappiness(memcg);
 			reclaimed = sc->nr_reclaimed;
 			scanned = sc->nr_scanned;
 
-			shrink_lruvec(lruvec, swappiness, sc, &lru_pages);
+			shrink_zone_memcg(zone, memcg, sc, &lru_pages);
 			zone_lru_pages += lru_pages;
 
 			if (memcg && is_classzone)
@@ -2884,8 +2882,6 @@ unsigned long mem_cgroup_shrink_node_zone(struct mem_cgroup *memcg,
 		.may_unmap = 1,
 		.may_swap = !noswap,
 	};
-	struct lruvec *lruvec = mem_cgroup_zone_lruvec(zone, memcg);
-	int swappiness = mem_cgroup_swappiness(memcg);
 	unsigned long lru_pages;
 
 	sc.gfp_mask = (gfp_mask & GFP_RECLAIM_MASK) |
@@ -2902,7 +2898,7 @@ unsigned long mem_cgroup_shrink_node_zone(struct mem_cgroup *memcg,
 	 * will pick up pages from other mem cgroup's as well. We hack
 	 * the priority and make it zero.
 	 */
-	shrink_lruvec(lruvec, swappiness, &sc, &lru_pages);
+	shrink_zone_memcg(zone, memcg, &sc, &lru_pages);
 
 	trace_mm_vmscan_memcg_softlimit_reclaim_end(sc.nr_reclaimed);
 
-- 
2.1.4

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

[toc] | [prev] | [next] | [standalone]

#1289848 — Re: [PATCH 2/7] mm: vmscan: pass memcg to get_scan_count()

On Thu, Dec 10, 2015 at 02:39:15PM +0300, Vladimir Davydov wrote:
> 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 <vdavydov@virtuozzo.com>

Acked-by: Johannes Weiner <hannes@cmpxchg.org>
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[toc] | [prev] | [next] | [standalone]

#1288463 — [PATCH 1/7] mm: memcontrol: charge swap 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 <vdavydov@virtuozzo.com>
---
 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(-)

diff --git a/include/linux/memcontrol.h b/include/linux/memcontrol.h
index c6a5ed2f2744..993c9a26b637 100644
--- a/include/linux/memcontrol.h
+++ b/include/linux/memcontrol.h
@@ -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;
 
diff --git a/include/linux/swap.h b/include/linux/swap.h
index 457181844b6e..f4b3ccdcba91 100644
--- a/include/linux/swap.h
+++ b/include/linux/swap.h
@@ -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)
 {
 }
diff --git a/mm/memcontrol.c b/mm/memcontrol.c
index 7f5c6abf5421..9d10e2819ec4 100644
--- a/mm/memcontrol.c
+++ b/mm/memcontrol.c
@@ -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));
 	}
diff --git a/mm/shmem.c b/mm/shmem.c
index 9b051115a100..659a90d8305c 100644
--- a/mm/shmem.c
+++ b/mm/shmem.c
@@ -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);
diff --git a/mm/swap_state.c b/mm/swap_state.c
index d783872d746c..dea39cb03967 100644
--- a/mm/swap_state.c
+++ b/mm/swap_state.c
@@ -170,6 +170,11 @@ int add_to_swap(struct page *page, struct list_head *list)
 	if (!entry.val)
 		return 0;
 
+	if (mem_cgroup_charge_swap(page, entry)) {
+		swapcache_free(entry);
+		return 0;
+	}
+
 	if (unlikely(PageTransHuge(page)))
 		if (unlikely(split_huge_page_to_list(page, list))) {
 			swapcache_free(entry);
-- 
2.1.4

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

[toc] | [prev] | [next] | [standalone]

#1288628 — Re: [PATCH 1/7] mm: memcontrol: charge swap to cgroup2

On Thu, Dec 10, 2015 at 02:39:14PM +0300, Vladimir Davydov wrote:
> 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 <vdavydov@virtuozzo.com>

This looks great!

Acked-by: Johannes Weiner <hannes@cmpxchg.org>

I have a few questions, but none of them show-stoppers:

> ---
>  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(-)
> 
> diff --git a/include/linux/memcontrol.h b/include/linux/memcontrol.h
> index c6a5ed2f2744..993c9a26b637 100644
> --- a/include/linux/memcontrol.h
> +++ b/include/linux/memcontrol.h
> @@ -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;

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.

> diff --git a/include/linux/swap.h b/include/linux/swap.h
> index 457181844b6e..f4b3ccdcba91 100644
> --- a/include/linux/swap.h
> +++ b/include/linux/swap.h
> @@ -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.

> @@ -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;

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

> @@ -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);

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.

> @@ -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.
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[toc] | [prev] | [next] | [standalone]

#1288682 — Re: [PATCH 1/7] mm: memcontrol: charge swap to cgroup2

On Thu, Dec 10, 2015 at 11:00:27AM -0500, Johannes Weiner wrote:
> On Thu, Dec 10, 2015 at 02:39:14PM +0300, Vladimir Davydov wrote:
...
> > diff --git a/include/linux/memcontrol.h b/include/linux/memcontrol.h
> > index c6a5ed2f2744..993c9a26b637 100644
> > --- a/include/linux/memcontrol.h
> > +++ b/include/linux/memcontrol.h
> > @@ -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;
> 
> 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.

Yeah, but we'd better do it in a separate patch.

> 
> > diff --git a/include/linux/swap.h b/include/linux/swap.h
> > index 457181844b6e..f4b3ccdcba91 100644
> > --- a/include/linux/swap.h
> > +++ b/include/linux/swap.h
> > @@ -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.

Hmm, I thought we only use try_charge name in memcontrol.c if there is
commit stage, e.g. we have memcg_kmem_charge, not memcg_kmem_try_charge.
This conflicts with page_counter semantics though, so we might want to
rename it.

> 
> > @@ -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;
> 
> This is taking a racy snapshot, so we don't rely on 100% accuracy. Can
> we do without the READ_ONCE()?

Well, I suppose we can, but passing a volatile value to min macro looks
a bit scary to me. What if swap_limit is changed from a finite value
less than total_swap_pages to inf while we are there? We might use an
infinite memory size in OOM which would screw up OOM scores AFAIU.
Unlikely, but still.

> 
> > @@ -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);
> 
> 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.

Well, may be. I'm afraid this might make mem_cgroup_charge_swap look a
bit messy due to necessity to check cgroup_subsys_on_dfl in the middle
of it, but I'll give it a try.

> 
> > @@ -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.
> 

Yeah, that would be cleaner. I wonder if we could drop swapaccount boot
param then so as not to clutter the API.

Thanks for the review!
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[toc] | [prev] | [next] | [standalone]

#1289073 — Re: [PATCH 1/7] mm: memcontrol: charge swap to cgroup2

On 2015/12/10 20:39, Vladimir Davydov wrote:
> 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 <vdavydov@virtuozzo.com>

setting swap.max=0 will work like mlock ?

Thanks,
-Kame


> ---
>   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(-)
> 
> diff --git a/include/linux/memcontrol.h b/include/linux/memcontrol.h
> index c6a5ed2f2744..993c9a26b637 100644
> --- a/include/linux/memcontrol.h
> +++ b/include/linux/memcontrol.h
> @@ -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;
>   
> diff --git a/include/linux/swap.h b/include/linux/swap.h
> index 457181844b6e..f4b3ccdcba91 100644
> --- a/include/linux/swap.h
> +++ b/include/linux/swap.h
> @@ -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)
>   {
>   }
> diff --git a/mm/memcontrol.c b/mm/memcontrol.c
> index 7f5c6abf5421..9d10e2819ec4 100644
> --- a/mm/memcontrol.c
> +++ b/mm/memcontrol.c
> @@ -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));
>   	}
> diff --git a/mm/shmem.c b/mm/shmem.c
> index 9b051115a100..659a90d8305c 100644
> --- a/mm/shmem.c
> +++ b/mm/shmem.c
> @@ -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);
> diff --git a/mm/swap_state.c b/mm/swap_state.c
> index d783872d746c..dea39cb03967 100644
> --- a/mm/swap_state.c
> +++ b/mm/swap_state.c
> @@ -170,6 +170,11 @@ int add_to_swap(struct page *page, struct list_head *list)
>   	if (!entry.val)
>   		return 0;
>   
> +	if (mem_cgroup_charge_swap(page, entry)) {
> +		swapcache_free(entry);
> +		return 0;
> +	}
> +
>   	if (unlikely(PageTransHuge(page)))
>   		if (unlikely(split_huge_page_to_list(page, list))) {
>   			swapcache_free(entry);
> 


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

[toc] | [prev] | [next] | [standalone]

#1289278 — Re: [PATCH 1/7] mm: memcontrol: charge swap to cgroup2

On Fri, Dec 11, 2015 at 11:48:57AM +0900, Kamezawa Hiroyuki wrote:
> On 2015/12/10 20:39, Vladimir Davydov wrote:
> > 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 <vdavydov@virtuozzo.com>
> 
> setting swap.max=0 will work like mlock ?

For anonymous memory - yes.

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

[toc] | [prev] | [next] | [standalone]

#1291285 — Re: [PATCH 1/7] mm: memcontrol: charge swap to cgroup2

On Thu 10-12-15 14:39:14, Vladimir Davydov wrote:
> 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.

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?

> The swap usage can be monitored using new memory.swap.current file and
> limited using memory.swap.max.
> 
> Signed-off-by: Vladimir Davydov <vdavydov@virtuozzo.com>
> ---
>  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(-)

[...]
-- 
Michal Hocko
SUSE Labs
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[toc] | [prev] | [next] | [standalone]

#1291297 — Re: [PATCH 1/7] mm: memcontrol: charge swap to cgroup2

On Mon, Dec 14, 2015 at 04:30:37PM +0100, Michal Hocko wrote:
> On Thu 10-12-15 14:39:14, Vladimir Davydov wrote:
> > 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.
> 
> 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.

Overcommitting untrusted workloads is already problematic because
reclaim is based on heuristics and references, and a malicious
workload can already interfere with it and create pressure on the
system or its neighboring groups. This patch doesn't make it better,
but it's not a new problem.

If you don't trust subhierarchies, don't give them more memory than
you can handle them taking. And then giving them swap is a resource
for them to use on top of that memory, not for you at the toplevel.
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[toc] | [prev] | [next] | [standalone]

#1291511 — Re: [PATCH 1/7] mm: memcontrol: charge swap to cgroup2

On Mon, Dec 14, 2015 at 04:30:37PM +0100, Michal Hocko wrote:
> On Thu 10-12-15 14:39:14, Vladimir Davydov wrote:
> > 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.
> 
> 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.

...
> 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.

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

[toc] | [prev] | [next] | [standalone]

#1291513 — Re: [PATCH 1/7] mm: memcontrol: charge swap to cgroup2

> 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.

If you don't trust it put it in a VM. If it's got access to GEM graphics
ioctls/nodes or some other kernel interfaces then it can blow up the
kernel without trying hard unless its constrained within a VM. VMs can
be extremely light weight if you avoid KVM emulating an entire PC.

Alan
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[toc] | [prev] | [next] | [standalone]

#1291813 — Re: [PATCH 1/7] mm: memcontrol: charge swap to cgroup2

On 2015/12/15 4:42, Vladimir Davydov wrote:
> On Mon, Dec 14, 2015 at 04:30:37PM +0100, Michal Hocko wrote:
>> On Thu 10-12-15 14:39:14, Vladimir Davydov wrote:
>>> 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.
>>
>> 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.
>
"protect anon" user's malicious degree is far lower than such cracker like users.

> 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 ?

Regards,
-Kame








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

[toc] | [prev] | [next] | [standalone]

#1292044 — Re: [PATCH 1/7] mm: memcontrol: charge swap to cgroup2

On Tue, Dec 15, 2015 at 12:22:41PM +0900, Kamezawa Hiroyuki wrote:
> On 2015/12/15 4:42, Vladimir Davydov wrote:
> >On Mon, Dec 14, 2015 at 04:30:37PM +0100, Michal Hocko wrote:
> >>On Thu 10-12-15 14:39:14, Vladimir Davydov wrote:
> >>>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.
> >>
> >>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.
> >
> "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.

> 
> >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.

> 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 majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[toc] | [prev] | [next] | [standalone]

#1292710 — Re: [PATCH 1/7] mm: memcontrol: charge swap to cgroup2

On 2015/12/15 20:02, Vladimir Davydov wrote:
> On Tue, Dec 15, 2015 at 12:22:41PM +0900, Kamezawa Hiroyuki wrote:
>> On 2015/12/15 4:42, Vladimir Davydov wrote:
>>> On Mon, Dec 14, 2015 at 04:30:37PM +0100, Michal Hocko wrote:
>>>> On Thu 10-12-15 14:39:14, Vladimir Davydov wrote:
>>>>> 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.
>>>>
>>>> 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.
>>>
>> "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.
>

I wanted to say almost all application developers want to set swap.limit=0 if allowed.
So, it's a usual people who can kill the system if swap imbalance is allowed.
  
>>
>>> 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.
>
>> 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.
>

Considering my customers, running OOM-Killer while there are free swap space is
system's error rather than their misconfiguration.

BTW, mlock() requires CAP_IPC_LOCK.
please set default unlimited and check capability at setting swap limit, at least.

Thanks,
-Kame


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


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

[toc] | [prev] | [next] | [standalone]

#1292237 — Re: [PATCH 1/7] mm: memcontrol: charge swap to cgroup2

On Tue, Dec 15, 2015 at 12:22:41PM +0900, Kamezawa Hiroyuki wrote:
> On 2015/12/15 4:42, Vladimir Davydov wrote:
> >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.
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[toc] | [prev] | [next] | [standalone]

Page 1 of 2  [1] 2  Next page →

Back to top | Article view | linux.kernel

csiph-web