From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1754792AbcAMQpN (ORCPT ); Wed, 13 Jan 2016 11:45:13 -0500 Received: from mail-wm0-f65.google.com ([74.125.82.65]:34919 "EHLO mail-wm0-f65.google.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1754152AbcAMQoq (ORCPT ); Wed, 13 Jan 2016 11:44:46 -0500 Date: Wed, 13 Jan 2016 17:44:42 +0100 From: Michal Hocko To: Vladimir Davydov Cc: Andrew Morton , Johannes Weiner , linux-mm@kvack.org, cgroups@vger.kernel.org, linux-kernel@vger.kernel.org Subject: Re: [PATCH v2 1/7] mm: memcontrol: charge swap to cgroup2 Message-ID: <20160113164442.GE17512@dhcp22.suse.cz> References: MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: User-Agent: Mutt/1.5.24 (2015-08-30) Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org [Sorry to come back late - I was too busy after holidays] On Thu 17-12-15 15:29:54, 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. > > Note, to charge swap resource properly in the unified hierarchy, we have > to make swap_entry_free uncharge swap only when ->usage reaches zero, > not just ->count, i.e. when all references to a swap entry, including > the one taken by swap cache, are gone. This is necessary, because > otherwise swap-in could result in uncharging swap even if the page is > still in swap cache and hence still occupies a swap entry. At the same > time, this shouldn't break memsw counter logic, where a page is never > charged twice for using both memory and swap, because in case of legacy > hierarchy we uncharge swap on commit (see mem_cgroup_commit_charge). > > Signed-off-by: Vladimir Davydov OK, let's go with this in the current form but I am still worried that configuring the swap limit properly is really non-trivial, especially when hard limits are not in place. But let's wait with further heuristics after users start complaining. Acked-by: Michal Hocko > --- > Changes in v2: > - Rename mem_cgroup_charge_swap -> mem_cgroup_try_charge_swap to > conform to other memcg charging methods. > - Remove unnecessary READ_ONCE from mem_cgroup_get_limit. > - Uncharge swap only when the last reference is gone, including the one > taken by swap cache. In other words, make swap_entry_free call > mem_cgroup_uncharge_swap only when ->usage reaches zero, not just > ->count. > - Note: I left legacy and default swap charge paths separated for now, > because unifying them (i.e. moving swap_cgroup_record to > mem_cgroup_try_charge_swap) would result in changing behavior of > MEM_CGROUP_STAT_SWAP counter on the legacy hierarchy. > > include/linux/memcontrol.h | 1 + > include/linux/swap.h | 6 +++ > mm/memcontrol.c | 118 ++++++++++++++++++++++++++++++++++++++++++--- > mm/shmem.c | 4 ++ > mm/swap_state.c | 5 ++ > mm/swapfile.c | 4 +- > 6 files changed, 128 insertions(+), 10 deletions(-) > > diff --git a/include/linux/memcontrol.h b/include/linux/memcontrol.h > index 27123e597eca..6e0126230878 100644 > --- a/include/linux/memcontrol.h > +++ b/include/linux/memcontrol.h > @@ -163,6 +163,7 @@ struct mem_cgroup { > > /* Accounted resources */ > struct page_counter memory; > + struct page_counter swap; > > /* Legacy consumer-oriented counters */ > struct page_counter memsw; > diff --git a/include/linux/swap.h b/include/linux/swap.h > index 457181844b6e..478e7dd038c7 100644 > --- a/include/linux/swap.h > +++ b/include/linux/swap.h > @@ -368,11 +368,17 @@ 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_try_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_try_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 9d951faebbd4..567b56da2c23 100644 > --- a/mm/memcontrol.c > +++ b/mm/memcontrol.c > @@ -1222,7 +1222,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))); > @@ -1261,9 +1261,12 @@ static unsigned long mem_cgroup_get_limit(struct mem_cgroup *memcg) > limit = 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); > + swap_limit = memcg->swap.limit; > + swap_limit = min(swap_limit, (unsigned long)total_swap_pages); > + limit = min(limit + swap_limit, memsw_limit); > } > return limit; > } > @@ -4198,11 +4201,13 @@ mem_cgroup_css_alloc(struct cgroup_subsys_state *parent_css) > if (parent && parent->use_hierarchy) { > memcg->use_hierarchy = true; > page_counter_init(&memcg->memory, &parent->memory); > + page_counter_init(&memcg->swap, &parent->swap); > page_counter_init(&memcg->memsw, &parent->memsw); > page_counter_init(&memcg->kmem, &parent->kmem); > page_counter_init(&memcg->tcpmem, &parent->tcpmem); > } else { > page_counter_init(&memcg->memory, NULL); > + page_counter_init(&memcg->swap, NULL); > page_counter_init(&memcg->memsw, NULL); > page_counter_init(&memcg->kmem, NULL); > page_counter_init(&memcg->tcpmem, NULL); > @@ -5212,7 +5217,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); > > @@ -5665,26 +5670,66 @@ void mem_cgroup_swapout(struct page *page, swp_entry_t entry) > memcg_check_events(memcg, page); > } > > +/* > + * mem_cgroup_try_charge_swap - try charging 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_try_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); > } > @@ -5708,6 +5753,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", > @@ -5739,6 +5841,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..95e89b26dac3 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_try_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..934b33879af1 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_try_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); > diff --git a/mm/swapfile.c b/mm/swapfile.c > index 7073faecb38f..efa279221302 100644 > --- a/mm/swapfile.c > +++ b/mm/swapfile.c > @@ -788,14 +788,12 @@ static unsigned char swap_entry_free(struct swap_info_struct *p, > count--; > } > > - if (!count) > - mem_cgroup_uncharge_swap(entry); > - > usage = count | has_cache; > p->swap_map[offset] = usage; > > /* free if no reference */ > if (!usage) { > + mem_cgroup_uncharge_swap(entry); > dec_cluster_info_page(p, p->cluster_info, offset); > if (offset < p->lowest_bit) > p->lowest_bit = offset; > -- > 2.1.4 > > -- > To unsubscribe, send a message with 'unsubscribe linux-mm' in > the body to majordomo@kvack.org. For more info on Linux MM, > see: http://www.linux-mm.org/ . > Don't email: email@kvack.org -- Michal Hocko SUSE Labs