On Mon 12-12-11 18:16:27, Ying Han wrote:
> In __mem_cgroup_try_charge() function, the parameter "oom" is passed from the
> caller indicating whether or not the charge should enter memcg oom kill. In
> fact, we should be able to eliminate that by using the existing gfp_mask and
> __GFP_NORETRY flag.
>
> This patch removed the "oom" parameter, and add the __GFP_NORETRY flag into
> gfp_mask for those doesn't want to enter memcg oom. There is no functional
> change for those setting false to "oom" like mem_cgroup_move_parent(), but
> __GFP_NORETRY now is checked for those even setting true to "oom".
>
> The __GFP_NORETRY is used in page allocator to bypass retry and oom kill. I
> believe there is a reason for callers to use that flag, and in memcg charge
> we need to respect it as well.
What is the reason for this change?
To be honest it makes the oom condition more obscure. __GFP_NORETRY
documentation doesn't say anything about OOM and one would have to know
details about allocator internals to follow this.
So I am not saying the patch is bad but I would need some strong reason
to like it ;)
>
> Signed-off-by: Ying Han <yinghan@xxxxxxxxxx>
> ---
> mm/memcontrol.c | 26 +++++++++++++-------------
> 1 files changed, 13 insertions(+), 13 deletions(-)
>
> diff --git a/mm/memcontrol.c b/mm/memcontrol.c
> index 894e0d2..4c49ca0 100644
> --- a/mm/memcontrol.c
> +++ b/mm/memcontrol.c
> @@ -2065,8 +2065,7 @@ static int mem_cgroup_do_charge(struct mem_cgroup *memcg, gfp_t gfp_mask,
> static int __mem_cgroup_try_charge(struct mm_struct *mm,
> gfp_t gfp_mask,
> unsigned int nr_pages,
> - struct mem_cgroup **ptr,
> - bool oom)
> + struct mem_cgroup **ptr)
> {
> unsigned int batch = max(CHARGE_BATCH, nr_pages);
> int nr_oom_retries = MEM_CGROUP_RECLAIM_RETRIES;
> @@ -2149,7 +2148,7 @@ again:
> }
>
> oom_check = false;
> - if (oom && !nr_oom_retries) {
> + if (!(gfp_mask & __GFP_NORETRY) && !nr_oom_retries) {
> oom_check = true;
> nr_oom_retries = MEM_CGROUP_RECLAIM_RETRIES;
> }
> @@ -2167,7 +2166,7 @@ again:
> css_put(&memcg->css);
> goto nomem;
> case CHARGE_NOMEM: /* OOM routine works */
> - if (!oom) {
> + if (gfp_mask & __GFP_NORETRY) {
> css_put(&memcg->css);
> goto nomem;
> }
> @@ -2456,10 +2455,11 @@ static int mem_cgroup_move_parent(struct page *page,
> if (isolate_lru_page(page))
> goto put;
>
> + gfp_mask |= __GFP_NORETRY;
> nr_pages = hpage_nr_pages(page);
>
> parent = mem_cgroup_from_cont(pcg);
> - ret = __mem_cgroup_try_charge(NULL, gfp_mask, nr_pages, &parent, false);
> + ret = __mem_cgroup_try_charge(NULL, gfp_mask, nr_pages, &parent);
> if (ret || !parent)
> goto put_back;
>
> @@ -2492,7 +2492,6 @@ static int mem_cgroup_charge_common(struct page *page, struct mm_struct *mm,
> struct mem_cgroup *memcg = NULL;
> unsigned int nr_pages = 1;
> struct page_cgroup *pc;
> - bool oom = true;
> int ret;
>
> if (PageTransHuge(page)) {
> @@ -2502,13 +2501,13 @@ static int mem_cgroup_charge_common(struct page *page, struct mm_struct *mm,
> * Never OOM-kill a process for a huge page. The
> * fault handler will fall back to regular pages.
> */
> - oom = false;
> + gfp_mask |= __GFP_NORETRY;
> }
>
> pc = lookup_page_cgroup(page);
> BUG_ON(!pc); /* XXX: remove this and move pc lookup into commit */
>
> - ret = __mem_cgroup_try_charge(mm, gfp_mask, nr_pages, &memcg, oom);
> + ret = __mem_cgroup_try_charge(mm, gfp_mask, nr_pages, &memcg);
> if (ret || !memcg)
> return ret;
>
> @@ -2571,7 +2570,7 @@ int mem_cgroup_cache_charge(struct page *page, struct mm_struct *mm,
> mm = &init_mm;
>
> if (page_is_file_cache(page)) {
> - ret = __mem_cgroup_try_charge(mm, gfp_mask, 1, &memcg, true);
> + ret = __mem_cgroup_try_charge(mm, gfp_mask, 1, &memcg);
> if (ret || !memcg)
> return ret;
>
> @@ -2629,13 +2628,13 @@ int mem_cgroup_try_charge_swapin(struct mm_struct *mm,
> if (!memcg)
> goto charge_cur_mm;
> *ptr = memcg;
> - ret = __mem_cgroup_try_charge(NULL, mask, 1, ptr, true);
> + ret = __mem_cgroup_try_charge(NULL, mask, 1, ptr);
> css_put(&memcg->css);
> return ret;
> charge_cur_mm:
> if (unlikely(!mm))
> mm = &init_mm;
> - return __mem_cgroup_try_charge(mm, mask, 1, ptr, true);
> + return __mem_cgroup_try_charge(mm, mask, 1, ptr);
> }
>
> static void
> @@ -3024,6 +3023,7 @@ int mem_cgroup_prepare_migration(struct page *page,
> int ret = 0;
>
> *ptr = NULL;
> + gfp_mask |= __GFP_NORETRY;
>
> VM_BUG_ON(PageTransHuge(page));
> if (mem_cgroup_disabled())
> @@ -3075,7 +3075,7 @@ int mem_cgroup_prepare_migration(struct page *page,
> return 0;
>
> *ptr = memcg;
> - ret = __mem_cgroup_try_charge(NULL, gfp_mask, 1, ptr, false);
> + ret = __mem_cgroup_try_charge(NULL, gfp_mask, 1, ptr);
> css_put(&memcg->css);/* drop extra refcnt */
> if (ret || *ptr == NULL) {
> if (PageAnon(page)) {
> @@ -4765,7 +4765,7 @@ one_by_one:
> cond_resched();
> }
> ret = __mem_cgroup_try_charge(NULL,
> - GFP_KERNEL, 1, &memcg, false);
> + GFP_KERNEL | __GFP_NORETRY, 1, &memcg);
> if (ret || !memcg)
> /* mem_cgroup_clear_mc() will do uncharge later */
> return -ENOMEM;
> --
> 1.7.3.1
>
> --
> To unsubscribe, send a message with 'unsubscribe linux-mm' in
> the body to majordomo@xxxxxxxxx. For more info on Linux MM,
> see: http://www.linux-mm.org/ .
> Fight unfair telecom internet charges in Canada: sign http://stopthemeter.ca/
> Don't email: <a href=mailto:"dont@xxxxxxxxx";> email@xxxxxxxxx </a>
--
Michal Hocko
SUSE Labs
SUSE LINUX s.r.o.
Lihovarska 1060/12
190 00 Praha 9
Czech Republic
--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to majordomo@xxxxxxxxx. For more info on Linux MM,
see: http://www.linux-mm.org/ .
Fight unfair telecom internet charges in Canada: sign http://stopthemeter.ca/
Don't email: <a href=mailto:"dont@xxxxxxxxx";> email@xxxxxxxxx </a>