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 = >smp;
> +CPUTopology *smp = >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 = _machine->smp;
> +CPUTopology *smp = _machine->smp;
> CPUS390XState *env = >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 = _machine->smp;
> +CPUTopology *smp = _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
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...
