| Message ID | 20240527131837.2630961-1-zhao1.liu@intel.com |
|---|---|
| State | New |
| Headers | show |
| Series | hw/core: Rename CpuTopology to CPUTopology | expand |
Zhao Liu <zhao1.liu@intel.com> writes: > Use CPUTopology to honor the generic style of CPU capitalization > abbreviations. > > Signed-off-by: Zhao Liu <zhao1.liu@intel.com> Is CPUFoo really more common than CpuFoo? It isn't in the qapi schema...
On Mon, Jun 03, 2024 at 01:54:15PM +0200, Markus Armbruster wrote: > Date: Mon, 03 Jun 2024 13:54:15 +0200 > From: Markus Armbruster <armbru@redhat.com> > Subject: Re: [PATCH] hw/core: Rename CpuTopology to CPUTopology > > Zhao Liu <zhao1.liu@intel.com> writes: > > > Use CPUTopology to honor the generic style of CPU capitalization > > abbreviations. > > > > Signed-off-by: Zhao Liu <zhao1.liu@intel.com> > > Is CPUFoo really more common than CpuFoo? It isn't in the qapi > schema... Hi Markus, do you think this style needs to be standardized? All-caps cases, like the widely used CPUState. And the common structures declared in include/qemu/typedefs.h, are all using CPU, not Cpu... If you feel this is necessary, I'd be happy to help more places change their names to standardize their style...
Zhao Liu <zhao1.liu@intel.com> writes: > On Mon, Jun 03, 2024 at 01:54:15PM +0200, Markus Armbruster wrote: >> Date: Mon, 03 Jun 2024 13:54:15 +0200 >> From: Markus Armbruster <armbru@redhat.com> >> Subject: Re: [PATCH] hw/core: Rename CpuTopology to CPUTopology >> >> Zhao Liu <zhao1.liu@intel.com> writes: >> >> > Use CPUTopology to honor the generic style of CPU capitalization >> > abbreviations. >> > >> > Signed-off-by: Zhao Liu <zhao1.liu@intel.com> >> >> Is CPUFoo really more common than CpuFoo? It isn't in the qapi >> schema... > > Hi Markus, do you think this style needs to be standardized? > > All-caps cases, like the widely used CPUState. > > And the common structures declared in include/qemu/typedefs.h, are all > using CPU, not Cpu... > > If you feel this is necessary, I'd be happy to help more places change > their names to standardize their style... The situation is unfortunate[*]. The renaming cure could be worse than the disease, though. In a situation like this, settling for local consistency is often the least bad. machine.json is locally consistent: it consistently uses CpuFoo. [*] We suck at systematic, disciplined naming.
On Tue, Jun 04, 2024 at 07:29:15AM +0200, Markus Armbruster wrote: > Date: Tue, 04 Jun 2024 07:29:15 +0200 > From: Markus Armbruster <armbru@redhat.com> > Subject: Re: [PATCH] hw/core: Rename CpuTopology to CPUTopology > > Zhao Liu <zhao1.liu@intel.com> writes: > > > On Mon, Jun 03, 2024 at 01:54:15PM +0200, Markus Armbruster wrote: > >> Date: Mon, 03 Jun 2024 13:54:15 +0200 > >> From: Markus Armbruster <armbru@redhat.com> > >> Subject: Re: [PATCH] hw/core: Rename CpuTopology to CPUTopology > >> > >> Zhao Liu <zhao1.liu@intel.com> writes: > >> > >> > Use CPUTopology to honor the generic style of CPU capitalization > >> > abbreviations. > >> > > >> > Signed-off-by: Zhao Liu <zhao1.liu@intel.com> > >> > >> Is CPUFoo really more common than CpuFoo? It isn't in the qapi > >> schema... > > > > Hi Markus, do you think this style needs to be standardized? > > > > All-caps cases, like the widely used CPUState. > > > > And the common structures declared in include/qemu/typedefs.h, are all > > using CPU, not Cpu... > > > > If you feel this is necessary, I'd be happy to help more places change > > their names to standardize their style... > > The situation is unfortunate[*]. The renaming cure could be worse than > the disease, though. > > In a situation like this, settling for local consistency is often the > least bad. machine.json is locally consistent: it consistently uses > CpuFoo. > > > [*] We suck at systematic, disciplined naming. I see, by local consistency principle, my another patch (adding topology enumeration in machine.json) should use Cpu... The CpuTopology that this patch modifies is located in include/hw/boards.h, where that looks as if this file prefers to use CPUs (defining the CPUArchIdList and CPUArchId). And there's also another case for all-caps, SMPCompatProps (using SMP not Smp). So I feel like this patch change still makes sense... Sorry if I'm being a bit obsessive. The most confusing thing in include/hw/boards.h is this structure: typedef struct CPUArchId { ... CpuInstanceProperties props; CPUState *cpu; ... } CPUArchId; CPU and Cpu are mixed together, but this is also explained by the local consistency principle, since the CpuInstanceProperties belong to machine.json. ;-)
Zhao Liu <zhao1.liu@intel.com> writes: > On Tue, Jun 04, 2024 at 07:29:15AM +0200, Markus Armbruster wrote: >> Date: Tue, 04 Jun 2024 07:29:15 +0200 >> From: Markus Armbruster <armbru@redhat.com> >> Subject: Re: [PATCH] hw/core: Rename CpuTopology to CPUTopology >> >> Zhao Liu <zhao1.liu@intel.com> writes: >> >> > On Mon, Jun 03, 2024 at 01:54:15PM +0200, Markus Armbruster wrote: >> >> Date: Mon, 03 Jun 2024 13:54:15 +0200 >> >> From: Markus Armbruster <armbru@redhat.com> >> >> Subject: Re: [PATCH] hw/core: Rename CpuTopology to CPUTopology >> >> >> >> Zhao Liu <zhao1.liu@intel.com> writes: >> >> >> >> > Use CPUTopology to honor the generic style of CPU capitalization >> >> > abbreviations. >> >> > >> >> > Signed-off-by: Zhao Liu <zhao1.liu@intel.com> >> >> >> >> Is CPUFoo really more common than CpuFoo? It isn't in the qapi >> >> schema... >> > >> > Hi Markus, do you think this style needs to be standardized? >> > >> > All-caps cases, like the widely used CPUState. >> > >> > And the common structures declared in include/qemu/typedefs.h, are all >> > using CPU, not Cpu... >> > >> > If you feel this is necessary, I'd be happy to help more places change >> > their names to standardize their style... >> >> The situation is unfortunate[*]. The renaming cure could be worse than >> the disease, though. >> >> In a situation like this, settling for local consistency is often the >> least bad. machine.json is locally consistent: it consistently uses >> CpuFoo. >> >> >> [*] We suck at systematic, disciplined naming. > > I see, by local consistency principle, my another patch (adding topology > enumeration in machine.json) should use Cpu... > > The CpuTopology that this patch modifies is located in include/hw/boards.h, > where that looks as if this file prefers to use CPUs (defining the > CPUArchIdList and CPUArchId). And there's also another case for all-caps, > SMPCompatProps (using SMP not Smp). So I feel like this patch change > still makes sense... Sorry if I'm being a bit obsessive. > > The most confusing thing in include/hw/boards.h is this structure: > > typedef struct CPUArchId { > ... > CpuInstanceProperties props; > CPUState *cpu; > ... > } CPUArchId; "Another fine mess" > CPU and Cpu are mixed together, but this is also explained by the local > consistency principle, since the CpuInstanceProperties belong to > machine.json. ;-) Yes.
diff --git a/hw/s390x/cpu-topology.c b/hw/s390x/cpu-topology.c index f16bdf65faa0..016f6c1c15ac 100644 --- a/hw/s390x/cpu-topology.c +++ b/hw/s390x/cpu-topology.c @@ -86,7 +86,7 @@ bool s390_has_topology(void) */ static void s390_topology_init(MachineState *ms) { - CpuTopology *smp = &ms->smp; + CPUTopology *smp = &ms->smp; s390_topology.cores_per_socket = g_new0(uint8_t, smp->sockets * smp->books * smp->drawers); @@ -181,7 +181,7 @@ void s390_topology_reset(void) */ static bool s390_topology_cpu_default(S390CPU *cpu, Error **errp) { - CpuTopology *smp = ¤t_machine->smp; + CPUTopology *smp = ¤t_machine->smp; CPUS390XState *env = &cpu->env; /* All geometry topology attributes must be set or all unset */ @@ -234,7 +234,7 @@ static bool s390_topology_check(uint16_t socket_id, uint16_t book_id, uint16_t drawer_id, uint16_t entitlement, bool dedicated, Error **errp) { - CpuTopology *smp = ¤t_machine->smp; + CPUTopology *smp = ¤t_machine->smp; if (socket_id >= smp->sockets) { error_setg(errp, "Unavailable socket: %d", socket_id); diff --git a/include/hw/boards.h b/include/hw/boards.h index 2fa800f11ae4..c1737f2a5736 100644 --- a/include/hw/boards.h +++ b/include/hw/boards.h @@ -334,7 +334,7 @@ typedef struct DeviceMemoryState { } DeviceMemoryState; /** - * CpuTopology: + * CPUTopology: * @cpus: the number of present logical processors on the machine * @drawers: the number of drawers on the machine * @books: the number of books in one drawer @@ -346,7 +346,7 @@ typedef struct DeviceMemoryState { * @threads: the number of threads in one core * @max_cpus: the maximum number of logical processors on the machine */ -typedef struct CpuTopology { +typedef struct CPUTopology { unsigned int cpus; unsigned int drawers; unsigned int books; @@ -357,7 +357,7 @@ typedef struct CpuTopology { unsigned int cores; unsigned int threads; unsigned int max_cpus; -} CpuTopology; +} CPUTopology; /** * MachineState: @@ -409,7 +409,7 @@ struct MachineState { const char *cpu_type; AccelState *accelerator; CPUArchIdList *possible_cpus; - CpuTopology smp; + CPUTopology smp; struct NVDIMMState *nvdimms_state; struct NumaState *numa_state; }; diff --git a/include/hw/s390x/cpu-topology.h b/include/hw/s390x/cpu-topology.h index c064f427e948..ff09c57a4428 100644 --- a/include/hw/s390x/cpu-topology.h +++ b/include/hw/s390x/cpu-topology.h @@ -63,17 +63,17 @@ static inline void s390_topology_reset(void) extern S390Topology s390_topology; -static inline int s390_std_socket(int n, CpuTopology *smp) +static inline int s390_std_socket(int n, CPUTopology *smp) { return (n / smp->cores) % smp->sockets; } -static inline int s390_std_book(int n, CpuTopology *smp) +static inline int s390_std_book(int n, CPUTopology *smp) { return (n / (smp->cores * smp->sockets)) % smp->books; } -static inline int s390_std_drawer(int n, CpuTopology *smp) +static inline int s390_std_drawer(int n, CPUTopology *smp) { return (n / (smp->cores * smp->sockets * smp->books)) % smp->drawers; } diff --git a/tests/unit/test-smp-parse.c b/tests/unit/test-smp-parse.c index 9fdba24fce56..f51138794ca1 100644 --- a/tests/unit/test-smp-parse.c +++ b/tests/unit/test-smp-parse.c @@ -120,8 +120,8 @@ */ typedef struct SMPTestData { SMPConfiguration config; - CpuTopology expect_prefer_sockets; - CpuTopology expect_prefer_cores; + CPUTopology expect_prefer_sockets; + CPUTopology expect_prefer_cores; const char *expect_error; } SMPTestData; @@ -643,7 +643,7 @@ static char *smp_config_to_string(const SMPConfiguration *config) } /* Use the different calculation than machine_topo_get_threads_per_socket(). */ -static unsigned int cpu_topology_get_threads_per_socket(const CpuTopology *topo) +static unsigned int cpu_topology_get_threads_per_socket(const CPUTopology *topo) { /* Check the divisor to avoid invalid topology examples causing SIGFPE. */ if (!topo->drawers || !topo->books || !topo->sockets) { @@ -654,7 +654,7 @@ static unsigned int cpu_topology_get_threads_per_socket(const CpuTopology *topo) } /* Use the different calculation than machine_topo_get_cores_per_socket(). */ -static unsigned int cpu_topology_get_cores_per_socket(const CpuTopology *topo) +static unsigned int cpu_topology_get_cores_per_socket(const CPUTopology *topo) { /* Check the divisor to avoid invalid topology examples causing SIGFPE. */ if (!topo->threads) { @@ -664,13 +664,13 @@ static unsigned int cpu_topology_get_cores_per_socket(const CpuTopology *topo) } } -static char *cpu_topology_to_string(const CpuTopology *topo, +static char *cpu_topology_to_string(const CPUTopology *topo, unsigned int threads_per_socket, unsigned int cores_per_socket, bool has_clusters) { return g_strdup_printf( - "(CpuTopology) {\n" + "(CPUTopology) {\n" " .cpus = %u,\n" " .drawers = %u,\n" " .books = %u,\n" @@ -692,7 +692,7 @@ static char *cpu_topology_to_string(const CpuTopology *topo, } static void check_parse(MachineState *ms, const SMPConfiguration *config, - const CpuTopology *expect_topo, const char *expect_err, + const CPUTopology *expect_topo, const char *expect_err, bool is_valid) { MachineClass *mc = MACHINE_GET_CLASS(ms);
Use CPUTopology to honor the generic style of CPU capitalization abbreviations. Signed-off-by: Zhao Liu <zhao1.liu@intel.com> --- * Split from the previous SMP cache RFC: https://lore.kernel.org/qemu-devel/20240220092504.726064-2-zhao1.liu@linux.intel.com/ --- hw/s390x/cpu-topology.c | 6 +++--- include/hw/boards.h | 8 ++++---- include/hw/s390x/cpu-topology.h | 6 +++--- tests/unit/test-smp-parse.c | 14 +++++++------- 4 files changed, 17 insertions(+), 17 deletions(-)