hw/core: Rename CpuTopology to CPUTopology

Commit Message

Zhao Liu May 27, 2024, 1:18 p.m. UTC

Use CPUTopology to honor the generic style of CPU capitalization
abbreviations.

Signed-off-by: Zhao Liu <[e-mail]>
---
 * Split from the previous SMP cache RFC:
   https://lore.kernel.org/qemu-devel/[e-mail]/
---
 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(-)

Comments

Markus Armbruster June 3, 2024, 11:54 a.m. UTC | #1

Zhao Liu <[e-mail]> writes:

> Use CPUTopology to honor the generic style of CPU capitalization
> abbreviations.
>
> Signed-off-by: Zhao Liu <[e-mail]>

Is CPUFoo really more common than CpuFoo?  It isn't in the qapi
schema...

Zhao Liu June 4, 2024, 4:15 a.m. UTC | #2

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 <[e-mail]>
> Subject: Re: [PATCH] hw/core: Rename CpuTopology to CPUTopology
> 
> Zhao Liu <[e-mail]> writes:
> 
> > Use CPUTopology to honor the generic style of CPU capitalization
> > abbreviations.
> >
> > Signed-off-by: Zhao Liu <[e-mail]>
> 
> 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...

Markus Armbruster June 4, 2024, 5:29 a.m. UTC | #3

Zhao Liu <[e-mail]> 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 <[e-mail]>
>> Subject: Re: [PATCH] hw/core: Rename CpuTopology to CPUTopology
>> 
>> Zhao Liu <[e-mail]> writes:
>> 
>> > Use CPUTopology to honor the generic style of CPU capitalization
>> > abbreviations.
>> >
>> > Signed-off-by: Zhao Liu <[e-mail]>
>> 
>> 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.

Zhao Liu June 4, 2024, 6:36 a.m. UTC | #4

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 <[e-mail]>
> Subject: Re: [PATCH] hw/core: Rename CpuTopology to CPUTopology
> 
> Zhao Liu <[e-mail]> 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 <[e-mail]>
> >> Subject: Re: [PATCH] hw/core: Rename CpuTopology to CPUTopology
> >> 
> >> Zhao Liu <[e-mail]> writes:
> >> 
> >> > Use CPUTopology to honor the generic style of CPU capitalization
> >> > abbreviations.
> >> >
> >> > Signed-off-by: Zhao Liu <[e-mail]>
> >> 
> >> 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. ;-)

Markus Armbruster June 4, 2024, 8:38 a.m. UTC | #5

Zhao Liu <[e-mail]> 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 <[e-mail]>
>> Subject: Re: [PATCH] hw/core: Rename CpuTopology to CPUTopology
>> 
>> Zhao Liu <[e-mail]> 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 <[e-mail]>
>> >> Subject: Re: [PATCH] hw/core: Rename CpuTopology to CPUTopology
>> >> 
>> >> Zhao Liu <[e-mail]> writes:
>> >> 
>> >> > Use CPUTopology to honor the generic style of CPU capitalization
>> >> > abbreviations.
>> >> >
>> >> > Signed-off-by: Zhao Liu <[e-mail]>
>> >> 
>> >> 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.

Zhao Liu June 19, 2024, 2:49 p.m. UTC | #6

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 <[e-mail]>
> 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 <[e-mail]>
> ---
>  * Split from the previous SMP cache RFC:
>    https://lore.kernel.org/qemu-devel/[e-mail]/
> ---
>  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 = &current_machine->smp;
> +    CPUTopology *smp = &current_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 = &current_machine->smp;
> +    CPUTopology *smp = &current_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);
> -- 
> 2.34.1
>

Thomas Huth June 19, 2024, 2:53 p.m. UTC | #7

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 <[e-mail]>

> 
> 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 <[e-mail]>
>> 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 <[e-mail]>
>> ---
>>   * Split from the previous SMP cache RFC:
>>     https://lore.kernel.org/qemu-devel/[e-mail]/
>> ---
>>   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(-)

Patch

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 = &current_machine->smp;
+    CPUTopology *smp = &current_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 = &current_machine->smp;
+    CPUTopology *smp = &current_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);