[PATCH 3/3] domaincapsmoc.c: mock virHostCPUGetMicrocodeVersion()

Daniel Henrique Barboza posted 3 patches 5 years, 5 months ago
[PATCH 3/3] domaincapsmoc.c: mock virHostCPUGetMicrocodeVersion()
Posted by Daniel Henrique Barboza 5 years, 5 months ago
Previous patch handled the runtime case where a non-x86 host is
fetching /proc/cpuinfo data for a microcode info that we know
it doesn't exist. This change alone speeded everything by a
bit for non-x86, but there is at least one major culprit left.

qemuxml2argvtest does several arch-specific tests, and a good
chunk of them are x86 exclusive. This means that 'hostArch'
will be seen as x86 for these tests, even when running in
non-x86 hosts. In a Power 9 server with 128 CPUs, qemuxmlargvtest
takes 298 seconds to complete in average, and 'perf record'
indicates that 95% of the time is spent in
virHostCPUGetMicrocodeVersion().

This patch mocks virHostCPUGetMicrocodeVersion() to always return
0 in the tests, avoiding /proc/cpuinfo reads. This will make all
tests behave arch-agnostic, and the microcode value being 0 has no
impact on any existing test.

This is a CI speed across the board for all archs, including x86,
given that we're not reading /proc/cpuinfo in the tests. For
a Thinkpad T480 laptop with 8 Intel i7 CPUs, qemuxml2argvtest
went from 15.50 sec to 12.50 seconds. The performance gain is even
more noticeable for huge servers with lots of CPUs. For the
Power 9 server mentioned above, this patch speeds qemuxml2argvtest
to 9 seconds, down from 298 sec.

Signed-off-by: Daniel Henrique Barboza <danielhb413@gmail.com>
---
 src/util/virhostcpu.h  | 3 ++-
 tests/domaincapsmock.c | 6 ++++++
 2 files changed, 8 insertions(+), 1 deletion(-)

diff --git a/src/util/virhostcpu.h b/src/util/virhostcpu.h
index 11cbcd72a3..6148f9e66f 100644
--- a/src/util/virhostcpu.h
+++ b/src/util/virhostcpu.h
@@ -75,7 +75,8 @@ virBitmapPtr virHostCPUGetSiblingsList(unsigned int cpu);
 
 int virHostCPUGetOnline(unsigned int cpu, bool *online);
 
-unsigned int virHostCPUGetMicrocodeVersion(virArch hostArch);
+unsigned int
+virHostCPUGetMicrocodeVersion(virArch hostArch) G_GNUC_NO_INLINE;
 
 int virHostCPUGetMSR(unsigned long index,
                      uint64_t *msr);
diff --git a/tests/domaincapsmock.c b/tests/domaincapsmock.c
index 90e17c19f0..d81a898dc0 100644
--- a/tests/domaincapsmock.c
+++ b/tests/domaincapsmock.c
@@ -34,3 +34,9 @@ virHostCPUGetKVMMaxVCPUs(void)
 {
     return INT_MAX;
 }
+
+unsigned int
+virHostCPUGetMicrocodeVersion(virArch hostArch G_GNUC_UNUSED)
+{
+    return 0;
+}
-- 
2.26.2

Re: [PATCH 3/3] domaincapsmoc.c: mock virHostCPUGetMicrocodeVersion()
Posted by Ján Tomko 5 years, 5 months ago
s/capsmoc.c/capsmock/

On a Monday in 2020, Daniel Henrique Barboza wrote:
>Previous patch handled the runtime case where a non-x86 host is
>fetching /proc/cpuinfo data for a microcode info that we know
>it doesn't exist. This change alone speeded everything by a
>bit for non-x86, but there is at least one major culprit left.
>
>qemuxml2argvtest does several arch-specific tests, and a good
>chunk of them are x86 exclusive. This means that 'hostArch'
>will be seen as x86 for these tests, even when running in
>non-x86 hosts. In a Power 9 server with 128 CPUs, qemuxmlargvtest

*qemuxml2argvtest

>takes 298 seconds to complete in average, and 'perf record'
>indicates that 95% of the time is spent in
>virHostCPUGetMicrocodeVersion().
>

No surprise, we call it 23448 times.

>This patch mocks virHostCPUGetMicrocodeVersion() to always return
>0 in the tests, avoiding /proc/cpuinfo reads. This will make all
>tests behave arch-agnostic, and the microcode value being 0 has no
>impact on any existing test.
>
>This is a CI speed across the board for all archs, including x86,
>given that we're not reading /proc/cpuinfo in the tests. For
>a Thinkpad T480 laptop with 8 Intel i7 CPUs, qemuxml2argvtest
>went from 15.50 sec to 12.50 seconds. The performance gain is even
>more noticeable for huge servers with lots of CPUs. For the
>Power 9 server mentioned above, this patch speeds qemuxml2argvtest
>to 9 seconds, down from 298 sec.
>

I assume the speedup for Power is measured for the whole series, not
just for this patch.

>Signed-off-by: Daniel Henrique Barboza <danielhb413@gmail.com>
>---
> src/util/virhostcpu.h  | 3 ++-
> tests/domaincapsmock.c | 6 ++++++
> 2 files changed, 8 insertions(+), 1 deletion(-)
>

Reviewed-by: Ján Tomko <jtomko@redhat.com>

Jano
Re: [PATCH 3/3] domaincapsmoc.c: mock virHostCPUGetMicrocodeVersion()
Posted by Daniel Henrique Barboza 5 years, 5 months ago

On 8/24/20 11:36 AM, Ján Tomko wrote:
> s/capsmoc.c/capsmock/
> 
> On a Monday in 2020, Daniel Henrique Barboza wrote:
>> Previous patch handled the runtime case where a non-x86 host is
>> fetching /proc/cpuinfo data for a microcode info that we know
>> it doesn't exist. This change alone speeded everything by a
>> bit for non-x86, but there is at least one major culprit left.
>>
>> qemuxml2argvtest does several arch-specific tests, and a good
>> chunk of them are x86 exclusive. This means that 'hostArch'
>> will be seen as x86 for these tests, even when running in
>> non-x86 hosts. In a Power 9 server with 128 CPUs, qemuxmlargvtest
> 
> *qemuxml2argvtest
> 
>> takes 298 seconds to complete in average, and 'perf record'
>> indicates that 95% of the time is spent in
>> virHostCPUGetMicrocodeVersion().
>>
> 
> No surprise, we call it 23448 times.
> 
>> This patch mocks virHostCPUGetMicrocodeVersion() to always return
>> 0 in the tests, avoiding /proc/cpuinfo reads. This will make all
>> tests behave arch-agnostic, and the microcode value being 0 has no
>> impact on any existing test.
>>
>> This is a CI speed across the board for all archs, including x86,
>> given that we're not reading /proc/cpuinfo in the tests. For
>> a Thinkpad T480 laptop with 8 Intel i7 CPUs, qemuxml2argvtest
>> went from 15.50 sec to 12.50 seconds. The performance gain is even
>> more noticeable for huge servers with lots of CPUs. For the
>> Power 9 server mentioned above, this patch speeds qemuxml2argvtest
>> to 9 seconds, down from 298 sec.
>>
> 
> I assume the speedup for Power is measured for the whole series, not
> just for this patch.


Yes, for the whole series, for both Power and my x86 laptop. Should've make
it clearer in the commit msg.


Thanks,


DHB


> 
>> Signed-off-by: Daniel Henrique Barboza <danielhb413@gmail.com>
>> ---
>> src/util/virhostcpu.h  | 3 ++-
>> tests/domaincapsmock.c | 6 ++++++
>> 2 files changed, 8 insertions(+), 1 deletion(-)
>>
> 
> Reviewed-by: Ján Tomko <jtomko@redhat.com>
> 
> Jano