Some time ago, we brought our UV BIOS callback code up to speed with the
new EFI memory mapping scheme, in commit:
d1be84a232e3 ("x86/uv: Update uv_bios_call() to use efi_call_virt_pointer()")
By leveraging some changes that I made to a few of the EFI runtime
callback mechanisms, in commit:
80e75596079f ("efi: Convert efi_call_virt() to efi_call_virt_pointer()")
This got everything running smoothly on UV, with the new EFI mapping
code. However, this left one, small loose end, in that EFI_OLD_MEMMAP
(a.k.a. efi=old_map) will no longer work on UV, on kernels that include
the aforementioned changes.
At the time this was not a major issue (in fact, it still really isn't),
but there's no reason that EFI_OLD_MEMMAP *shouldn't* work on our
systems. This commit adds a check into uv_bios_call, to see if we have
the EFI_OLD_MEMMAP bit set in efi.flags. If it is set, we fall back to
using our old callback method, which uses efi_call directly on the __va
of our function pointer.
v2: Invert if-statement and add unlikely() hint.
Signed-off-by: Alex Thorlton <[email protected]>
Cc: Mike Travis <[email protected]>
Cc: Russ Anderson <[email protected]>
Cc: Dimitri Sivanich <[email protected]>
Cc: Thomas Gleixner <[email protected]>
Cc: Ingo Molnar <[email protected]>
Cc: "H. Peter Anvin" <[email protected]>
Cc: Matt Fleming <[email protected]>
Cc: Masahiro Yamada <[email protected]>
Cc: [email protected]
---
arch/x86/platform/uv/bios_uv.c | 10 +++++++++-
1 file changed, 9 insertions(+), 1 deletion(-)
diff --git a/arch/x86/platform/uv/bios_uv.c b/arch/x86/platform/uv/bios_uv.c
index b4d5e95..d1f7422 100644
--- a/arch/x86/platform/uv/bios_uv.c
+++ b/arch/x86/platform/uv/bios_uv.c
@@ -40,7 +40,15 @@ s64 uv_bios_call(enum uv_bios_cmd which, u64 a1, u64 a2, u64 a3, u64 a4, u64 a5)
*/
return BIOS_STATUS_UNIMPLEMENTED;
- ret = efi_call_virt_pointer(tab, function, (u64)which, a1, a2, a3, a4, a5);
+ /*
+ * If EFI_OLD_MEMMAP is set, we need to fall back to using our old EFI
+ * callback method, which uses efi_call directly, with the kernel page tables.
+ */
+ if (unlikely(test_bit(EFI_OLD_MEMMAP, &efi.flags)))
+ ret = efi_call((void *)__va(tab->function), (u64)which, a1, a2, a3, a4, a5);
+ else
+ ret = efi_call_virt_pointer(tab, function, (u64)which, a1, a2, a3, a4, a5);
+
return ret;
}
EXPORT_SYMBOL_GPL(uv_bios_call);
--
1.8.5.6
On Wed, 19 Oct, at 08:48:51PM, Alex Thorlton wrote:
> Some time ago, we brought our UV BIOS callback code up to speed with the
> new EFI memory mapping scheme, in commit:
>
> d1be84a232e3 ("x86/uv: Update uv_bios_call() to use efi_call_virt_pointer()")
>
> By leveraging some changes that I made to a few of the EFI runtime
> callback mechanisms, in commit:
>
> 80e75596079f ("efi: Convert efi_call_virt() to efi_call_virt_pointer()")
>
> This got everything running smoothly on UV, with the new EFI mapping
> code. However, this left one, small loose end, in that EFI_OLD_MEMMAP
> (a.k.a. efi=old_map) will no longer work on UV, on kernels that include
> the aforementioned changes.
>
> At the time this was not a major issue (in fact, it still really isn't),
> but there's no reason that EFI_OLD_MEMMAP *shouldn't* work on our
> systems. This commit adds a check into uv_bios_call, to see if we have
> the EFI_OLD_MEMMAP bit set in efi.flags. If it is set, we fall back to
> using our old callback method, which uses efi_call directly on the __va
> of our function pointer.
>
> v2: Invert if-statement and add unlikely() hint.
>
> Signed-off-by: Alex Thorlton <[email protected]>
> Cc: Mike Travis <[email protected]>
> Cc: Russ Anderson <[email protected]>
> Cc: Dimitri Sivanich <[email protected]>
> Cc: Thomas Gleixner <[email protected]>
> Cc: Ingo Molnar <[email protected]>
> Cc: "H. Peter Anvin" <[email protected]>
> Cc: Matt Fleming <[email protected]>
> Cc: Masahiro Yamada <[email protected]>
> Cc: [email protected]
> ---
> arch/x86/platform/uv/bios_uv.c | 10 +++++++++-
> 1 file changed, 9 insertions(+), 1 deletion(-)
Looks fine to me. Applied to 'next'. Thanks!
Commit-ID: caef78b6cdeddf4ad364f95910bba6b43b8eb9bf
Gitweb: http://git.kernel.org/tip/caef78b6cdeddf4ad364f95910bba6b43b8eb9bf
Author: Alex Thorlton <[email protected]>
AuthorDate: Wed, 19 Oct 2016 20:48:51 -0500
Committer: Ingo Molnar <[email protected]>
CommitDate: Thu, 20 Oct 2016 08:47:58 +0200
x86/platform/UV: Fix support for EFI_OLD_MEMMAP after BIOS callback updates
Some time ago, we brought our UV BIOS callback code up to speed with the
new EFI memory mapping scheme, in commit:
d1be84a232e3 ("x86/uv: Update uv_bios_call() to use efi_call_virt_pointer()")
By leveraging some changes that I made to a few of the EFI runtime
callback mechanisms, in commit:
80e75596079f ("efi: Convert efi_call_virt() to efi_call_virt_pointer()")
This got everything running smoothly on UV, with the new EFI mapping
code. However, this left one, small loose end, in that EFI_OLD_MEMMAP
(a.k.a. efi=old_map) will no longer work on UV, on kernels that include
the aforementioned changes.
At the time this was not a major issue (in fact, it still really isn't),
but there's no reason that EFI_OLD_MEMMAP *shouldn't* work on our
systems. This commit adds a check into uv_bios_call(), to see if we have
the EFI_OLD_MEMMAP bit set in efi.flags. If it is set, we fall back to
using our old callback method, which uses efi_call() directly on the __va()
of our function pointer.
Signed-off-by: Alex Thorlton <[email protected]>
Acked-by: Matt Fleming <[email protected]>
Cc: <[email protected]> # v4.7 and later
Cc: Andy Lutomirski <[email protected]>
Cc: Ard Biesheuvel <[email protected]>
Cc: Borislav Petkov <[email protected]>
Cc: Brian Gerst <[email protected]>
Cc: Denys Vlasenko <[email protected]>
Cc: Dimitri Sivanich <[email protected]>
Cc: H. Peter Anvin <[email protected]>
Cc: Josh Poimboeuf <[email protected]>
Cc: Linus Torvalds <[email protected]>
Cc: Masahiro Yamada <[email protected]>
Cc: Mike Travis <[email protected]>
Cc: Peter Zijlstra <[email protected]>
Cc: Russ Anderson <[email protected]>
Cc: Thomas Gleixner <[email protected]>
Cc: [email protected]
Link: http://lkml.kernel.org/r/[email protected]
Signed-off-by: Ingo Molnar <[email protected]>
---
arch/x86/platform/uv/bios_uv.c | 10 +++++++++-
1 file changed, 9 insertions(+), 1 deletion(-)
diff --git a/arch/x86/platform/uv/bios_uv.c b/arch/x86/platform/uv/bios_uv.c
index b4d5e95..4a6a5a2 100644
--- a/arch/x86/platform/uv/bios_uv.c
+++ b/arch/x86/platform/uv/bios_uv.c
@@ -40,7 +40,15 @@ s64 uv_bios_call(enum uv_bios_cmd which, u64 a1, u64 a2, u64 a3, u64 a4, u64 a5)
*/
return BIOS_STATUS_UNIMPLEMENTED;
- ret = efi_call_virt_pointer(tab, function, (u64)which, a1, a2, a3, a4, a5);
+ /*
+ * If EFI_OLD_MEMMAP is set, we need to fall back to using our old EFI
+ * callback method, which uses efi_call() directly, with the kernel page tables:
+ */
+ if (unlikely(test_bit(EFI_OLD_MEMMAP, &efi.flags)))
+ ret = efi_call((void *)__va(tab->function), (u64)which, a1, a2, a3, a4, a5);
+ else
+ ret = efi_call_virt_pointer(tab, function, (u64)which, a1, a2, a3, a4, a5);
+
return ret;
}
EXPORT_SYMBOL_GPL(uv_bios_call);
FYI, we noticed a -2.3% regression of will-it-scale.per_thread_ops due to commit:
commit 71854cb812ec23bfe5f63d52217e6b9e6cb901f5 ("x86/platform/UV: Fix support for EFI_OLD_MEMMAP after BIOS callback updates")
https://github.com/0day-ci/linux Alex-Thorlton/x86-platform-UV-Fix-support-for-EFI_OLD_MEMMAP-after-BIOS-callback-updates/20161020-095215
in testcase: will-it-scale
on test machine: 12 threads Intel(R) Core(TM) i7 CPU X 980 @ 3.33GHz with 6G memory
with following parameters:
test: read2
cpufreq_governor: performance
Will It Scale takes a testcase and runs it from 1 through to n parallel copies to see if the testcase will scale. It builds both a process and threads based test in order to see any differences between the two.
Disclaimer:
Results have been estimated based on internal Intel analysis and are provided
for informational purposes only. Any difference in system hardware or software
design or configuration may affect actual performance.
Details are as below:
-------------------------------------------------------------------------------------------------->
To reproduce:
git clone git://git.kernel.org/pub/scm/linux/kernel/git/wfg/lkp-tests.git
cd lkp-tests
bin/lkp install job.yaml # job file is attached in this email
bin/lkp run job.yaml
=========================================================================================
compiler/cpufreq_governor/kconfig/rootfs/tbox_group/test/testcase:
gcc-6/performance/x86_64-rhel-7.2/debian-x86_64-2016-08-31.cgz/wsm/read2/will-it-scale
commit:
v4.9-rc1
71854cb812 ("x86/platform/UV: Fix support for EFI_OLD_MEMMAP after BIOS callback updates")
v4.9-rc1 71854cb812ec23bfe5f63d5221
---------------- --------------------------
fail:runs %reproduction fail:runs
| | |
%stddev %change %stddev
\ | \
1646359 ? 0% -2.3% 1608758 ? 1% will-it-scale.per_thread_ops
981.11 ? 0% +2.3% 1003 ? 0% will-it-scale.time.system_time
81.90 ? 0% -27.5% 59.34 ? 0% will-it-scale.time.user_time
4679 ? 88% -43.4% 2649 ? 7% cpuidle.C1E-NHM.usage
76734 ? 21% -22.4% 59544 ? 4% softirqs.RCU
0.29 ? 2% +32.2% 0.38 ? 2% perf-stat.branch-miss-rate%
2.953e+09 ? 2% +31.1% 3.871e+09 ? 2% perf-stat.branch-misses
183791 ? 10% +15.1% 211493 ? 7% sched_debug.cpu.avg_idle.stddev
0.00 ? 11% +24.6% 0.00 ? 3% sched_debug.cpu.next_balance.stddev
13311 ? 12% +23.7% 16466 ? 1% sched_debug.cpu.ttwu_count.min
will-it-scale.time.user_time
85 ++---------------------------------------------------------------------+
| .*.*.. .*..*. .*..*. .*.. .*..*. .* |
80 *+ *.*..* *..* *..*..* * *..*.*. |
| |
| |
75 ++ |
| |
70 ++ |
| |
65 ++ |
| |
| O O |
60 O+ O O O O O O O O O O O O O O O O O O
| O O O O O O O O |
55 ++---------------------------------------------------------------------+
will-it-scale.time.system_time
1010 ++-------------------------------------------------------------------+
| |
1005 ++ O O O O O O O O O |
O O O O O O O O O O O O O O O O O O
| O O |
1000 ++ |
| |
995 ++ |
| |
990 ++ |
| |
| |
985 ++ |
*. .*..*.*.. .*.*.. .*.*..*. .*.. .*.*..*. |
980 ++*--*---------*-*-------*-*---------*----*-*---------*--------------+
[*] bisect-good sample
[O] bisect-bad sample
Thanks,
Xiaolong
On Tue, 25 Oct 2016, kernel test robot wrote:
> FYI, we noticed a -2.3% regression of will-it-scale.per_thread_ops due to commit:
>
> commit 71854cb812ec23bfe5f63d52217e6b9e6cb901f5 ("x86/platform/UV: Fix support for EFI_OLD_MEMMAP after BIOS callback updates")
> https://github.com/0day-ci/linux Alex-Thorlton/x86-platform-UV-Fix-support-for-EFI_OLD_MEMMAP-after-BIOS-callback-updates/20161020-095215
>
> in testcase: will-it-scale
> on test machine: 12 threads Intel(R) Core(TM) i7 CPU X 980 @ 3.33GHz with 6G memory
This is completely bogus. That patch does not even affect anything outside
of the SGI UV platform. And on your i7 system uv_bios_call() is definitely
not invoked.
I appreciate your effort, but posting such obviously bogus results does not
make people more confident in your testing efforts.
Thanks,
tglx
On 10/25, Thomas Gleixner wrote:
>On Tue, 25 Oct 2016, kernel test robot wrote:
>> FYI, we noticed a -2.3% regression of will-it-scale.per_thread_ops due to commit:
>>
>> commit 71854cb812ec23bfe5f63d52217e6b9e6cb901f5 ("x86/platform/UV: Fix support for EFI_OLD_MEMMAP after BIOS callback updates")
>> https://github.com/0day-ci/linux Alex-Thorlton/x86-platform-UV-Fix-support-for-EFI_OLD_MEMMAP-after-BIOS-callback-updates/20161020-095215
>>
>> in testcase: will-it-scale
>> on test machine: 12 threads Intel(R) Core(TM) i7 CPU X 980 @ 3.33GHz with 6G memory
>
>This is completely bogus. That patch does not even affect anything outside
>of the SGI UV platform. And on your i7 system uv_bios_call() is definitely
>not invoked.
Yes, this is weird, the per_thread_ops change is small and should be run
to run variation, the actual significant change is will-it-scale.time.user_time
-27% decrease, but the patch seems not relevant, we can't interpret it. :(
We've tried to queue the jobs (4 times) for 71854cb812ec23bfe5f63d52217e6b9e6cb901f5 and v4.9-rc1
with new kconfig (added CONFIG_DEBUG_INFO_REDUCED), and result shows
user_time change is quite stable.
=========================================================================================
compiler/cpufreq_governor/kconfig/rootfs/tbox_group/test/testcase:
gcc-6/performance/x86_64-rhel-7.2+CONFIG_DEBUG_INFO_REDUCED/debian-x86_64-2016-08-31.cgz/wsm/read2/will-it-scale
commit:
v4.9-rc1
71854cb812ec23bfe5f63d52217e6b9e6cb901f5
v4.9-rc1 71854cb812ec23bfe5f63d5221
---------------- --------------------------
%stddev %change %stddev
\ | \
1670068 ? 0% -3.8% 1606650 ? 1% will-it-scale.per_thread_ops
9749 ? 2% +1328.0% 139222 ?105% will-it-scale.time.involuntary_context_switches
981.29 ? 0% +2.2% 1002 ? 0% will-it-scale.time.system_time
81.78 ? 0% -26.9% 59.74 ? 0% will-it-scale.time.user_time
32894 ? 0% -3.1% 31863 ? 2% vmstat.system.cs
9749 ? 2% +1328.0% 139222 ?105% time.involuntary_context_switches
380917 ? 2% -10.2% 341970 ? 3% sched_debug.cpu.avg_idle.avg
89166 ? 33% -73.4% 23731 ? 29% sched_debug.cpu.avg_idle.min
16.38 ? 10% -32.3% 11.08 ? 18% sched_debug.cpu.nr_uninterruptible.max
0.29 ? 1% +32.6% 0.38 ? 1% perf-stat.branch-miss-rate%
2.897e+09 ? 1% +33.5% 3.867e+09 ? 2% perf-stat.branch-misses
10084878 ? 0% -3.2% 9761852 ? 2% perf-stat.context-switches
0.00 ? 7% -9.3% 0.00 ? 1% perf-stat.dTLB-store-miss-rate%
33489012 ? 7% -9.2% 30416429 ? 1% perf-stat.dTLB-store-misses
Thanks,
Xiaolong
>
>I appreciate your effort, but posting such obviously bogus results does not
>make people more confident in your testing efforts.
>
>Thanks,
>
> tglx
On Thu, 27 Oct 2016, Ye Xiaolong wrote:
> Yes, this is weird, the per_thread_ops change is small and should be run
> to run variation, the actual significant change is will-it-scale.time.user_time
> -27% decrease, but the patch seems not relevant, we can't interpret it. :(
>
> We've tried to queue the jobs (4 times) for
> 71854cb812ec23bfe5f63d52217e6b9e6cb901f5 and v4.9-rc1 with new kconfig
> (added CONFIG_DEBUG_INFO_REDUCED), and result shows user_time change is
> quite stable.
> v4.9-rc1 71854cb812ec23bfe5f63d5221
> ---------------- --------------------------
> %stddev %change %stddev
> \ | \
> 1670068 ? 0% -3.8% 1606650 ? 1% will-it-scale.per_thread_ops
> 9749 ? 2% +1328.0% 139222 ?105% will-it-scale.time.involuntary_context_switches
^^^^^^ This is massive
I have no explanation for this either, but you really should try to figure
out what's going on here.
The only difference between plain rc1 and rc + this patch is the resulting
text size and therefor some other unrelated stuff moving to different
places in memory which has some yet to figure out side effects.
Thanks,
tglx
Hi Thomas,
It's been a big challenge that we'll occasionally run into such bisect
whose data show clear changes, however cannot be easily explained by
looking at the code logic.
On Fri, Oct 28, 2016 at 12:37:45AM +0200, Thomas Gleixner wrote:
>On Thu, 27 Oct 2016, Ye Xiaolong wrote:
>> Yes, this is weird, the per_thread_ops change is small and should be run
>> to run variation, the actual significant change is will-it-scale.time.user_time
>> -27% decrease, but the patch seems not relevant, we can't interpret it. :(
>>
>> We've tried to queue the jobs (4 times) for
>> 71854cb812ec23bfe5f63d52217e6b9e6cb901f5 and v4.9-rc1 with new kconfig
>> (added CONFIG_DEBUG_INFO_REDUCED), and result shows user_time change is
>> quite stable.
>
>> v4.9-rc1 71854cb812ec23bfe5f63d5221
>> ---------------- --------------------------
>> %stddev %change %stddev
>> \ | \
>> 1670068 ± 0% -3.8% 1606650 ± 1% will-it-scale.per_thread_ops
>> 9749 ± 2% +1328.0% 139222 ±105% will-it-scale.time.involuntary_context_switches
>
> ^^^^^^ This is massive
>
>I have no explanation for this either, but you really should try to figure
>out what's going on here.
Xiaolong, how about doing a small debug patch (a WARN_ONCE() line may
be enough) to verify whether the code path is executed?
It'd also help to compare vmlinux according to Thomas' reasoning:
>The only difference between plain rc1 and rc + this patch is the resulting
>text size and therefor some other unrelated stuff moving to different
>places in memory which has some yet to figure out side effects.
Yeah that's possible.
>From bisect POV, the below graphs show the user_time and system_time
are clearly and consistently different before/after commit 71854cb812.
So this commit must impacted something.
Legend:
[*] bisect good samples (eg. tests run on commits before 71854cb812)
[o] bisect bad samples (eg. tests run on commits since 71854cb812)
will-it-scale.time.user_time
85 ++---------------------------------------------------------------------+
| .*.*.. .*..*. .*..*. .*.. .*..*. .* |
80 *+ *.*..* *..* *..*..* * *..*.*. |
| |
| |
75 ++ |
| |
70 ++ |
| |
65 ++ |
| |
| O O |
60 O+ O O O O O O O O O O O O O O O O O O
| O O O O O O O O |
55 ++---------------------------------------------------------------------+
will-it-scale.time.system_time
1010 ++-------------------------------------------------------------------+
| |
1005 ++ O O O O O O O O O |
O O O O O O O O O O O O O O O O O O
| O O |
1000 ++ |
| |
995 ++ |
| |
990 ++ |
| |
| |
985 ++ |
*. .*..*.*.. .*.*.. .*.*..*. .*.. .*.*..*. |
980 ++*--*---------*-*-------*-*---------*----*-*---------*--------------+
The voluntary_context_switches increase looks obvious, too, though not
as consistent:
will-it-scale.time.voluntary_context_switches
160000 ++-----------------------------------------------------------------+
| O |
150000 ++ |
| |
140000 ++ O O O |
| O O O |
130000 ++ O |
| O O O |
120000 ++ O .* .*.. O .* |
O .O.*. O+ .*..* O .*.*. +O O O O |
110000 *+*. *. .*.* *. .*..* *.. O O O O
| *..*.*. * O O * |
100000 ++ O |
| O |
90000 ++-----------------------------------------------------------------+
So do the branch misses:
perf-stat.branch-misses
6.5e+09 ++----------------------------------------------------------------+
| |
6e+09 ++ O O |
5.5e+09 ++ |
| |
5e+09 ++ |
O O O |
4.5e+09 ++ O O O O |
| |
4e+09 ++ O O O O O O O O O O O O
3.5e+09 ++ |
| O O |
3e+09 *+*.. .*.*..*.*.*..*.O O .*.*..O.O. .*.* |
| *.*.*. O O *.*..* *..*.*. |
2.5e+09 ++----------------------------------------------------------------+
perf-stat.branch-miss-rate%
0.6 ++------------------O------------------------------------------------+
| O |
0.55 ++ |
| |
0.5 ++ |
O O O O |
0.45 ++ O O O |
| |
0.4 ++ O O O O |
| O O O O O O O O
0.35 ++ |
| O |
0.3 *+*.. .*..*.*..*.*..*.O O .*.O.O..O. .*.* |
| *.*..* O O *..*.*. *..*.*. |
0.25 ++-------------------------------------------------------------------+
Regards,
Fengguang
On Mon, 31 Oct 2016, Fengguang Wu wrote:
> On Fri, Oct 28, 2016 at 12:37:45AM +0200, Thomas Gleixner wrote:
> > I have no explanation for this either, but you really should try to figure
> > out what's going on here.
>
> Xiaolong, how about doing a small debug patch (a WARN_ONCE() line may
> be enough) to verify whether the code path is executed?
>
> It'd also help to compare vmlinux according to Thomas' reasoning:
>
> > The only difference between plain rc1 and rc + this patch is the resulting
> > text size and therefor some other unrelated stuff moving to different
> > places in memory which has some yet to figure out side effects.
So one other thing you might try is using rc3 as a base line and then
revert the patch in question on top of rc3 and see whether that weird
behaviour persists.
> > From bisect POV, the below graphs show the user_time and system_time
> are clearly and consistently different before/after commit 71854cb812.
> So this commit must impacted something.
I agree. I just have no idea what happens.
Thanks,
tglx