Re: [RFC PATCH] mm/memcontrol: Increase threshold for draining per-cpu stocked bytes.

[Imran Khan <imran.f.khan@oracle.com>]

[-- Attachment #1: Type: text/plain, Size: 8620 bytes --]

On 6/1/21 2:29 pm, Roman Gushchin wrote:

> On Wed, Jan 06, 2021 at 02:07:12PM +1100, Imran Khan wrote:
>> On 6/1/21 5:45 am, Roman Gushchin wrote:
>>> On Tue, Jan 05, 2021 at 10:23:52AM -0800, Roman Gushchin wrote:
>>>> On Tue, Jan 05, 2021 at 04:07:42PM +0000, Imran Khan wrote:
>>>>> While allocating objects whose size is multiple of PAGE_SIZE,
>>>>> say kmalloc-4K, we charge one page for extra bytes corresponding
>>>>> to the obj_cgroup membership pointer and remainder of the charged
>>>>> page gets added to per-cpu stocked bytes. If this allocation is
>>>>> followed by another allocation of the same size, the stocked bytes
>>>>> will not suffice and thus we endup charging an extra page
>>>>> again for membership pointer and remainder of this page gets added
>>>>> to per-cpu stocked bytes. This second addition will cause amount of
>>>>> stocked bytes to go beyond PAGE_SIZE and hence will result in
>>>>> invocation of drain_obj_stock.
>>>>>
>>>>> So if we are in a scenario where we are consecutively allocating,
>>>>> several PAGE_SIZE multiple sized objects, the stocked bytes will
>>>>> never be enough to suffice a request and every second request will
>>>>> trigger draining of stocked bytes.
>>>>>
>>>>> For example invoking __alloc_skb multiple times with
>>>>> 2K < packet size < 4K will give a call graph like:
>>>>>
>>>>> __alloc_skb
>>>>>       |
>>>>>       |__kmalloc_reserve.isra.61
>>>>>       |    |
>>>>>       |    |__kmalloc_node_track_caller
>>>>>       |    |    |
>>>>>       |    |    |slab_pre_alloc_hook.constprop.88
>>>>>       |    |     obj_cgroup_charge
>>>>>       |    |    |    |
>>>>>       |    |    |    |__memcg_kmem_charge
>>>>>       |    |    |    |    |
>>>>>       |    |    |    |    |page_counter_try_charge
>>>>>       |    |    |    |
>>>>>       |    |    |    |refill_obj_stock
>>>>>       |    |    |    |    |
>>>>>       |    |    |    |    |drain_obj_stock.isra.68
>>>>>       |    |    |    |    |    |
>>>>>       |    |    |    |    |    |__memcg_kmem_uncharge
>>>>>       |    |    |    |    |    |    |
>>>>>       |    |    |    |    |    |    |page_counter_uncharge
>>>>>       |    |    |    |    |    |    |    |
>>>>>       |    |    |    |    |    |    |    |page_counter_cancel
>>>>>       |    |    |
>>>>>       |    |    |
>>>>>       |    |    |__slab_alloc
>>>>>       |    |    |    |
>>>>>       |    |    |    |___slab_alloc
>>>>>       |    |    |    |
>>>>>       |    |    |slab_post_alloc_hook
>>>>>
>>>>> This frequent draining of stock bytes and resultant charging of pages
>>>>> increases the CPU load and hence deteriorates the scheduler latency.
>>>>>
>>>>> The above mentioned scenario and it's impact can be seen by running
>>>>> hackbench with large packet size on v5.8 and subsequent kernels. The
>>>>> deterioration in hackbench number starts appearing from v5.9 kernel,
>>>>> 'commit f2fe7b09a52b ("mm: memcg/slab: charge individual slab objects
>>>>> instead of pages")'.
>>>>>
>>>>> Increasing the draining limit to twice of KMALLOC_MAX_CACHE_SIZE
>>>>> (a safe upper limit for size of slab cache objects), will avoid draining
>>>>> of stock, every second allocation request, for the above mentioned
>>>>> scenario and hence will reduce the CPU load for such cases. For
>>>>> allocation of smaller objects or other allocation patterns the behaviour
>>>>> will be same as before.
>>>>>
>>>>> This change increases the draining threshold for per-cpu stocked bytes
>>>>> from PAGE_SIZE to KMALLOC_MAX_CACHE_SIZE * 2.
>>>> Hello, Imran!
>>>>
>>>> Yes, it makes total sense to me.
>> Hi Roman,
>>
>> Thanks for reviewing this patch.
>>
>>>> Btw, in earlier versions of the new slab controller there was a separate stock
>>>> for byte-sized charging and it was 32 pages large. Later Johannes suggested
>>>> the current layered design and he thought that because of the layering a single
>>>> page is enough for the upper layer.
>>>>
>>>>> Below are the hackbench numbers with and without this change on
>>>>> v5.10.0-rc7.
>>>>>
>>>>> Without this change:
>>>>>       # hackbench process 10 1000 -s 100000
>>>>>       Running in process mode with 10 groups using 40 file descriptors
>>>>>       each (== 400 tasks)
>>>>>       Each sender will pass 100 messages of 100000 bytes
>>>>>       Time: 4.401
>>>>>
>>>>>       # hackbench process 10 1000 -s 100000
>>>>>       Running in process mode with 10 groups using 40 file descriptors
>>>>>       each (== 400 tasks)
>>>>>       Each sender will pass 100 messages of 100000 bytes
>>>>>       Time: 4.470
>>>>>
>>>>> With this change:
>>>>>       # hackbench process 10 1000 -s 100000
>>>>>       Running in process mode with 10 groups using 40 file descriptors
>>>>>       each (== 400 tasks)
>>>>>       Each sender will pass 100 messages of 100000 bytes
>>>>>       Time: 3.782
>>>>>
>>>>>       # hackbench process 10 1000 -s 100000
>>>>>       Running in process mode with 10 groups using 40 file descriptors
>>>>>       each (== 400 tasks)
>>>>>       Each sender will pass 100 messages of 100000 bytes
>>>>>       Time: 3.827
>>>>>
>>>>> As can be seen the change gives an improvement of about 15% in hackbench
>>>>> numbers.
>>>>> Also numbers obtained with the change are inline with those obtained
>>>>> from v5.8 kernel.
>>>> The difference is quite impressive!
>>>>
>>>> I wonder if you tried smaller values than KMALLOC_MAX_CACHE_SIZE * 2?
>>>> Let's say 16 and 32?
>> I have tested my change with smaller sizes as well and could see similar difference
>> in hackbench numbers
>>
>> Without change(5.10.0-rc7 vanilla):
>>
>> # hackbench process 10 1000 -s 16
>> Running in process mode with 10 groups using 40 file descriptors each (== 400 tasks)
>> Each sender will pass 100 messages of 16 bytes
>> Time: 0.429
>>
>> # hackbench process 10 1000 -s 32
>> Running in process mode with 10 groups using 40 file descriptors each (== 400 tasks)
>> Each sender will pass 100 messages of 32 bytes
>> Time: 0.458
>>
>> With my changes on top of 5.10.0-rc7
>> # hackbench process 10 1000 -s 16
>> Running in process mode with 10 groups using 40 file descriptors each (== 400 tasks)
>> Each sender will pass 100 messages of 16 bytes
>> Time: 0.347
>>
>> # hackbench process 10 1000 -s 32
>> Running in process mode with 10 groups using 40 file descriptors each (== 400 tasks)
>> Each sender will pass 100 messages of 32 bytes
>> Time: 0.324
>>
>> I am confirming using BCC based argdist tool that these sizes result in call to
>> __alloc_skb with size as 16 and 32 respectively.
>>
>>>> KMALLOC_MAX_CACHE_SIZE * 2 makes sense to me, but then the whole construction
>>>> with two layer caching is very questionable. Anyway, it's not a reason to not
>>>> merge your patch, just something I wanna look at later.
>>> Hm, can you, please, benchmark the following change (without your change)?
>>>
>>> @@ -3204,7 +3204,7 @@ static void drain_obj_stock(struct memcg_stock_pcp *stock)
>>>    		if (nr_pages) {
>>>    			rcu_read_lock();
>>> -			__memcg_kmem_uncharge(obj_cgroup_memcg(old), nr_pages);
>>> +			refill_stock(obj_cgroup_memcg(old), nr_pages);
>>>    			rcu_read_unlock();
>>>    		}
>> I have tested this change on top of v5.10-rc7 and this too gives performance improvement.
>> I further confirmed using flamegraphs that with this change too we are avoiding following
>> CPU intensive path
>>
>> |__memcg_kmem_uncharge
>>      |
>>      |page_counter_uncharge
>>      |    |
>>      |    |page_counter_cancel
>>
>> Please find the hackbench numbers with your change as given below:
>> # hackbench process 10 1000 -s 100000
>> Running in process mode with 10 groups using 40 file descriptors each (== 400 tasks)
>> Each sender will pass 100 messages of 100000 bytes
>> Time: 3.841
>>
>> # hackbench process 10 1000 -s 100000
>> Running in process mode with 10 groups using 40 file descriptors each (== 400 tasks)
>> Each sender will pass 100 messages of 100000 bytes
>> Time: 3.863
>>
>> # hackbench process 10 1000 -s 16
>> Running in process mode with 10 groups using 40 file descriptors each (== 400 tasks)
>> Each sender will pass 100 messages of 16 bytes
>> Time: 0.306
>>
>> # hackbench process 10 1000 -s 32
>> Running in process mode with 10 groups using 40 file descriptors each (== 400 tasks)
>> Each sender will pass 100 messages of 32 bytes
>> Time: 0.320
> Thank you for testing it!
>
> If there is no significant difference, I'd prefer to stick with this change instead of increasing
> the size of the percpu batch, because it will preserve the accuracy of accounting.
>
> Will it work for you?

Yes, this works for me too.

Thanks,
Imran

>
> Thanks!

[-- Attachment #2: Type: text/html, Size: 9462 bytes --]