Re: [PATCH 2/8] qapi/qom: Introduce smp-cache object

[Zhao Liu]

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@redhat.com>
> Subject: Re: [PATCH 2/8] qapi/qom: Introduce smp-cache object
> 
> Zhao Liu <zhao1.liu@intel.com> 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@redhat.com>
> > Signed-off-by: Zhao Liu <zhao1.liu@intel.com>
> 
> [...]
> 
> > 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?

> 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, 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?

> > +  '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
> 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?

> > diff --git a/qapi/qapi-schema.json b/qapi/qapi-schema.json
> > index b1581988e4eb..25394f2cda50 100644
> > --- a/qapi/qapi-schema.json
> > +++ b/qapi/qapi-schema.json
> > @@ -64,11 +64,11 @@
> >  { 'include': 'compat.json' }
> >  { 'include': 'control.json' }
> >  { 'include': 'introspect.json' }
> > -{ 'include': 'qom.json' }
> > -{ 'include': 'qdev.json' }
> >  { 'include': 'machine-common.json' }
> >  { 'include': 'machine.json' }
> >  { 'include': 'machine-target.json' }
> > +{ 'include': 'qom.json' }
> > +{ 'include': 'qdev.json' }
> >  { 'include': 'replay.json' }
> >  { 'include': 'yank.json' }
> >  { 'include': 'misc.json' }
> 
> Worth explaining in the commit message, I think.

Because of the include relationship between the json files, I need to
change the order. I had a "crazy" proposal to clean up this:
https://lore.kernel.org/qemu-devel/20240517062748.782366-1-zhao1.liu@intel.com/

Thanks,
Zhao