| lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
|
Open Source and information security mailing list archives
| ||
|
Date: Fri, 21 Jan 2011 17:48:18 +0900
From: Daisuke Nishimura <nishimura@....nes.nec.co.jp>
To: KAMEZAWA Hiroyuki <kamezawa.hiroyu@...fujitsu.com>
Cc: "linux-mm@...ck.org" <linux-mm@...ck.org>,
"linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>,
"hannes@...xchg.org" <hannes@...xchg.org>,
"balbir@...ux.vnet.ibm.com" <balbir@...ux.vnet.ibm.com>,
"akpm@...ux-foundation.org" <akpm@...ux-foundation.org>,
Daisuke Nishimura <nishimura@....nes.nec.co.jp>
Subject: Re: [PATCH 4/7] memcg : fix charge function of THP allocation.
On Fri, 21 Jan 2011 15:44:30 +0900
KAMEZAWA Hiroyuki <kamezawa.hiroyu@...fujitsu.com> wrote:
> From: KAMEZAWA Hiroyuki <kamezawa.hiroyu@...fujitsu.com>
>
> When THP is used, Hugepage size charge can happen. It's not handled
> correctly in mem_cgroup_do_charge(). For example, THP can fallback
> to small page allocation when HUGEPAGE allocation seems difficult
> or busy, but memory cgroup doesn't understand it and continue to
> try HUGEPAGE charging. And the worst thing is memory cgroup
> believes 'memory reclaim succeeded' if limit - usage > PAGE_SIZE.
>
> By this, khugepaged etc...can goes into inifinite reclaim loop
> if tasks in memcg are busy.
>
> After this patch
> - Hugepage allocation will fail if 1st trial of page reclaim fails.
> - distinguish THP allocaton from Bached allocation.
>
> Signed-off-by: KAMEZAWA Hiroyuki <kamezawa.hiroyu@...fujitsu.com>
> ---
> mm/memcontrol.c | 51 +++++++++++++++++++++++++++++++++++----------------
> 1 file changed, 35 insertions(+), 16 deletions(-)
>
> Index: mmotm-0107/mm/memcontrol.c
> ===================================================================
> --- mmotm-0107.orig/mm/memcontrol.c
> +++ mmotm-0107/mm/memcontrol.c
> @@ -1812,24 +1812,25 @@ enum {
> CHARGE_OK, /* success */
> CHARGE_RETRY, /* need to retry but retry is not bad */
> CHARGE_NOMEM, /* we can't do more. return -ENOMEM */
> + CHARGE_NEED_BREAK, /* big size allocation failure */
> CHARGE_WOULDBLOCK, /* GFP_WAIT wasn't set and no enough res. */
> CHARGE_OOM_DIE, /* the current is killed because of OOM */
> };
>
> static int __mem_cgroup_do_charge(struct mem_cgroup *mem, gfp_t gfp_mask,
> - int csize, bool oom_check)
> + int page_size, bool do_reclaim, bool oom_check)
I'm sorry, I can't understand why we need 'do_reclaim'. See below.
> {
> struct mem_cgroup *mem_over_limit;
> struct res_counter *fail_res;
> unsigned long flags = 0;
> int ret;
>
> - ret = res_counter_charge(&mem->res, csize, &fail_res);
> + ret = res_counter_charge(&mem->res, page_size, &fail_res);
>
> if (likely(!ret)) {
> if (!do_swap_account)
> return CHARGE_OK;
> - ret = res_counter_charge(&mem->memsw, csize, &fail_res);
> + ret = res_counter_charge(&mem->memsw, page_size, &fail_res);
> if (likely(!ret))
> return CHARGE_OK;
>
> @@ -1838,14 +1839,14 @@ static int __mem_cgroup_do_charge(struct
> } else
> mem_over_limit = mem_cgroup_from_res_counter(fail_res, res);
>
> - if (csize > PAGE_SIZE) /* change csize and retry */
> + if (!do_reclaim)
> return CHARGE_RETRY;
>
>From the very beginning, do we need this "CHARGE_RETRY" ?
> if (!(gfp_mask & __GFP_WAIT))
> return CHARGE_WOULDBLOCK;
>
> ret = mem_cgroup_hierarchical_reclaim(mem_over_limit, NULL,
> - gfp_mask, flags, csize);
> + gfp_mask, flags, page_size);
> /*
> * try_to_free_mem_cgroup_pages() might not give us a full
> * picture of reclaim. Some pages are reclaimed and might be
> @@ -1853,19 +1854,28 @@ static int __mem_cgroup_do_charge(struct
> * Check the limit again to see if the reclaim reduced the
> * current usage of the cgroup before giving up
> */
> - if (ret || mem_cgroup_check_under_limit(mem_over_limit, csize))
> + if (ret || mem_cgroup_check_under_limit(mem_over_limit, page_size))
> return CHARGE_RETRY;
>
> /*
> + * When page_size > PAGE_SIZE, THP calls this function and it's
> + * ok to tell 'there are not enough pages for hugepage'. THP will
> + * fallback into PAGE_SIZE allocation. If we do reclaim eagerly,
> + * page splitting will occur and it seems much worse.
> + */
> + if (page_size > PAGE_SIZE)
> + return CHARGE_NEED_BREAK;
> +
> + /*
> * At task move, charge accounts can be doubly counted. So, it's
> * better to wait until the end of task_move if something is going on.
> */
> if (mem_cgroup_wait_acct_move(mem_over_limit))
> return CHARGE_RETRY;
> -
> /* If we don't need to call oom-killer at el, return immediately */
> if (!oom_check)
> return CHARGE_NOMEM;
> +
> /* check OOM */
> if (!mem_cgroup_handle_oom(mem_over_limit, gfp_mask))
> return CHARGE_OOM_DIE;
> @@ -1885,7 +1895,7 @@ static int __mem_cgroup_try_charge(struc
> int nr_oom_retries = MEM_CGROUP_RECLAIM_RETRIES;
> struct mem_cgroup *mem = NULL;
> int ret;
> - int csize = max(CHARGE_SIZE, (unsigned long) page_size);
> + bool use_pcp_cache = (page_size == PAGE_SIZE);
>
> /*
> * Unlike gloval-vm's OOM-kill, we're not in memory shortage
> @@ -1910,7 +1920,7 @@ again:
> VM_BUG_ON(css_is_removed(&mem->css));
> if (mem_cgroup_is_root(mem))
> goto done;
> - if (page_size == PAGE_SIZE && consume_stock(mem))
> + if (use_pcp_cache && consume_stock(mem))
> goto done;
> css_get(&mem->css);
> } else {
> @@ -1933,7 +1943,7 @@ again:
> rcu_read_unlock();
> goto done;
> }
> - if (page_size == PAGE_SIZE && consume_stock(mem)) {
> + if (use_pcp_cache && consume_stock(mem)) {
> /*
> * It seems dagerous to access memcg without css_get().
> * But considering how consume_stok works, it's not
> @@ -1967,17 +1977,26 @@ again:
> oom_check = true;
> nr_oom_retries = MEM_CGROUP_RECLAIM_RETRIES;
> }
> -
> - ret = __mem_cgroup_do_charge(mem, gfp_mask, csize, oom_check);
> + if (use_pcp_cache)
> + ret = __mem_cgroup_do_charge(mem, gfp_mask,
> + CHARGE_SIZE, false, oom_check);
> + else
> + ret = __mem_cgroup_do_charge(mem, gfp_mask,
> + page_size, true, oom_check);
>
hmm, this confuses me. I think 'use_pcp_cache' will be used to decide
whether we should do consume_stock() or not, but why we change charge size
and reclaim behavior depending on it ? I think this code itself is right,
but using 'use_pcp_cache' confused me.
> switch (ret) {
> case CHARGE_OK:
> break;
> case CHARGE_RETRY: /* not in OOM situation but retry */
> - csize = page_size;
> + if (use_pcp_cache)/* need to reclaim pages */
> + use_pcp_cache = false;
> css_put(&mem->css);
> mem = NULL;
> goto again;
> + case CHARGE_NEED_BREAK: /* page_size > PAGE_SIZE */
> + css_put(&mem->css);
> + /* returning faiulre doesn't mean OOM for hugepages */
> + goto nomem;
I like this change.
> case CHARGE_WOULDBLOCK: /* !__GFP_WAIT */
> css_put(&mem->css);
> goto nomem;
> @@ -1994,9 +2013,9 @@ again:
> goto bypass;
> }
> } while (ret != CHARGE_OK);
> -
> - if (csize > page_size)
> - refill_stock(mem, csize - page_size);
> + /* This flag is cleared when we fail CHAEGE_SIZE charge. */
> + if (use_pcp_cache)
> + refill_stock(mem, CHARGE_SIZE - page_size);
Ditto. can't we keep 'csize' and old code here ?
> css_put(&mem->css);
> done:
> *memcg = mem;
>
Thanks,
Daisuke Nishimura.
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Powered by blists - more mailing lists