(2012/05/16 15:42), Glauber Costa wrote:
> On 05/15/2012 06:57 AM, KAMEZAWA Hiroyuki wrote:
>>> +#ifdef CONFIG_CGROUP_MEM_RES_CTLR_KMEM
>>>> +int memcg_charge_kmem(struct mem_cgroup *memcg, gfp_t gfp, s64 delta)
>>>> +{
>>>> + struct res_counter *fail_res;
>>>> + struct mem_cgroup *_memcg;
>>>> + int may_oom, ret;
>>>> + bool nofail = false;
>>>> +
>>>> + may_oom = (gfp& __GFP_WAIT)&& (gfp& __GFP_FS)&&
>>>> + !(gfp& __GFP_NORETRY);
>>>> +
>>>> + ret = 0;
>>>> +
>>>> + if (!memcg)
>>>> + return ret;
>>>> +
>>>> + _memcg = memcg;
>>>> + ret = __mem_cgroup_try_charge(NULL, gfp, delta / PAGE_SIZE,
>>>> + &_memcg, may_oom);
>>>> +
>>>> + if ((ret == -EINTR) || (ret&& (gfp& __GFP_NOFAIL))) {
>>>> + nofail = true;
>>>> + /*
>>>> + * __mem_cgroup_try_charge() chose to bypass to root due
>>>> + * to OOM kill or fatal signal.
>>>> + * Since our only options are to either fail the
>>>> + * allocation or charge it to this cgroup, force the
>>>> + * change, going above the limit if needed.
>>>> + */
>>>> + res_counter_charge_nofail(&memcg->res, delta,&fail_res);
>>>> + if (do_swap_account)
>>>> + res_counter_charge_nofail(&memcg->memsw, delta,
>>>> + &fail_res);
>>>> + } else if (ret == -ENOMEM)
>>>> + return ret;
>>>> +
>>>> + if (nofail)
>>>> + res_counter_charge_nofail(&memcg->kmem, delta,&fail_res);
>>>> + else
>>>> + ret = res_counter_charge(&memcg->kmem, delta,&fail_res);
>>
>> Ouch, you allow usage> limit ? It's BUG.
>>
>> IMHO, if GFP_NOFAIL, memcg accounting should be skipped. Please
>>
>> if (gfp_mask& __GFP_NOFAIL)
>> return 0;
>>
>> Or avoid calling memcg_charge_kmem() you can do that as you do in patch 19/29,
>> I guess you can use a trick like
>>
>> == in 19/29
>> + if (!current->mm || atomic_read(¤t->memcg_kmem_skip_account))
>> + return cachep;
>> +
>> gfp |= cachep->allocflags;
>> ==
>>
>> == change like this
>> gfp |= cachep->allocflags;
>>
>> if (!current->mm || current->memcg_kmem_skip_account || gfp& __GFP_NOFAIL))
>> ==
>>
>> Is this difficult ?
>>
>> Thanks,
>> -Kame
>
> Well, we disagree with that.
> I actually voiced this earlier to Suleiman in the thread, but it is good
> that you brought this up again - this is quite important.
>
> I will repeat my rationale here, and if you still are not convinced,
> tell me and I will be happy to switch over.
>
> I believe that the whole reasoning behind this, is to have allocations
> failing if we go over limit. If the allocation won't fail anyway, it
> doesn't really matter who we charge this to.
>
> However, if the allocation still came from a particular memcg, those
> nofail allocation may prevent it to use more memory when a normal
> allocation takes place.
>
> Consider this:
>
> limit = 4M
> usage = 4M - 4k
>
> If at this point the memcg hits a NOFAIL allocation worth 2 pages, by
> the method I am using, the memcg will be at 4M + 4k after the
> allocation. Charging it to the root memcg will leave it at 4M - 4k.
>
> This means that to be able to allocate a page again, you need to free
> two other pages, be it the 2 pages used by the GFP allocation or any
> other. In other words: the memcg that originated the charge is held
> accountable for it. If he says it can't fail for whatever reason, fine,
> we respect that, but we punish it later for other allocations.
>
I personally think 'we punish it later' is bad thing at resource accounting.
We have 'hard limit'. It's not soft limit.
> Without that GFP_NOFAIL becomes just a nice way for people to bypass
> those controls altogether, since after a ton of GFP_NOFAIL allocations,
> normal allocations will still succeed.
>
Allowing people to bypass is not bad because they're kernel.
But, IIUC, from gfp.h
==
* __GFP_NOFAIL: The VM implementation _must_ retry infinitely: the caller
* cannot handle allocation failures. This modifier is deprecated and no new
* users should be added.
==
GFP_NOFAIL will go away and no new user is recommended.
So, please skip GFP_NOFAIL accounting and avoid to write
"usage may go over limit if you're unfortune, sorry" into memcg documentation.
> The change you propose is totally doable. I just don't believe it should
> be done.
>
> But let me know where you stand.
>
My stand point is keeping "usage <= limit" is the spec. and
important in enterprise system. So, please avoid usage > limit.
Thanks,
-Kame
--
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>