On Tue, Dec 13, 2011 at 8:21 AM, Michal Hocko <mhocko@xxxxxxx> wrote:
> 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 ;)
Thank you for looking into this :)
This patch was made as part of the effort solving the livelock issue.
Then it becomes a separate question by itself.
I don't quite understand the mismatch on gfp_mask = __GFP_NORETRY &&
oom_check == true. In page allocator we bypass the retry as well as
the oom if the flag is set, but we ingore the flag totally when
getting into the memcg charge. Why is that, and is there any
implications to use "oom_check" instead?
Thanks
--Ying
>
>>
>> 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