Zhao Liu <zhao1.liu@xxxxxxxxx> writes:
> Hi Markus,
>
> I realized I should reply this mail first...
>
> On Wed, Jul 24, 2024 at 01:35:17PM +0200, Markus Armbruster wrote:
>> Date: Wed, 24 Jul 2024 13:35:17 +0200
>> From: Markus Armbruster <armbru@xxxxxxxxxx>
>> Subject: Re: [PATCH 2/8] qapi/qom: Introduce smp-cache object
>>
>> Zhao Liu <zhao1.liu@xxxxxxxxx> writes:
>>
>> > Hi Markus,
>> >
>> > On Mon, Jul 22, 2024 at 03:33:13PM +0200, Markus Armbruster wrote:
>> >> Date: Mon, 22 Jul 2024 15:33:13 +0200
>> >> From: Markus Armbruster <armbru@xxxxxxxxxx>
>> >> Subject: Re: [PATCH 2/8] qapi/qom: Introduce smp-cache object
>> >>
>> >> Zhao Liu <zhao1.liu@xxxxxxxxx> writes:
>> >>
>> >> > Introduce smp-cache object so that user could define cache properties.
>> >> >
>> >> > In smp-cache object, define cache topology based on CPU topology level
>> >> > with two reasons:
>> >> >
>> >> > 1. In practice, a cache will always be bound to the CPU container
>> >> > (either private in the CPU container or shared among multiple
>> >> > containers), and CPU container is often expressed in terms of CPU
>> >> > topology level.
>> >> > 2. The x86's cache-related CPUIDs encode cache topology based on APIC
>> >> > ID's CPU topology layout. And the ACPI PPTT table that ARM/RISCV
>> >> > relies on also requires CPU containers to help indicate the private
>> >> > shared hierarchy of the cache. Therefore, for SMP systems, it is
>> >> > natural to use the CPU topology hierarchy directly in QEMU to define
>> >> > the cache topology.
>> >> >
>> >> > Currently, separated L1 cache (L1 data cache and L1 instruction cache)
>> >> > with unified higher-level cache (e.g., unified L2 and L3 caches), is the
>> >> > most common cache architectures.
>> >> >
>> >> > Therefore, enumerate the L1 D-cache, L1 I-cache, L2 cache and L3 cache
>> >> > with smp-cache object to add the basic cache topology support.
>> >> >
>> >> > Suggested-by: Daniel P. Berrange <berrange@xxxxxxxxxx>
>> >> > Signed-off-by: Zhao Liu <zhao1.liu@xxxxxxxxx>
>> >>
>> >> [...]
>> >>
>> >> > diff --git a/qapi/machine-common.json b/qapi/machine-common.json
>> >> > index 82413c668bdb..8b8c0e9eeb86 100644
>> >> > --- a/qapi/machine-common.json
>> >> > +++ b/qapi/machine-common.json
>> >> > @@ -64,3 +64,53 @@
>> >> > 'prefix': 'CPU_TOPO_LEVEL',
>> >> > 'data': [ 'invalid', 'thread', 'core', 'module', 'cluster',
>> >> > 'die', 'socket', 'book', 'drawer', 'default' ] }
>> >> > +
>> >> > +##
>> >> > +# @SMPCacheName:
>> >>
>> >> Why the SMP in this name? Because it's currently only used by SMP
>> >> stuff? Or is there another reason I'm missing?
>> >
>> > Yes, I suppose it can only be used in SMP case.
>> >
>> > Because Intel's heterogeneous CPUs have different topologies for cache,
>> > for example, Alderlake's L2, for P core, L2 is per P-core, but for E
>> > core, L2 is per module (4 E cores per module). Thus I would like to keep
>> > the topology semantics of this object and -smp as consistent as possible.
>> >
>> > Do you agree?
>>
>> I don't know enough to meaningfully agree or disagree. I know just
>> enough to annoy you with questions :)
>
> Welcome and no problem!
>
>> This series adds a way to configure caches.
>>
>> Structure of the configuration data: a list
>>
>> [{"name": N, "topo": T}, ...]
>>
>> where N can be "l1d", "l1i", "l2", or "l3",
>> and T can be "invalid", "thread", "core", "module", "cluster",
>> "die", "socket", "book", "drawer", or "default".
>>
>> What's the use case? The commit messages don't tell.
>
> i386 has the default cache topology model: l1 per core/l2 per core/l3
> per die.
>
> Cache topology affects scheduler performance, e.g., kernel's cluster
> scheduling.
>
> Of course I can hardcode some cache topology model in the specific cpu
> model that corresponds to the actual hardware, but for -cpu host/max,
> the default i386 cache topology model has no flexibility, and the
> host-cpu-cache option doesn't have enough fine-grained control over the
> cache topology.
>
> So I want to provide a way to allow user create more fleasible cache
> topology. Just like cpu topology.
So the use case is exposing a configurable cache topology to the guest
in order to increase performance. Performance can increase when the
configured virtual topology is closer to the physical topology than a
default topology would be. This can be the case with CPU host or max.
Correct?
>> Why does that use case make no sense without SMP?
>
> As the example I mentioned, for Intel hyrbid architecture, P cores has
> l2 per core and E cores has l2 per module. Then either setting the l2
> topology level as core nor module, can emulate the real case.
>
> Even considering the more extreme case of Intel 14th MTL CPU, where
> some E cores have L3 and some don't even have L3. As well as the last
> time you and Daniel mentioned that in the future we could consider
> covering more cache properties such as cache size. But the l3 size can
> be different in the same system, like AMD's x3D technology. So
> generally configuring properties for @name in a list can't take into
> account the differences of heterogeneous caches with the same @name.
>
> Hope my poor english explains the problem well. :-)
I think I understand why you want to configure caches. My question was
about the connection to SMP.
Say we run a guest with a single core, no SMP. Could configuring caches
still be useful then?
>> Can the same @name occur multiple times? Documentation doesn't tell.
>> If yes, what does that mean?
>
> Yes, this means the later one will override the previous one with the same
> name.
Needs documenting.
If you make it an error, you don't have to document it :)
>> Say we later add value "l1" for unified level 1 cache. Would "l1" then
>> conflict with "l1d" and "l1u"?
>
> Yes, we should check in smp/machine code and ban l1 and l1i/l1d at the
> same time. This check I suppose is easy to add.
>
>> May @topo be "invalid"? Documentation doesn't tell. If yes, what does
>> that mean?
>
> Yes, just follow the intel's spec, invalid means the current topology
> information is invalid, which is used to encode x86 CPUIDs. So when I
> move this level to qapi, I just keeped this. Otherwise, I need to
> re-implement the i386 specific invalid level.
I'm afraid I don't understand what is supposed to happen when I tell
QEMU to make a cache's topology invalid.
>> >> The more idiomatic QAPI name would be SmpCacheName. Likewise for the
>> >> other type names below.
>> >
>> > I hesitated here as well, but considering that SMPConfiguration is "SMP"
>> > and not "Smp", it has that name. I'll change to SmpCacheName for strict
>> > initial capitalization.
>> >
>> >> > +#
>> >> > +# An enumeration of cache for SMP systems. The cache name here is
>> >> > +# a combination of cache level and cache type.
>> >>
>> >> The first sentence feels awkward. Maybe
>> >>
>> >> # Caches an SMP system may have.
>> >>
>> >> > +#
>> >> > +# @l1d: L1 data cache.
>> >> > +#
>> >> > +# @l1i: L1 instruction cache.
>> >> > +#
>> >> > +# @l2: L2 (unified) cache.
>> >> > +#
>> >> > +# @l3: L3 (unified) cache
>> >> > +#
>> >> > +# Since: 9.1
>> >> > +##
>> >>
>> >> This assumes the L1 cache is split, and L2 and L3 are unified.
>> >>
>> >> If we model a system with say a unified L1 cache, we'd simply extend
>> >> this enum. No real difference to extending it for additional levels.
>> >> Correct?
>> >
>> > Yes. For unified L1, we just need add a "l1" which is opposed to l1i/l1d.
>> >
>> >> > +{ 'enum': 'SMPCacheName',
>> >> > + 'prefix': 'SMP_CACHE',
>> >>
>> >> Why not call it SmpCache, and ditch 'prefix'?
>> >
>> > Because the SMPCache structure in smp_cache.h uses the similar name:
>> >
>> > +#define TYPE_SMP_CACHE "smp-cache"
>> > +OBJECT_DECLARE_SIMPLE_TYPE(SMPCache, SMP_CACHE)
>> > +
>> > +struct SMPCache {
>> > + Object parent_obj;
>> > +
>> > + SMPCacheProperty props[SMP_CACHE__MAX];
>> > +};
>> >
>> > Naming is always difficult,
>>
>> Oh yes.
>>
>> > so I would use Smpcache here if you feel that
>> > SmpCache is sufficient to distinguish it from SMPCache, or I would also
>> > rename the SMPCache structure to SMPCacheState in smp_cache.h.
>> >
>> > Which way do you prefer?
>>
>> Having both QAPI enum SmpCache and handwritten type SMPCache is clearly
>> undesirable.
>>
>> I retract my suggestion to name the enum SmpCache. The thing clearly is
>> not a cache. SmpCacheName is better.
>>
>> If you drop 'prefix', the generated C enum values look like
>> SMP_CACHE_NAME_FOO. Would that work for you?
>
> I think the SmpCacheName is ok, since there's no other better names.
>
>> The "name" part bothers me a bit. A name doesn't define what something
>> is. A type does. SmpCacheType?
>
> Ah, I also considerred this. I didn't use "type" because people usually
> uses cache type to indicate INSTRUCTION/DATA/UNIFIED and cache level to
> indicate LEVEL 1/LEVEL 2/LEVEL 3. The enumeration here is a combination of
> type+level. So I think it's better to avoid the type term.
SmpCacheLevelAndType is quite a mouthful.
>> >> > + 'data': [ 'l1d', 'l1i', 'l2', 'l3' ] }
>> >>
>> >> > +
>> >> > +##
>> >> > +# @SMPCacheProperty:
>> >>
>> >> Sure we want to call this "property" (singular) and not "properties"?
>> >> What if we add members to this type?
>> >>
>> >> > +#
>> >> > +# Cache information for SMP systems.
>> >> > +#
>> >> > +# @name: Cache name.
>> >> > +#
>> >> > +# @topo: Cache topology level. It accepts the CPU topology
>> >> > +# enumeration as the parameter, i.e., CPUs in the same
>> >> > +# topology container share the same cache.
>> >> > +#
>> >> > +# Since: 9.1
>> >> > +##
>> >> > +{ 'struct': 'SMPCacheProperty',
>> >> > + 'data': {
>> >> > + 'name': 'SMPCacheName',
>> >> > + 'topo': 'CpuTopologyLevel' } }
>> >>
>> >> We tend to avoid abbreviations in the QAPI schema. Please consider
>> >> naming this 'topology'.
>> >
>> > Sure!
>> >
>> >> > +
>> >> > +##
>> >> > +# @SMPCacheProperties:
>> >> > +#
>> >> > +# List wrapper of SMPCacheProperty.
>> >> > +#
>> >> > +# @caches: the SMPCacheProperty list.
>> >> > +#
>> >> > +# Since 9.1
>> >> > +##
>> >> > +{ 'struct': 'SMPCacheProperties',
>> >> > + 'data': { 'caches': ['SMPCacheProperty'] } }
>> >>
>> >> Ah, now I see why you used the singular above!
>> >>
>> >> However, this type holds the properties of call caches. It is a list
>>
>> "of all caches" (can't type).
>
> Sorry I didn't get your point?
I had typoed "of call caches", which makes no sense, so I corrected it.
>> >> where each element holds the properties of a single cache. Calling the
>> >> former "cache property" and the latter "cache properties" is confusing.
>> >
>> > Yes...
>> >
>> >> SmpCachesProperties and SmpCacheProperties would put the singular
>> >> vs. plural where it belongs. Sounds a bit awkward to me, though.
>> >> Naming is hard.
>> >
>> > For SmpCachesProperties, it's easy to overlook the first "s".
>> >
>> >> Other ideas, anybody?
>> >
>> > Maybe SmpCacheOptions or SmpCachesPropertyWrapper?
>>
>> I wonder why we have a single QOM object to configure all caches, and
>> not one QOM object per cache.
>
> I have the thoughts and questions here:
>
> https://lore.kernel.org/qemu-devel/20240704031603.1744546-1-zhao1.liu@xxxxxxxxx/T/#m8adba8ba14ebac0c9935fbf45983cc71e53ccf45
>
> We could discuss this issue in that thread :-).
Okay.
[...]
