Re: [LKP] [lkp-robot] [mm, memcontrol] 309fe96bfc: vm-scalability.throughput +23.0% improvement
On Fri, Jun 01, 2018 at 03:26:04PM +0800, Aaron Lu wrote: > On Mon, May 28, 2018 at 07:40:19PM +0800, kernel test robot wrote: > > > > Greeting, > > > > FYI, we noticed a +23.0% improvement of vm-scalability.throughput due to > > commit: > > > > > > commit: 309fe96bfc0ae387f53612927a8f0dc3eb056efd ("mm, memcontrol: > > implement memory.swap.events") > > https://git.kernel.org/cgit/linux/kernel/git/next/linux-next.git master > > > > in testcase: vm-scalability > > on test machine: 144 threads Intel(R) Xeon(R) CPU E7-8890 v3 @ 2.50GHz with > > 512G memory > > with following 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/ > > > > With the patch I just sent out: > "mem_cgroup: make sure moving_account, move_lock_task and stat_cpu in the > same cacheline" > > Applying this commit on top doesn't yield 23% improvement any more, but > a 6% performace drop... > I found the culprit being the following one line introduced in this commit: > > diff --git a/mm/memcontrol.c b/mm/memcontrol.c > index d90b0201a8c4..07ab974c0a49 100644 > --- a/mm/memcontrol.c > +++ b/mm/memcontrol.c > @@ -6019,13 +6019,17 @@ int mem_cgroup_try_charge_swap(struct page *page, > swp_entry_t entry) > if (!memcg) > return 0; > > - if (!entry.val) > + if (!entry.val) { > + memcg_memory_event(memcg, MEMCG_SWAP_FAIL); Removing this line restored performance but it really doesn't make any sense. Ying suggested it might be code alignment related and suggested to use a different compiler than gcc-7.2. Then I used gcc-6.4 and turned out the test result to be pretty much the same for the two commits: (each test has run for 3 times) $ grep throughput base/*/stats.json base/0/stats.json: "vm-scalability.throughput": 89207489, base/1/stats.json: "vm-scalability.throughput": 89982933, base/2/stats.json: "vm-scalability.throughput": 90436592, $ grep throughput head/*/stats.json head/0/stats.json: "vm-scalability.throughput": 90882775, head/1/stats.json: "vm-scalability.throughput": 90675220, head/2/stats.json: "vm-scalability.throughput": 91173479, So probably it's really related to code alignment and this bisected commit doesn't cause performance change(as expected). > return 0; > + } > > memcg = mem_cgroup_id_get_online(memcg); > > If I remove that memcg_memory_event() call, performance will restore. > > It's beyond my understanding why this code path matters since there is > no swap device setup in the test machine so I don't see how possible > get_swap_page() could ever be called. > > Still investigating... >
Re: [LKP] [lkp-robot] [mm, memcontrol] 309fe96bfc: vm-scalability.throughput +23.0% improvement
On Mon, May 28, 2018 at 07:40:19PM +0800, kernel test robot wrote: > > Greeting, > > FYI, we noticed a +23.0% improvement of vm-scalability.throughput due to > commit: > > > commit: 309fe96bfc0ae387f53612927a8f0dc3eb056efd ("mm, memcontrol: implement > memory.swap.events") > https://git.kernel.org/cgit/linux/kernel/git/next/linux-next.git master > > in testcase: vm-scalability > on test machine: 144 threads Intel(R) Xeon(R) CPU E7-8890 v3 @ 2.50GHz with > 512G memory > with following 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/ > With the patch I just sent out: "mem_cgroup: make sure moving_account, move_lock_task and stat_cpu in the same cacheline" Applying this commit on top doesn't yield 23% improvement any more, but a 6% performace drop... I found the culprit being the following one line introduced in this commit: diff --git a/mm/memcontrol.c b/mm/memcontrol.c index d90b0201a8c4..07ab974c0a49 100644 --- a/mm/memcontrol.c +++ b/mm/memcontrol.c @@ -6019,13 +6019,17 @@ int mem_cgroup_try_charge_swap(struct page *page, swp_entry_t entry) if (!memcg) return 0; - if (!entry.val) + if (!entry.val) { + memcg_memory_event(memcg, MEMCG_SWAP_FAIL); return 0; + } memcg = mem_cgroup_id_get_online(memcg); If I remove that memcg_memory_event() call, performance will restore. It's beyond my understanding why this code path matters since there is no swap device setup in the test machine so I don't see how possible get_swap_page() could ever be called. Still investigating...
Re: [LKP] [lkp-robot] [mm, memcontrol] 309fe96bfc: vm-scalability.throughput +23.0% improvement
On Tue, May 29, 2018 at 10:27:51AM +0200, Michal Hocko wrote: > On Tue 29-05-18 16:11:27, Aaron Lu wrote: > > On Tue, May 29, 2018 at 09:58:00AM +0200, Michal Hocko wrote: > > > On Tue 29-05-18 03:15:51, Lu, Aaron wrote: > > > > On Mon, 2018-05-28 at 14:03 +0200, Michal Hocko wrote: > > > > > On Mon 28-05-18 19:40:19, kernel test robot wrote: > > > > > > > > > > > > Greeting, > > > > > > > > > > > > FYI, we noticed a +23.0% improvement of vm-scalability.throughput > > > > > > due to commit: > > > > > > > > > > > > > > > > > > commit: 309fe96bfc0ae387f53612927a8f0dc3eb056efd ("mm, memcontrol: > > > > > > implement memory.swap.events") > > > > > > https://git.kernel.org/cgit/linux/kernel/git/next/linux-next.git > > > > > > master > > > > > > > > > > This doesn't make any sense to me. The patch merely adds an > > > > > accounting. > > > > > It doesn't optimize anything. So I strongly suspect the result is just > > > > > misleading or the test (environment) misconfigured. Not the first time > > > > > I am seeing something like that I am afraid. > > > > > > > > > > > > > Most likely the same situation as: > > > > " > > > > FYI, we noticed a -27.2% regression of will-it-scale.per_process_ops > > > > due to commit: > > > > > > > > > > > > commit: e27be240df53f1a20c659168e722b5d9f16cc7f4 ("mm: memcg: make sure > > > > memory.events is uptodate when waking pollers") > > > > https://git.kernel.org/cgit/linux/kernel/git/torvalds/linux.git master > > > > " > > > > > > > > Where the performance change is due to layout change of > > > > 'struct mem_cgroup': > > > > http://lkml.kernel.org/r/20180528085201.ga2...@intel.com > > > > > > I do not follow. How can _this_ patch lead to an improvement when it > > > actually _adds_ an accounting? The other report you are mentioning is a > > > > This patch also changed the layout of 'struct mem_cgroup': > > > > diff --git a/include/linux/memcontrol.h b/include/linux/memcontrol.h > > index d99b71bc2c66..517096c3cc99 100644 > > --- a/include/linux/memcontrol.h > > +++ b/include/linux/memcontrol.h > > @@ -208,6 +210,9 @@ struct mem_cgroup { > > atomic_long_t memory_events[MEMCG_NR_MEMORY_EVENTS]; > > struct cgroup_file events_file; > > > > + /* handle for "memory.swap.events" */ > > + struct cgroup_file swap_events_file; > > + > > /* protect arrays of thresholds */ > > struct mutex thresholds_lock; > > > > And I'm guessing that might be the cause. > > Ohh, you are right! Sorry, I've missed that part. Never mind, I want to thank you for taking a look at these reports :-) I just tried to move this newly added field to the bottom of the structure(just above 'struct mem_cgroup_per_node *nodeinfo[0];'), and performance dropped to 82665166, still much better than base but already worse than this patch. As you said in another email, this is really fragile.
Re: [LKP] [lkp-robot] [mm, memcontrol] 309fe96bfc: vm-scalability.throughput +23.0% improvement
On Tue 29-05-18 16:11:27, Aaron Lu wrote: > On Tue, May 29, 2018 at 09:58:00AM +0200, Michal Hocko wrote: > > On Tue 29-05-18 03:15:51, Lu, Aaron wrote: > > > On Mon, 2018-05-28 at 14:03 +0200, Michal Hocko wrote: > > > > On Mon 28-05-18 19:40:19, kernel test robot wrote: > > > > > > > > > > Greeting, > > > > > > > > > > FYI, we noticed a +23.0% improvement of vm-scalability.throughput due > > > > > to commit: > > > > > > > > > > > > > > > commit: 309fe96bfc0ae387f53612927a8f0dc3eb056efd ("mm, memcontrol: > > > > > implement memory.swap.events") > > > > > https://git.kernel.org/cgit/linux/kernel/git/next/linux-next.git > > > > > master > > > > > > > > This doesn't make any sense to me. The patch merely adds an accounting. > > > > It doesn't optimize anything. So I strongly suspect the result is just > > > > misleading or the test (environment) misconfigured. Not the first time > > > > I am seeing something like that I am afraid. > > > > > > > > > > Most likely the same situation as: > > > " > > > FYI, we noticed a -27.2% regression of will-it-scale.per_process_ops > > > due to commit: > > > > > > > > > commit: e27be240df53f1a20c659168e722b5d9f16cc7f4 ("mm: memcg: make sure > > > memory.events is uptodate when waking pollers") > > > https://git.kernel.org/cgit/linux/kernel/git/torvalds/linux.git master > > > " > > > > > > Where the performance change is due to layout change of > > > 'struct mem_cgroup': > > > http://lkml.kernel.org/r/20180528085201.ga2...@intel.com > > > > I do not follow. How can _this_ patch lead to an improvement when it > > actually _adds_ an accounting? The other report you are mentioning is a > > This patch also changed the layout of 'struct mem_cgroup': > > diff --git a/include/linux/memcontrol.h b/include/linux/memcontrol.h > index d99b71bc2c66..517096c3cc99 100644 > --- a/include/linux/memcontrol.h > +++ b/include/linux/memcontrol.h > @@ -208,6 +210,9 @@ struct mem_cgroup { > atomic_long_t memory_events[MEMCG_NR_MEMORY_EVENTS]; > struct cgroup_file events_file; > > + /* handle for "memory.swap.events" */ > + struct cgroup_file swap_events_file; > + > /* protect arrays of thresholds */ > struct mutex thresholds_lock; > > And I'm guessing that might be the cause. Ohh, you are right! Sorry, I've missed that part. -- Michal Hocko SUSE Labs
Re: [LKP] [lkp-robot] [mm, memcontrol] 309fe96bfc: vm-scalability.throughput +23.0% improvement
On Tue, May 29, 2018 at 09:58:00AM +0200, Michal Hocko wrote: > On Tue 29-05-18 03:15:51, Lu, Aaron wrote: > > On Mon, 2018-05-28 at 14:03 +0200, Michal Hocko wrote: > > > On Mon 28-05-18 19:40:19, kernel test robot wrote: > > > > > > > > Greeting, > > > > > > > > FYI, we noticed a +23.0% improvement of vm-scalability.throughput due > > > > to commit: > > > > > > > > > > > > commit: 309fe96bfc0ae387f53612927a8f0dc3eb056efd ("mm, memcontrol: > > > > implement memory.swap.events") > > > > https://git.kernel.org/cgit/linux/kernel/git/next/linux-next.git master > > > > > > This doesn't make any sense to me. The patch merely adds an accounting. > > > It doesn't optimize anything. So I strongly suspect the result is just > > > misleading or the test (environment) misconfigured. Not the first time > > > I am seeing something like that I am afraid. > > > > > > > Most likely the same situation as: > > " > > FYI, we noticed a -27.2% regression of will-it-scale.per_process_ops > > due to commit: > > > > > > commit: e27be240df53f1a20c659168e722b5d9f16cc7f4 ("mm: memcg: make sure > > memory.events is uptodate when waking pollers") > > https://git.kernel.org/cgit/linux/kernel/git/torvalds/linux.git master > > " > > > > Where the performance change is due to layout change of > > 'struct mem_cgroup': > > http://lkml.kernel.org/r/20180528085201.ga2...@intel.com > > I do not follow. How can _this_ patch lead to an improvement when it > actually _adds_ an accounting? The other report you are mentioning is a This patch also changed the layout of 'struct mem_cgroup': diff --git a/include/linux/memcontrol.h b/include/linux/memcontrol.h index d99b71bc2c66..517096c3cc99 100644 --- a/include/linux/memcontrol.h +++ b/include/linux/memcontrol.h @@ -208,6 +210,9 @@ struct mem_cgroup { atomic_long_t memory_events[MEMCG_NR_MEMORY_EVENTS]; struct cgroup_file events_file; + /* handle for "memory.swap.events" */ + struct cgroup_file swap_events_file; + /* protect arrays of thresholds */ struct mutex thresholds_lock; And I'm guessing that might be the cause. > _regression_ and I can imagine that the layout changes can lead to that > result.
Re: [LKP] [lkp-robot] [mm, memcontrol] 309fe96bfc: vm-scalability.throughput +23.0% improvement
On Tue 29-05-18 03:15:51, Lu, Aaron wrote: > On Mon, 2018-05-28 at 14:03 +0200, Michal Hocko wrote: > > On Mon 28-05-18 19:40:19, kernel test robot wrote: > > > > > > Greeting, > > > > > > FYI, we noticed a +23.0% improvement of vm-scalability.throughput due to > > > commit: > > > > > > > > > commit: 309fe96bfc0ae387f53612927a8f0dc3eb056efd ("mm, memcontrol: > > > implement memory.swap.events") > > > https://git.kernel.org/cgit/linux/kernel/git/next/linux-next.git master > > > > This doesn't make any sense to me. The patch merely adds an accounting. > > It doesn't optimize anything. So I strongly suspect the result is just > > misleading or the test (environment) misconfigured. Not the first time > > I am seeing something like that I am afraid. > > > > Most likely the same situation as: > " > FYI, we noticed a -27.2% regression of will-it-scale.per_process_ops > due to commit: > > > commit: e27be240df53f1a20c659168e722b5d9f16cc7f4 ("mm: memcg: make sure > memory.events is uptodate when waking pollers") > https://git.kernel.org/cgit/linux/kernel/git/torvalds/linux.git master > " > > Where the performance change is due to layout change of > 'struct mem_cgroup': > http://lkml.kernel.org/r/20180528085201.ga2...@intel.com I do not follow. How can _this_ patch lead to an improvement when it actually _adds_ an accounting? The other report you are mentioning is a _regression_ and I can imagine that the layout changes can lead to that result. -- Michal Hocko SUSE Labs
Re: [LKP] [lkp-robot] [mm, memcontrol] 309fe96bfc: vm-scalability.throughput +23.0% improvement
On Mon, 2018-05-28 at 14:03 +0200, Michal Hocko wrote: > On Mon 28-05-18 19:40:19, kernel test robot wrote: > > > > Greeting, > > > > FYI, we noticed a +23.0% improvement of vm-scalability.throughput due to > > commit: > > > > > > commit: 309fe96bfc0ae387f53612927a8f0dc3eb056efd ("mm, memcontrol: > > implement memory.swap.events") > > https://git.kernel.org/cgit/linux/kernel/git/next/linux-next.git master > > This doesn't make any sense to me. The patch merely adds an accounting. > It doesn't optimize anything. So I strongly suspect the result is just > misleading or the test (environment) misconfigured. Not the first time > I am seeing something like that I am afraid. > Most likely the same situation as: " FYI, we noticed a -27.2% regression of will-it-scale.per_process_ops due to commit: commit: e27be240df53f1a20c659168e722b5d9f16cc7f4 ("mm: memcg: make sure memory.events is uptodate when waking pollers") https://git.kernel.org/cgit/linux/kernel/git/torvalds/linux.git master " Where the performance change is due to layout change of 'struct mem_cgroup': http://lkml.kernel.org/r/20180528085201.ga2...@intel.com