linux-kernel.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
* [RFC PATCH] mm/memcontrol: Increase threshold for draining per-cpu stocked bytes.
@ 2021-01-05 16:07 Imran Khan
  2021-01-05 18:23 ` Roman Gushchin
  0 siblings, 1 reply; 5+ messages in thread
From: Imran Khan @ 2021-01-05 16:07 UTC (permalink / raw)
  To: hannes, mhocko; +Cc: akpm, cgroups, linux-mm, linux-kernel, Imran Khan

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.

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.

Signed-off-by: Imran Khan <imran.f.khan@oracle.com>
---
 mm/memcontrol.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/mm/memcontrol.c b/mm/memcontrol.c
index 0d74b80..c04633c 100644
--- a/mm/memcontrol.c
+++ b/mm/memcontrol.c
@@ -3256,7 +3256,7 @@ static void refill_obj_stock(struct obj_cgroup *objcg, unsigned int nr_bytes)
 	}
 	stock->nr_bytes += nr_bytes;
 
-	if (stock->nr_bytes > PAGE_SIZE)
+	if (stock->nr_bytes > KMALLOC_MAX_CACHE_SIZE * 2)
 		drain_obj_stock(stock);
 
 	local_irq_restore(flags);
-- 
1.8.3.1


^ permalink raw reply related	[flat|nested] 5+ messages in thread

* Re: [RFC PATCH] mm/memcontrol: Increase threshold for draining per-cpu stocked bytes.
  2021-01-05 16:07 [RFC PATCH] mm/memcontrol: Increase threshold for draining per-cpu stocked bytes Imran Khan
@ 2021-01-05 18:23 ` Roman Gushchin
  2021-01-05 18:45   ` Roman Gushchin
  0 siblings, 1 reply; 5+ messages in thread
From: Roman Gushchin @ 2021-01-05 18:23 UTC (permalink / raw)
  To: Imran Khan; +Cc: hannes, mhocko, akpm, cgroups, linux-mm, linux-kernel

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.

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?

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.

> 
> Signed-off-by: Imran Khan <imran.f.khan@oracle.com>

Reviewed-by: Roman Gushchin <guro@fb.com>

Thanks!

^ permalink raw reply	[flat|nested] 5+ messages in thread

* Re: [RFC PATCH] mm/memcontrol: Increase threshold for draining per-cpu stocked bytes.
  2021-01-05 18:23 ` Roman Gushchin
@ 2021-01-05 18:45   ` Roman Gushchin
       [not found]     ` <127fa24c-d4c4-5c24-ec30-ea6349f37923@oracle.com>
  0 siblings, 1 reply; 5+ messages in thread
From: Roman Gushchin @ 2021-01-05 18:45 UTC (permalink / raw)
  To: Imran Khan; +Cc: hannes, mhocko, akpm, cgroups, linux-mm, linux-kernel

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.
> 
> 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?
> 
> 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();
 		}
 

^ permalink raw reply	[flat|nested] 5+ messages in thread

* Re: [RFC PATCH] mm/memcontrol: Increase threshold for draining per-cpu stocked bytes.
       [not found]     ` <127fa24c-d4c4-5c24-ec30-ea6349f37923@oracle.com>
@ 2021-01-06  3:29       ` Roman Gushchin
       [not found]         ` <9bd3018e-7182-fe4a-3ba2-ed0cf2e0875a@oracle.com>
  0 siblings, 1 reply; 5+ messages in thread
From: Roman Gushchin @ 2021-01-06  3:29 UTC (permalink / raw)
  To: Imran Khan; +Cc: hannes, mhocko, akpm, cgroups, linux-mm, linux-kernel

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?

Thanks!

^ permalink raw reply	[flat|nested] 5+ messages in thread

* Re: [RFC PATCH] mm/memcontrol: Increase threshold for draining per-cpu stocked bytes.
       [not found]         ` <9bd3018e-7182-fe4a-3ba2-ed0cf2e0875a@oracle.com>
@ 2021-01-06  4:26           ` Roman Gushchin
  0 siblings, 0 replies; 5+ messages in thread
From: Roman Gushchin @ 2021-01-06  4:26 UTC (permalink / raw)
  To: Imran Khan; +Cc: hannes, mhocko, akpm, cgroups, linux-mm, linux-kernel

On Wed, Jan 06, 2021 at 02:39:05PM +1100, Imran Khan wrote:
> 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.

Great!

Just sent the full version of the patch (you're in cc).

It's slightly different: initially I've missed the handling of a separate kmem page counter.
There should be no difference on cgroup v2, and hopefully it will be still acceptable on cgroup v1.
Your Tested-by will be highly appreciated.

Thank you!

Roman

^ permalink raw reply	[flat|nested] 5+ messages in thread

end of thread, other threads:[~2021-01-06  4:27 UTC | newest]

Thread overview: 5+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2021-01-05 16:07 [RFC PATCH] mm/memcontrol: Increase threshold for draining per-cpu stocked bytes Imran Khan
2021-01-05 18:23 ` Roman Gushchin
2021-01-05 18:45   ` Roman Gushchin
     [not found]     ` <127fa24c-d4c4-5c24-ec30-ea6349f37923@oracle.com>
2021-01-06  3:29       ` Roman Gushchin
     [not found]         ` <9bd3018e-7182-fe4a-3ba2-ed0cf2e0875a@oracle.com>
2021-01-06  4:26           ` Roman Gushchin

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).