[Qemu-devel] When to use qemu/typedefs.h (was: [PATCH 23/28] numa: Don't include hw/boards.h into sysemu/numa.h)

[Markus Armbruster <armbru@redhat.com>]

Cc'ing a few more people who might be interested.

Eduardo Habkost <ehabkost@redhat.com> writes:

> On Fri, Jul 26, 2019 at 02:05:37PM +0200, Markus Armbruster wrote:
>> sysemu/numa.h includes hw/boards.h just for the CPUArchId typedef, at
>> the cost of pulling in more than two dozen extra headers indirectly.
>> 
>> I could move the typedef from hw/boards.h to qemu/typedefs.h.  But
>> it's used in just two headers: boards.h and numa.h.
>> 
>> I could move it to another header both its users include.
>> exec/cpu-common.h seems to be the least bad fit.
>> 
>> But I'm keeping this simple & stupid: declare the struct tag in
>> numa.h.
>> 
>> Cc: Eduardo Habkost <ehabkost@redhat.com>
>> Cc: Marcel Apfelbaum <marcel.apfelbaum@gmail.com>
>> Signed-off-by: Markus Armbruster <armbru@redhat.com>
>> ---
>>  exec.c                     | 1 +
>>  hw/core/machine-qmp-cmds.c | 1 +
>>  hw/core/machine.c          | 1 +
>>  hw/mem/pc-dimm.c           | 1 +
>>  include/hw/boards.h        | 2 +-
>>  include/sysemu/numa.h      | 9 +++++++--
>>  6 files changed, 12 insertions(+), 3 deletions(-)
>> 
>> diff --git a/exec.c b/exec.c
>> index 6d60fdfb1f..4d9e146c79 100644
>> --- a/exec.c
>> +++ b/exec.c
>> @@ -41,6 +41,7 @@
>>  #if defined(CONFIG_USER_ONLY)
>>  #include "qemu.h"
>>  #else /* !CONFIG_USER_ONLY */
>> +#include "hw/boards.h"
>>  #include "exec/memory.h"
>>  #include "exec/ioport.h"
>>  #include "sysemu/dma.h"
>> diff --git a/hw/core/machine-qmp-cmds.c b/hw/core/machine-qmp-cmds.c
>> index c83e8954e1..d8284671f0 100644
>> --- a/hw/core/machine-qmp-cmds.c
>> +++ b/hw/core/machine-qmp-cmds.c
>> @@ -9,6 +9,7 @@
>>  
>>  #include "qemu/osdep.h"
>>  #include "cpu.h"
>> +#include "hw/boards.h"
>>  #include "qapi/error.h"
>>  #include "qapi/qapi-commands-machine.h"
>>  #include "qapi/qmp/qerror.h"
>> diff --git a/hw/core/machine.c b/hw/core/machine.c
>> index 2c9c93983a..901f3fa905 100644
>> --- a/hw/core/machine.c
>> +++ b/hw/core/machine.c
>> @@ -23,6 +23,7 @@
>>  #include "sysemu/numa.h"
>>  #include "qemu/error-report.h"
>>  #include "sysemu/qtest.h"
>> +#include "hw/boards.h"
>>  #include "hw/pci/pci.h"
>>  #include "hw/mem/nvdimm.h"
>>  
>> diff --git a/hw/mem/pc-dimm.c b/hw/mem/pc-dimm.c
>> index 0c83dcd61e..fa90d4fc6c 100644
>> --- a/hw/mem/pc-dimm.c
>> +++ b/hw/mem/pc-dimm.c
>> @@ -19,6 +19,7 @@
>>   */
>>  
>>  #include "qemu/osdep.h"
>> +#include "hw/boards.h"
>>  #include "hw/mem/pc-dimm.h"
>>  #include "hw/qdev-properties.h"
>>  #include "migration/vmstate.h"
>> diff --git a/include/hw/boards.h b/include/hw/boards.h
>> index 67e551636a..739d109fe1 100644
>> --- a/include/hw/boards.h
>> +++ b/include/hw/boards.h
>> @@ -86,7 +86,7 @@ void machine_class_allow_dynamic_sysbus_dev(MachineClass *mc, const char *type);
>>   * @props - CPU object properties, initialized by board
>>   * #vcpus_count - number of threads provided by @cpu object
>>   */
>> -typedef struct {
>> +typedef struct CPUArchId {
>>      uint64_t arch_id;
>>      int64_t vcpus_count;
>>      CpuInstanceProperties props;
>> diff --git a/include/sysemu/numa.h b/include/sysemu/numa.h
>> index 01a263eba2..4c4c1dee9b 100644
>> --- a/include/sysemu/numa.h
>> +++ b/include/sysemu/numa.h
>> @@ -4,7 +4,10 @@
>>  #include "qemu/bitmap.h"
>>  #include "sysemu/sysemu.h"
>>  #include "sysemu/hostmem.h"
>> -#include "hw/boards.h"
>> +#include "qapi/qapi-types-machine.h"
>
> This seems to be needed because of NodeInfo.

NumaOptions, actually.

>> +#include "exec/cpu-common.h"
>
> This seems to be needed because of ram_addr_t.

Correct.

> Reviewed-by: Eduardo Habkost <ehabkost@redhat.com>
>
>
>> +
>> +struct CPUArchId;
>>  
>
> I wonder if we should do the same for other struct types like
> NodeInfo.

NumaOptions, I think.

> Why is it bad to require the inclusion of hw/boards.h just
> because of CPUArchId, but acceptable to require the inclusion of
> qapi-types-machine.h just to be able to write "NodeInfo *nodes"
> instead of "struct NodeInfo *nodes" below?

hw/boards.h is worse.  Both headers pull in qapi/qapi-builtin-types.h
and qapi/qapi-types-common.h, but hw/boards.h pulls in ~60 more.

That doesn't mean including qapi/qapi-types-common.h is good.

For better or worse[*], our coding style mandates typedefs.

We generally declare a typedef name in exactly one place.  The obvious
place is together with the type it denotes.

Non-local users of the type then need to include the header that
declares the typedef name.

This can lead to inclusion cycles, in particular for complete struct and
union types that need more types for their members.

We move typedefs to qemu/typedefs.h to break such cycles.

We also do it to include less, for less frequent recompilation.  As this
series demonstrates, we're not very good at that.  When to put a typedef
in qemu/typedefs.h for this purpose is not obvious.  If we simply put
all of them there, we'd recompile the world every time we add a typedef,
which is pretty much exactly what we're trying to avoid.

Some of qemu/typedefs.h's typedefs are used in dozens or even hundreds
of other headers.  Quite a few only in one.  The latter likely should
not be there.

We occasionally give up and use types directly rather than their typedef
names, flouting the coding style.  This patch does.  Trades messing with
qemu/typedefs.h for having to write 'struct' a few times.

Should we give up more?  Or not at all?

Can we have some guidelines on proper use of qemu/typedefs.h?

>>  extern int nb_numa_nodes;   /* Number of NUMA nodes */
>>  extern bool have_numa_distance;
>> @@ -32,5 +35,7 @@ void numa_legacy_auto_assign_ram(MachineClass *mc, NodeInfo *nodes,
>>                                   int nb_nodes, ram_addr_t size);
>>  void numa_default_auto_assign_ram(MachineClass *mc, NodeInfo *nodes,
>>                                    int nb_nodes, ram_addr_t size);
>> -void numa_cpu_pre_plug(const CPUArchId *slot, DeviceState *dev, Error **errp);
>> +void numa_cpu_pre_plug(const struct CPUArchId *slot, DeviceState *dev,
>> +                       Error **errp);
>> +
>>  #endif
>> -- 
>> 2.21.0
>> 


[*] Worse if you ask me.