> On Aug 15, 2018, at 10:32 AM, Shakeel Butt <shakeelb@xxxxxxxxxx> wrote:
>
>> On Wed, Aug 15, 2018 at 10:26 AM Roman Gushchin <guro@xxxxxx> wrote:
>>
>>> On Wed, Aug 15, 2018 at 10:12:42AM -0700, Andy Lutomirski wrote:
>>>
>>>
>>>>> On Aug 15, 2018, at 9:55 AM, Roman Gushchin <guro@xxxxxx> wrote:
>>>>>
>>>>>> On Wed, Aug 15, 2018 at 12:39:23PM -0400, Johannes Weiner wrote:
>>>>>> On Tue, Aug 14, 2018 at 05:36:19PM -0700, Roman Gushchin wrote:
>>>>>> @@ -224,9 +224,14 @@ static unsigned long *alloc_thread_stack_node(struct task_struct *tsk, int node)
>>>>>> return s->addr;
>>>>>> }
>>>>>>
>>>>>> + /*
>>>>>> + * Allocated stacks are cached and later reused by new threads,
>>>>>> + * so memcg accounting is performed manually on assigning/releasing
>>>>>> + * stacks to tasks. Drop __GFP_ACCOUNT.
>>>>>> + */
>>>>>> stack = __vmalloc_node_range(THREAD_SIZE, THREAD_ALIGN,
>>>>>> VMALLOC_START, VMALLOC_END,
>>>>>> - THREADINFO_GFP,
>>>>>> + THREADINFO_GFP & ~__GFP_ACCOUNT,
>>>>>> PAGE_KERNEL,
>>>>>> 0, node, __builtin_return_address(0));
>>>>>>
>>>>>> @@ -246,12 +251,41 @@ static unsigned long *alloc_thread_stack_node(struct task_struct *tsk, int node)
>>>>>> #endif
>>>>>> }
>>>>>>
>>>>>> +static void memcg_charge_kernel_stack(struct task_struct *tsk)
>>>>>> +{
>>>>>> +#ifdef CONFIG_VMAP_STACK
>>>>>> + struct vm_struct *vm = task_stack_vm_area(tsk);
>>>>>> +
>>>>>> + if (vm) {
>>>>>> + int i;
>>>>>> +
>>>>>> + for (i = 0; i < THREAD_SIZE / PAGE_SIZE; i++)
>>>>>> + memcg_kmem_charge(vm->pages[i], __GFP_NOFAIL,
>>>>>> + compound_order(vm->pages[i]));
>>>>>> +
>>>>>> + /* All stack pages belong to the same memcg. */
>>>>>> + mod_memcg_page_state(vm->pages[0], MEMCG_KERNEL_STACK_KB,
>>>>>> + THREAD_SIZE / 1024);
>>>>>> + }
>>>>>> +#endif
>>>>>> +}
>>>>>
>>>>> Before this change, the memory limit can fail the fork, but afterwards
>>>>> fork() can grow memory consumption unimpeded by the cgroup settings.
>>>>>
>>>>> Can we continue to use try_charge() here and fail the fork?
>>>>
>>>> We can, but I'm not convinced we should.
>>>>
>>>> Kernel stack is relatively small, and it's already allocated at this point.
>>>> So IMO exceeding the memcg limit for 1-2 pages isn't worse than
>>>> adding complexity and handle this case (e.g. uncharge partially
>>>> charged stack). Do you have an example, when it does matter?
>>>
>>> What bounds it to just a few pages? Couldn’t there be lots of forks in flight that all hit this path? It’s unlikely, and there are surely easier DoS vectors, but still.
>>
>> Because any following memcg-aware allocation will fail.
>> There is also the pid cgroup controlled which can be used to limit the number
>> of forks.
>>
>> Anyway, I'm ok to handle the this case and fail fork,
>> if you think it does matter.
>
> Roman, before adding more changes do benchmark this. Maybe disabling
> the stack caching for CONFIG_MEMCG is much cleaner.
>
>
Unless memcg accounting is colossally slow, the caching should be left on. vmalloc() isn’t inherently slow, but vfree() is, since we need to do a global broadcast TLB flush after enough vfree() calls.
