2024-01-23 07:22:13

by Oliver Sang

[permalink] [raw]
Subject: Re: [linus:master] [mm] 8d59d2214c: vm-scalability.throughput -36.6% regression

hi, Yosry Ahmed,

On Mon, Jan 22, 2024 at 01:39:19PM -0800, Yosry Ahmed wrote:
> On Mon, Jan 22, 2024 at 12:39 AM kernel test robot
> <[email protected]> wrote:
> >
> >
> >
> > hi, Yosry Ahmed,
> >
> > per your suggestion in
> > https://lore.kernel.org/all/CAJD7tkameJBrJQxRj+ibKL6-yd-i0wyoyv2cgZdh3ZepA1p7wA@mail.gmail.com/
> > "I think it would be useful to know if there are
> > regressions/improvements in other microbenchmarks, at least to
> > investigate whether they represent real regressions."
> >
> > we still report below two regressions to you just FYI what we observed in our
> > microbenchmark tests.
> > (we still captured will-it-scale::fallocate regression but ignore here per
> > your commit message)
> >
> >
> > Hello,
> >
> > kernel test robot noticed a -36.6% regression of vm-scalability.throughput on:
> >
> >
> > commit: 8d59d2214c2362e7a9d185d80b613e632581af7b ("mm: memcg: make stats flushing threshold per-memcg")
> > https://git.kernel.org/cgit/linux/kernel/git/torvalds/linux.git master
> >
> > testcase: vm-scalability
> > test machine: 224 threads 4 sockets Intel(R) Xeon(R) Platinum 8380H CPU @ 2.90GHz (Cooper Lake) with 192G memory
> > parameters:
> >
> > runtime: 300s
> > size: 1T
> > test: lru-shm
> > cpufreq_governor: performance
> >
> > test-description: The motivation behind this suite is to exercise functions and regions of the mm/ of the Linux kernel which are of interest to us.
> > test-url: https://git.kernel.org/cgit/linux/kernel/git/wfg/vm-scalability.git/
> >
> > In addition to that, the commit also has significant impact on the following tests:
> >
> > +------------------+----------------------------------------------------------------------------------------------------+
> > | testcase: change | will-it-scale: will-it-scale.per_process_ops -32.3% regression |
> > | test machine | 104 threads 2 sockets (Skylake) with 192G memory |
> > | test parameters | cpufreq_governor=performance |
> > | | mode=process |
> > | | nr_task=50% |
> > | | test=tlb_flush2 |
> > +------------------+----------------------------------------------------------------------------------------------------+
> >
> >
> > If you fix the issue in a separate patch/commit (i.e. not just a new version of
> > the same patch/commit), kindly add following tags
> > | Reported-by: kernel test robot <[email protected]>
> > | Closes: https://lore.kernel.org/oe-lkp/[email protected]
>
> Thanks for reporting this. We have had these patches running on O(10K)
> machines in our production for a while now, and there haven't been any
> complaints (at least not yet). OTOH, we do see significant CPU savings
> on reading memcg stats.
>
> That being said, I think we can improve the performance here by
> caching pointers to the parent_memcg->vmstats_percpu and
> memcg->vmstats in struct memcg_vmstat_percpu. This should
> significantly reduce the memory fetches in the loop in
> memcg_rstat_updated().
>
> Oliver, would you be able to test if the attached patch helps? It's
> based on 8d59d2214c236.

the patch failed to compile:

build_errors:
- "mm/memcontrol.c:731:38: error: 'x' undeclared (first use in this function)"


>
> [..]




2024-01-23 07:43:13

by Yosry Ahmed

[permalink] [raw]
Subject: Re: [linus:master] [mm] 8d59d2214c: vm-scalability.throughput -36.6% regression

> > Oliver, would you be able to test if the attached patch helps? It's
> > based on 8d59d2214c236.
>
> the patch failed to compile:
>
> build_errors:
> - "mm/memcontrol.c:731:38: error: 'x' undeclared (first use in this function)"

Apologizes, apparently I sent the patch with some pending diff in my
tree that I hadn't committed. Please find a fixed patch attached.

Thanks.


Attachments:
0001-mm-memcg-optimize-parent-iteration-in-memcg_rstat_up.patch (3.94 kB)

2024-01-24 08:27:10

by Oliver Sang

[permalink] [raw]
Subject: Re: [linus:master] [mm] 8d59d2214c: vm-scalability.throughput -36.6% regression

hi, Yosry Ahmed,

On Mon, Jan 22, 2024 at 11:42:04PM -0800, Yosry Ahmed wrote:
> > > Oliver, would you be able to test if the attached patch helps? It's
> > > based on 8d59d2214c236.
> >
> > the patch failed to compile:
> >
> > build_errors:
> > - "mm/memcontrol.c:731:38: error: 'x' undeclared (first use in this function)"
>
> Apologizes, apparently I sent the patch with some pending diff in my
> tree that I hadn't committed. Please find a fixed patch attached.

the regression disappears after applying the patch.

Tested-by: kernel test robot <[email protected]>

for 1st regression we reported (details is attached as vm-scalability):

=========================================================================================
compiler/cpufreq_governor/kconfig/rootfs/runtime/size/tbox_group/test/testcase:
gcc-12/performance/x86_64-rhel-8.3/debian-11.1-x86_64-20220510.cgz/300s/1T/lkp-cpl-4sp2/lru-shm/vm-scalability

commit:
e0bf1dc859fdd mm: memcg: move vmstats structs definition above flushing code
8d59d2214c236 mm: memcg: make stats flushing threshold per-memcg
0cba55e237ba6 mm: memcg: optimize parent iteration in memcg_rstat_updated()

e0bf1dc859fdd08e 8d59d2214c2362e7a9d185d80b6 0cba55e237ba61489c0a29f7d27
---------------- --------------------------- ---------------------------
%stddev %change %stddev %change %stddev
\ | \ | \
946447 -37.8% 588327 -1.1% 936279 vm-scalability.median
2.131e+08 -36.6% 1.351e+08 -1.4% 2.102e+08 vm-scalability.throughput



for 2nd regression (details is attached as will-it-scale-tlb_flush2):

=========================================================================================
compiler/cpufreq_governor/kconfig/mode/nr_task/rootfs/tbox_group/test/testcase:
gcc-12/performance/x86_64-rhel-8.3/process/50%/debian-11.1-x86_64-20220510.cgz/lkp-skl-fpga01/tlb_flush2/will-it-scale

commit:
e0bf1dc859fdd mm: memcg: move vmstats structs definition above flushing code
8d59d2214c236 mm: memcg: make stats flushing threshold per-memcg
0cba55e237ba6 mm: memcg: optimize parent iteration in memcg_rstat_updated()

e0bf1dc859fdd08e 8d59d2214c2362e7a9d185d80b6 0cba55e237ba61489c0a29f7d27
---------------- --------------------------- ---------------------------
%stddev %change %stddev %change %stddev
\ | \ | \
2132437 -32.3% 1444430 +0.9% 2151460 will-it-scale.52.processes
41008 -32.3% 27776 +0.9% 41373 will-it-scale.per_process_ops



interesting thing is, it also helps on will-it-scale:fallocate tests which we
ignored in original report (details is attached as will-it-scale-fallocate1):

=========================================================================================
compiler/cpufreq_governor/kconfig/mode/nr_task/rootfs/tbox_group/test/testcase:
gcc-12/performance/x86_64-rhel-8.3/process/100%/debian-11.1-x86_64-20220510.cgz/lkp-cpl-4sp2/fallocate1/will-it-scale

commit:
e0bf1dc859fdd mm: memcg: move vmstats structs definition above flushing code
8d59d2214c236 mm: memcg: make stats flushing threshold per-memcg
0cba55e237ba6 mm: memcg: optimize parent iteration in memcg_rstat_updated()

e0bf1dc859fdd08e 8d59d2214c2362e7a9d185d80b6 0cba55e237ba61489c0a29f7d27
---------------- --------------------------- ---------------------------
%stddev %change %stddev %change %stddev
\ | \ | \
5426049 -33.8% 3590953 +3.3% 5605429 will-it-scale.224.processes
24222 -33.8% 16030 +3.3% 25023 will-it-scale.per_process_ops

>
> Thanks.



Attachments:
(No filename) (3.93 kB)
vm-scalability (81.33 kB)
will-it-scale-tlb_flush2 (38.19 kB)
will-it-scale-fallocate1 (33.26 kB)
Download all attachments

2024-01-24 09:12:49

by Yosry Ahmed

[permalink] [raw]
Subject: Re: [linus:master] [mm] 8d59d2214c: vm-scalability.throughput -36.6% regression

On Wed, Jan 24, 2024 at 12:26 AM Oliver Sang <[email protected]> wrote:
>
> hi, Yosry Ahmed,
>
> On Mon, Jan 22, 2024 at 11:42:04PM -0800, Yosry Ahmed wrote:
> > > > Oliver, would you be able to test if the attached patch helps? It's
> > > > based on 8d59d2214c236.
> > >
> > > the patch failed to compile:
> > >
> > > build_errors:
> > > - "mm/memcontrol.c:731:38: error: 'x' undeclared (first use in this function)"
> >
> > Apologizes, apparently I sent the patch with some pending diff in my
> > tree that I hadn't committed. Please find a fixed patch attached.
>
> the regression disappears after applying the patch.
>
> Tested-by: kernel test robot <[email protected]>

Awesome! Thanks for testing. I will formalize the patch and send it
out for review.