[Qemu-devel] [RFC PATCH 0/3] Series short description

[Qemu-devel] [RFC PATCH 0/3] Series short description

[Dario Faggioli]

The following series implements...

---

Dario Faggioli (3):
      i386: add properties for customizing L2 and L3 caches size
      i386: custom cache size in CPUID2 and CPUID4 descriptors
      i386: custom cache size in AMD's CPUID descriptors too


 0 files changed

--
Signature

[Qemu-devel] [RFC PATCH 1/3] i386: add properties for customizing L2 and L3 caches size

[Dario Faggioli]

Make it possible to specify a custom size for the L2 and
L3 caches, from the command line.

This can be useful in cases where applications or libraries
check, within the guest, the cache size and behave differently
depending on what they actually see.

Signed-off-by: Dario Faggioli <dfaggioli@suse.com>
---
I am not entirely sure I got the include/hw/i386 bits right (i.e.,
whether I should include the new properties in PC_COMPAT_3_0 and, if
yes, if the stanzas are correct). I'll dig further (and accept any
help/advice :-D )
---
Cc: "Michael S. Tsirkin" <mst@redhat.com>
Cc: Marcel Apfelbaum <marcel.apfelbaum@gmail.com>
Cc: Paolo Bonzini <pbonzini@redhat.com>
Cc: Richard Henderson <rth@twiddle.net>
Cc: Eduardo Habkost <ehabkost@redhat.com>
---
 0 files changed

diff --git a/include/hw/i386/pc.h b/include/hw/i386/pc.h
index 136fe497b6..1094bba68c 100644
--- a/include/hw/i386/pc.h
+++ b/include/hw/i386/pc.h
@@ -308,6 +308,14 @@ bool e820_get_entry(int, uint32_t, uint64_t *, uint64_t *);
         .driver   = "Skylake-Server-IBRS" "-" TYPE_X86_CPU,\
         .property = "pku",\
         .value    = "off",\
+    },{\
+        .driver   = TYPE_X86_CPU,\
+        .property = "l3-cache-size",\
+        .value    = "off",\
+    },{\
+        .driver   = TYPE_X86_CPU,\
+        .property = "l2-cache-size",\
+        .value    = "off",\
     },
 
 #define PC_COMPAT_2_12 \
diff --git a/target/i386/cpu.c b/target/i386/cpu.c
index f81d35e1f9..b8ccb2be04 100644
--- a/target/i386/cpu.c
+++ b/target/i386/cpu.c
@@ -5778,6 +5778,14 @@ static Property x86_cpu_properties[] = {
     DEFINE_PROP_INT32("x-hv-max-vps", X86CPU, hv_max_vps, -1),
     DEFINE_PROP_BOOL("x-hv-synic-kvm-only", X86CPU, hyperv_synic_kvm_only,
                      false),
+
+    /*
+     * Custom size for L2 and/or L3 cache. Default (0) means we use the
+     * default value for the CPU.
+     */
+    DEFINE_PROP_SIZE("l2-cache-size", X86CPU, l2_cache_size, 0),
+    DEFINE_PROP_SIZE("l3-cache-size", X86CPU, l3_cache_size, 0),
+
     DEFINE_PROP_END_OF_LIST()
 };
 
diff --git a/target/i386/cpu.h b/target/i386/cpu.h
index 9c52d0cbeb..ba0b913448 100644
--- a/target/i386/cpu.h
+++ b/target/i386/cpu.h
@@ -1476,6 +1476,9 @@ struct X86CPU {
     int32_t core_id;
     int32_t thread_id;
 
+    uint64_t l2_cache_size;
+    uint64_t l3_cache_size;
+
     int32_t hv_max_vps;
 };
 

[Qemu-devel] [RFC PATCH 2/3] i386: custom cache size in CPUID2 and CPUID4 descriptors

[Dario Faggioli]

If specified on the command line, alter the cache(s) properties
accordingly, before encoding them in the CPUID descriptors.

Tweak the number of sets (if defined), to retain consistency.

Unless some specific size values are used (either by chance
or voluntarily), we won't find any matching CPUID-2 descriptor,
and 0xFF will be used. This shouldn't be a problem, as we have
CPUID-4.

Signed-off-by: Dario Faggioli <dfaggioli@suse.com>
---
I'm no CPUID expert. I'm not sure I've fully understodd the relationship
between CPUID-2 and CPUID-4. The solution implemented here, is the best
I could come up with, and it worked on all the CPU types that I've tried.
If it's wrong/suboptimal, I'm happy to think to something else/rework.
---
Cc: Paolo Bonzini <pbonzini@redhat.com>
Cc: Richard Henderson <rth@twiddle.net>
Cc: Eduardo Habkost <ehabkost@redhat.com>
---
 0 files changed

diff --git a/target/i386/cpu.c b/target/i386/cpu.c
index b8ccb2be04..17aff19561 100644
--- a/target/i386/cpu.c
+++ b/target/i386/cpu.c
@@ -426,6 +426,24 @@ static void encode_cache_cpuid8000001d(CPUCacheInfo *cache, CPUState *cs,
            (cache->complex_indexing ? CACHE_COMPLEX_IDX : 0);
 }
 
+static void set_custom_cache_size(CPUCacheInfo *c, uint64_t sz)
+{
+    /*
+     * Descriptors that have 'sets', also have 'partitions' initialized,
+     * so we can compute the new number of sets. For others, just tweak the
+     * size.
+     */
+    assert(c->partitions > 0 || c->sets == 0);
+    if (c->sets > 0) {
+        uint32_t sets = sz / (c->line_size * c->associativity * c->partitions);
+
+        if (sets == 0)
+            return;
+        c->sets = sets;
+    }
+    c->size = sz;
+}
+
 /* Data structure to hold the configuration info for a given core index */
 struct core_topology {
     /* core complex id of the current core index */
@@ -4193,8 +4211,14 @@ void cpu_x86_cpuid(CPUX86State *env, uint32_t index, uint32_t count,
         if (!cpu->enable_l3_cache) {
             *ecx = 0;
         } else {
+            if (cpu->l3_cache_size > 0)
+                set_custom_cache_size(env->cache_info_cpuid2.l3_cache,
+                                      cpu->l3_cache_size);
             *ecx = cpuid2_cache_descriptor(env->cache_info_cpuid2.l3_cache);
         }
+        if (cpu->l2_cache_size > 0)
+            set_custom_cache_size(env->cache_info_cpuid2.l2_cache,
+                                  cpu->l2_cache_size);
         *edx = (cpuid2_cache_descriptor(env->cache_info_cpuid2.l1d_cache) << 16) |
                (cpuid2_cache_descriptor(env->cache_info_cpuid2.l1i_cache) <<  8) |
                (cpuid2_cache_descriptor(env->cache_info_cpuid2.l2_cache));
@@ -4222,6 +4246,9 @@ void cpu_x86_cpuid(CPUX86State *env, uint32_t index, uint32_t count,
                                     eax, ebx, ecx, edx);
                 break;
             case 2: /* L2 cache info */
+                if (cpu->l2_cache_size > 0)
+                    set_custom_cache_size(env->cache_info_cpuid4.l2_cache,
+                                          cpu->l2_cache_size);
                 encode_cache_cpuid4(env->cache_info_cpuid4.l2_cache,
                                     cs->nr_threads, cs->nr_cores,
                                     eax, ebx, ecx, edx);
@@ -4229,6 +4256,9 @@ void cpu_x86_cpuid(CPUX86State *env, uint32_t index, uint32_t count,
             case 3: /* L3 cache info */
                 pkg_offset = apicid_pkg_offset(cs->nr_cores, cs->nr_threads);
                 if (cpu->enable_l3_cache) {
+                    if (cpu->l3_cache_size > 0)
+                        set_custom_cache_size(env->cache_info_cpuid4.l3_cache,
+                                              cpu->l3_cache_size);
                     encode_cache_cpuid4(env->cache_info_cpuid4.l3_cache,
                                         (1 << pkg_offset), cs->nr_cores,
                                         eax, ebx, ecx, edx);

[Qemu-devel] [RFC PATCH 3/3] i386: custom cache size in AMD's CPUID descriptors too

[Dario Faggioli]

If specified on the command line, alter the cache size(s)
properties accordingly, before encoding them in the AMD's
CPUID cache descriptors too (i.e., 80000006 and 8000001d).

Signed-off-by: Dario Faggioli <dfaggioli@suse.com>
---
Cc: Paolo Bonzini <pbonzini@redhat.com>
Cc: Richard Henderson <rth@twiddle.net>
Cc: Eduardo Habkost <ehabkost@redhat.com>
---
 0 files changed

diff --git a/target/i386/cpu.c b/target/i386/cpu.c
index 17aff19561..4949d6b907 100644
--- a/target/i386/cpu.c
+++ b/target/i386/cpu.c
@@ -4490,6 +4490,12 @@ void cpu_x86_cpuid(CPUX86State *env, uint32_t index, uint32_t count,
                (L2_DTLB_4K_ENTRIES << 16) | \
                (AMD_ENC_ASSOC(L2_ITLB_4K_ASSOC) << 12) | \
                (L2_ITLB_4K_ENTRIES);
+        if (cpu->l2_cache_size > 0)
+            set_custom_cache_size(env->cache_info_amd.l2_cache,
+                                  cpu->l2_cache_size);
+        if (cpu->enable_l3_cache && cpu->l3_cache_size > 0)
+            set_custom_cache_size(env->cache_info_amd.l3_cache,
+                                  cpu->l3_cache_size);
         encode_cache_cpuid80000006(env->cache_info_amd.l2_cache,
                                    cpu->enable_l3_cache ?
                                    env->cache_info_amd.l3_cache : NULL,
@@ -4546,10 +4552,16 @@ void cpu_x86_cpuid(CPUX86State *env, uint32_t index, uint32_t count,
                                        eax, ebx, ecx, edx);
             break;
         case 2: /* L2 cache info */
+            if (cpu->l2_cache_size > 0)
+                set_custom_cache_size(env->cache_info_amd.l2_cache,
+                                      cpu->l2_cache_size * MiB);
             encode_cache_cpuid8000001d(env->cache_info_amd.l2_cache, cs,
                                        eax, ebx, ecx, edx);
             break;
         case 3: /* L3 cache info */
+            if (cpu->enable_l3_cache && cpu->l3_cache_size > 0)
+                set_custom_cache_size(env->cache_info_amd.l3_cache,
+                                      cpu->l3_cache_size * MiB);
             encode_cache_cpuid8000001d(env->cache_info_amd.l3_cache, cs,
                                        eax, ebx, ecx, edx);
             break;

Re: [Qemu-devel] [RFC PATCH 0/3] Series short description

[Dario Faggioli]

[-- Attachment #1: Type: text/plain, Size: 3566 bytes --]

Wow... Mmm, not sure what went wrong... Anyway, this is the cover
letter I thought I had sent. Sorry :-/
--
Hello everyone,

This is Dario, from SUSE, and this is the first time I touch QEMU. :-D

So, basically, while playing with an AMD EPYC box, we came across a weird
performance regression between host and guest. It was happening with the
STREAM benchmark, and we tracked it down to non-temporal stores _not_ being
used, inside the guest.

More specifically, this was because the glibc version we were dealing with had
heuristics for deciding whether or not to use NT instructions. Basically, it
was checking is how big the L2 and L3 caches are, as compared to how many
threads are actually sharing such caches.

Currently, as far as cache layout and size are concerned, we only have the
following options:
- no L3 cache,
- emulated L3 cache, which means the default cache layout for the chosen CPU
  is used,
- host L3 cache info, which means the cache layout of the host is used.

Now, in our case, 'host-cache-info' made sense, because we were pinning vcpus
as well as doing other optimizations. However, as the VM had _less_ vcpus than
the host had pcpus, the result of the heuristics was to avoid non-temporal
stores, causing the unexpectedly high drop in performance. And, as you can
imagine, we could not fix things by using 'l3-cache=on' either.

This made us think this could be a general problem, and not only an issue for
our benchmarks, and here it comes this series. :-)

Basically, while we can, of course, control the number of vcpus a guest has
already --as well as how they are arranged within the guest topology-- we can't
control how big are the caches the guest sees. And this is what this series
tries to implement: giving the user the ability to tweak the actual size of the
L2 and L3 caches, to deal with all those cases when the guest OS or userspace
do check that, and behave differently depending on what they see.

Yes, this is not at all that common, but happens, and hece the feature can
be considered useful, IMO. And yes, it is definitely something meant for those
cases where one is carefully tuning and highly optimizing, with things like
vcpu pinning, etc.

I've tested with many CPU models, and the cahce info from inside the guest
looks consistent. I haven't re-run the benchmarks that triggered all this work,
as I don't have the proper hardware handy right now, but I'm planning to
(although, as said, this looks like a general problem to me).

I've got libvirt patches for exposing these new properties in the works, but
of course they only make sense if/when this series is accepted.

As I said, it's my first submission, and it's RFC because there are a couple
of things that I'm not sure I got right (details in the single patches).

Any comment or advice more than welcome. :-)

Thanks and Regards,
Dario
---
Dario Faggioli (3):
      i386: add properties for customizing L2 and L3 cache sizes
      i386: custom cache size in CPUID2 and CPUID4 descriptors
      i386: custom cache size in AMD's CPUID descriptors too

 include/hw/i386/pc.h |    8 ++++++++
 target/i386/cpu.c    |   50 ++++++++++++++++++++++++++++++++++++++++++++++++++
 target/i386/cpu.h    |    3 +++
 3 files changed, 61 insertions(+)

-- 
<<This happens because I choose it to happen!>> (Raistlin Majere)
-----------------------------------------------------------------
Dario Faggioli, Ph.D, http://about.me/dario.faggioli
Software Engineer @ SUSE https://www.suse.com/

[-- Attachment #2: This is a digitally signed message part --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

Re: [Qemu-devel] [RFC PATCH 0/3] Series short description

[Daniel P. Berrangé]

On Wed, Nov 14, 2018 at 12:08:42PM +0100, Dario Faggioli wrote:
> Wow... Mmm, not sure what went wrong... Anyway, this is the cover
> letter I thought I had sent. Sorry :-/

No problem !

If you have not come across it before, "git-publish" is a great addon
tool for git to make sending patch series more pain-free

   https://github.com/stefanha/git-publish

QEMU git provides a config file for it, so you can just run "git-publish"
with no args and it will send a series containing all patches on your
currently checked out branch, automatically reading the MAINTAINERS file
to figure out who should be CC'd on the series.

The only global setup is for your $HOME/.gitconfig to contain a setting
for "smtpserver" under the "[sendemail]" group for outbound SMTP relay.

> --
> Hello everyone,
> 
> This is Dario, from SUSE, and this is the first time I touch QEMU. :-D

Welcome & thanks for your first patch(es) to QEMU.

I'll let others comment on the actual code you've sent...

> So, basically, while playing with an AMD EPYC box, we came across a weird
> performance regression between host and guest. It was happening with the
> STREAM benchmark, and we tracked it down to non-temporal stores _not_ being
> used, inside the guest.
> 
> More specifically, this was because the glibc version we were dealing with had
> heuristics for deciding whether or not to use NT instructions. Basically, it
> was checking is how big the L2 and L3 caches are, as compared to how many
> threads are actually sharing such caches.
> 
> Currently, as far as cache layout and size are concerned, we only have the
> following options:
> - no L3 cache,
> - emulated L3 cache, which means the default cache layout for the chosen CPU
>   is used,
> - host L3 cache info, which means the cache layout of the host is used.
> 
> Now, in our case, 'host-cache-info' made sense, because we were pinning vcpus
> as well as doing other optimizations. However, as the VM had _less_ vcpus than
> the host had pcpus, the result of the heuristics was to avoid non-temporal
> stores, causing the unexpectedly high drop in performance. And, as you can
> imagine, we could not fix things by using 'l3-cache=on' either.
> 
> This made us think this could be a general problem, and not only an issue for
> our benchmarks, and here it comes this series. :-)
> 
> Basically, while we can, of course, control the number of vcpus a guest has
> already --as well as how they are arranged within the guest topology-- we can't
> control how big are the caches the guest sees. And this is what this series
> tries to implement: giving the user the ability to tweak the actual size of the
> L2 and L3 caches, to deal with all those cases when the guest OS or userspace
> do check that, and behave differently depending on what they see.
> 
> Yes, this is not at all that common, but happens, and hece the feature can
> be considered useful, IMO. And yes, it is definitely something meant for those
> cases where one is carefully tuning and highly optimizing, with things like
> vcpu pinning, etc.
> 
> I've tested with many CPU models, and the cahce info from inside the guest
> looks consistent. I haven't re-run the benchmarks that triggered all this work,
> as I don't have the proper hardware handy right now, but I'm planning to
> (although, as said, this looks like a general problem to me).
> 
> I've got libvirt patches for exposing these new properties in the works, but
> of course they only make sense if/when this series is accepted.
> 
> As I said, it's my first submission, and it's RFC because there are a couple
> of things that I'm not sure I got right (details in the single patches).
> 
> Any comment or advice more than welcome. :-)
> 
> Thanks and Regards,
> Dario
> ---
> Dario Faggioli (3):
>       i386: add properties for customizing L2 and L3 cache sizes
>       i386: custom cache size in CPUID2 and CPUID4 descriptors
>       i386: custom cache size in AMD's CPUID descriptors too
> 
>  include/hw/i386/pc.h |    8 ++++++++
>  target/i386/cpu.c    |   50 ++++++++++++++++++++++++++++++++++++++++++++++++++
>  target/i386/cpu.h    |    3 +++
>  3 files changed, 61 insertions(+)
> 
> -- 
> <<This happens because I choose it to happen!>> (Raistlin Majere)
> -----------------------------------------------------------------
> Dario Faggioli, Ph.D, http://about.me/dario.faggioli
> Software Engineer @ SUSE https://www.suse.com/



Regards,
Daniel
-- 
|: https://berrange.com      -o-    https://www.flickr.com/photos/dberrange :|
|: https://libvirt.org         -o-            https://fstop138.berrange.com :|
|: https://entangle-photo.org    -o-    https://www.instagram.com/dberrange :|

Re: [Qemu-devel] [RFC PATCH 1/3] i386: add properties for customizing L2 and L3 caches size

[Eric Blake]

On 11/14/18 4:56 AM, Dario Faggioli wrote:
> Make it possible to specify a custom size for the L2 and
> L3 caches, from the command line.
> 
> This can be useful in cases where applications or libraries
> check, within the guest, the cache size and behave differently
> depending on what they actually see.
> 
> Signed-off-by: Dario Faggioli <dfaggioli@suse.com>
> ---
> I am not entirely sure I got the include/hw/i386 bits right (i.e.,
> whether I should include the new properties in PC_COMPAT_3_0 and, if
> yes, if the stanzas are correct). I'll dig further (and accept any
> help/advice :-D )
> ---
> Cc: "Michael S. Tsirkin" <mst@redhat.com>
> Cc: Marcel Apfelbaum <marcel.apfelbaum@gmail.com>
> Cc: Paolo Bonzini <pbonzini@redhat.com>
> Cc: Richard Henderson <rth@twiddle.net>
> Cc: Eduardo Habkost <ehabkost@redhat.com>
> ---
>   0 files changed

That's an odd diffstat. Why is git not giving you the normal diffstat 
with an actual summary of files changed?

-- 
Eric Blake, Principal Software Engineer
Red Hat, Inc.           +1-919-301-3266
Virtualization:  qemu.org | libvirt.org

Re: [Qemu-devel] [RFC PATCH 0/3] Series short description

[Dario Faggioli]

[-- Attachment #1: Type: text/plain, Size: 1133 bytes --]

On Wed, 2018-11-14 at 11:29 +0000, Daniel P. Berrangé wrote:
> On Wed, Nov 14, 2018 at 12:08:42PM +0100, Dario Faggioli wrote:
> > Wow... Mmm, not sure what went wrong... Anyway, this is the cover
> > letter I thought I had sent. Sorry :-/
> 
> No problem !
> 
Hello,

> If you have not come across it before, "git-publish" is a great addon
> tool for git to make sending patch series more pain-free
> 
>    https://github.com/stefanha/git-publish
> 
> [...]
> 
Yes, I've heard of it. I'm already planning to check if it works well
with stgit, which I also use (and wish to continue to :-) ).

If it does, I'll definitely start using it.

> > --
> > Hello everyone,
> > 
> > This is Dario, from SUSE, and this is the first time I touch QEMU.
> > :-D
> 
> Welcome & thanks for your first patch(es) to QEMU.
> 
Thanks for the warm welcome. :-D

Regards,
Dario
-- 
<<This happens because I choose it to happen!>> (Raistlin Majere)
-----------------------------------------------------------------
Dario Faggioli, Ph.D, http://about.me/dario.faggioli
Software Engineer @ SUSE https://www.suse.com/

[-- Attachment #2: This is a digitally signed message part --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

Re: [Qemu-devel] [RFC PATCH 1/3] i386: add properties for customizing L2 and L3 caches size

[Dario Faggioli]

[-- Attachment #1: Type: text/plain, Size: 1027 bytes --]

On Wed, 2018-11-14 at 08:14 -0600, Eric Blake wrote:
> On 11/14/18 4:56 AM, Dario Faggioli wrote:
> > ---
> >   0 files changed
> 
> That's an odd diffstat. Why is git not giving you the normal
> diffstat 
> with an actual summary of files changed?
> 
Ah, more weirdness about this submission. :-O

I've just tried re-sending the series to myself and, for this patch, I
do see a diffstat that makes sense:

 include/hw/i386/pc.h |    8 ++++++++
 target/i386/cpu.c    |    8 ++++++++
 target/i386/cpu.h    |    3 +++
 3 files changed, 19 insertions(+)

And same is true for all other series I've sent around in the same way
and with the same tool (I went double checking a couple of them! :-P).

So, clearly, something went wrong this time. :-/

Regards,
Dario
-- 
<<This happens because I choose it to happen!>> (Raistlin Majere)
-----------------------------------------------------------------
Dario Faggioli, Ph.D, http://about.me/dario.faggioli
Software Engineer @ SUSE https://www.suse.com/

[-- Attachment #2: This is a digitally signed message part --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

Re: [Qemu-devel] [RFC PATCH 0/3] Series short description

[no-reply]

Hi,

This series seems to have some coding style problems. See output below for
more information:

Message-id: 154219299016.19470.9372139354280787961.stgit@wayrath
Type: series
Subject: [Qemu-devel] [RFC PATCH 0/3] Series short description

=== TEST SCRIPT BEGIN ===
#!/bin/bash

BASE=base
n=1
total=$(git log --oneline $BASE.. | wc -l)
failed=0

git config --local diff.renamelimit 0
git config --local diff.renames True
git config --local diff.algorithm histogram

commits="$(git log --format=%H --reverse $BASE..)"
for c in $commits; do
    echo "Checking PATCH $n/$total: $(git log -n 1 --format=%s $c)..."
    if ! git show $c --format=email | ./scripts/checkpatch.pl --mailback -; then
        failed=1
        echo
    fi
    n=$((n+1))
done

exit $failed
=== TEST SCRIPT END ===

Updating 3c8cf5a9c21ff8782164d1def7f44bd888713384
Switched to a new branch 'test'
28cb7a9 i386: custom cache size in AMD's CPUID descriptors too
1708b37 i386: custom cache size in CPUID2 and CPUID4 descriptors
ed1b0e0 i386: add properties for customizing L2 and L3 caches size

=== OUTPUT BEGIN ===
Checking PATCH 1/3: i386: add properties for customizing L2 and L3 caches size...
Checking PATCH 2/3: i386: custom cache size in CPUID2 and CPUID4 descriptors...
ERROR: braces {} are necessary for all arms of this statement
#38: FILE: target/i386/cpu.c:440:
+        if (sets == 0)
[...]

total: 1 errors, 0 warnings, 56 lines checked

Your patch has style problems, please review.  If any of these errors
are false positives report them to the maintainer, see
CHECKPATCH in MAINTAINERS.

Checking PATCH 3/3: i386: custom cache size in AMD's CPUID descriptors too...
=== OUTPUT END ===

Test command exited with code: 1


---
Email generated automatically by Patchew [http://patchew.org/].
Please send your feedback to patchew-devel@redhat.com