Re: [PATCH] hw/core: Rename CpuTopology to CPUTopology
On 19/06/2024 16.49, Zhao Liu wrote: Hi maintainers, Per my communication with Markus, it seems this renaming matches the "local consistency" principle in (include/hw/boards.h). :-) So do you think this change is acceptable? I don't care too much, both ways of naming look acceptable to me... ... but in case somebody else wants to merge this, FWIW: s390x parts Acked-by: Thomas Huth On Mon, May 27, 2024 at 09:18:37PM +0800, Zhao Liu wrote: Date: Mon, 27 May 2024 21:18:37 +0800 From: Zhao Liu Subject: [PATCH] hw/core: Rename CpuTopology to CPUTopology X-Mailer: git-send-email 2.34.1 Use CPUTopology to honor the generic style of CPU capitalization abbreviations. Signed-off-by: Zhao Liu --- * Split from the previous SMP cache RFC: https://lore.kernel.org/qemu-devel/20240220092504.726064-2-zhao1@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(-)
Re: [PATCH] hw/core: Rename CpuTopology to CPUTopology
Hi maintainers, Per my communication with Markus, it seems this renaming matches the "local consistency" principle in (include/hw/boards.h). :-) So do you think this change is acceptable? Thanks, Zhao On Mon, May 27, 2024 at 09:18:37PM +0800, Zhao Liu wrote: > Date: Mon, 27 May 2024 21:18:37 +0800 > From: Zhao Liu > Subject: [PATCH] hw/core: Rename CpuTopology to CPUTopology > X-Mailer: git-send-email 2.34.1 > > Use CPUTopology to honor the generic style of CPU capitalization > abbreviations. > > Signed-off-by: Zhao Liu > --- > * Split from the previous SMP cache RFC: > > https://lore.kernel.org/qemu-devel/20240220092504.726064-2-zhao1@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(-) > > 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
Re: [PATCH] hw/core: Rename CpuTopology to CPUTopology
Zhao Liu 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 >> Subject: Re: [PATCH] hw/core: Rename CpuTopology to CPUTopology >> >> Zhao Liu 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 >> >> Subject: Re: [PATCH] hw/core: Rename CpuTopology to CPUTopology >> >> >> >> Zhao Liu writes: >> >> >> >> > Use CPUTopology to honor the generic style of CPU capitalization >> >> > abbreviations. >> >> > >> >> > Signed-off-by: Zhao Liu >> >> >> >> 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.
Re: [PATCH] hw/core: Rename CpuTopology to CPUTopology
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 > Subject: Re: [PATCH] hw/core: Rename CpuTopology to CPUTopology > > Zhao Liu 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 > >> Subject: Re: [PATCH] hw/core: Rename CpuTopology to CPUTopology > >> > >> Zhao Liu writes: > >> > >> > Use CPUTopology to honor the generic style of CPU capitalization > >> > abbreviations. > >> > > >> > Signed-off-by: Zhao Liu > >> > >> 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. ;-)
Re: [PATCH] hw/core: Rename CpuTopology to CPUTopology
Zhao Liu 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 >> Subject: Re: [PATCH] hw/core: Rename CpuTopology to CPUTopology >> >> Zhao Liu writes: >> >> > Use CPUTopology to honor the generic style of CPU capitalization >> > abbreviations. >> > >> > Signed-off-by: Zhao Liu >> >> 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.
Re: [PATCH] hw/core: Rename CpuTopology to CPUTopology
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 > Subject: Re: [PATCH] hw/core: Rename CpuTopology to CPUTopology > > Zhao Liu writes: > > > Use CPUTopology to honor the generic style of CPU capitalization > > abbreviations. > > > > Signed-off-by: Zhao Liu > > 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...
Re: [PATCH] hw/core: Rename CpuTopology to CPUTopology
Zhao Liu writes: > Use CPUTopology to honor the generic style of CPU capitalization > abbreviations. > > Signed-off-by: Zhao Liu Is CPUFoo really more common than CpuFoo? It isn't in the qapi schema...
[PATCH] hw/core: Rename CpuTopology to CPUTopology
Use CPUTopology to honor the generic style of CPU capitalization abbreviations. Signed-off-by: Zhao Liu --- * Split from the previous SMP cache RFC: https://lore.kernel.org/qemu-devel/20240220092504.726064-2-zhao1@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(-) 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