[Qemu-devel] [PATCH 0/2] vl: Ensure two functions in the appropriate location

[Qemu-devel] [PATCH 0/2] vl: Ensure two functions in the appropriate location

[Dou Liyang]

I found and try to fix a bug, which shows below.

[Problem]
---------

As I use this command:
./x86_64-softmmu/qemu-system-x86_64 \
	-hda /image/fedora.img \
	-m 1G,slots=4,maxmem=4G \
	-enable-kvm \
	-smp 2,maxcpus=8,sockets=2,cores=2,threads=2 \
	-device qemu64-x86_64-cpu,id=cpu1,socket-id=0,core-id=1,thread-id=0 \
	-device qemu64-x86_64-cpu,id=cpu2,socket-id=0,core-id=1,thread-id=1 \
	-device qemu64-x86_64-cpu,id=cpu3,socket-id=1,core-id=0,thread-id=0 \
	-device qemu64-x86_64-cpu,id=cpu4,socket-id=1,core-id=0,thread-id=1 \
	-device qemu64-x86_64-cpu,id=cpu5,socket-id=1,core-id=1,thread-id=0 \
	-device qemu64-x86_64-cpu,id=cpu6,socket-id=1,core-id=1,thread-id=1 \
	-numa node,nodeid=0,cpus=0-1 \
	-numa node,nodeid=1,cpus=2-3 \
	-numa node,nodeid=2,cpus=4-5 \
	-numa node,nodeid=3,cpus=6-7 \
	-monitor stdio \

In Qemu monitor:

(qemu) info numa

4 nodes
node 0 cpus: 0 1 2 3 4 5 6 7
node 0 size: 256 MB
node 1 cpus:
node 1 size: 256 MB
node 2 cpus:
node 2 size: 256 MB
node 3 cpus:
node 3 size: 256 MB

* It is amazing that the cpus are all in node one. *

Then, I used the "numactl -H" in the guest, and found that the mapping is
right. So I guess it is not big problem, may has some bugs in the initialization.
I followed the initialization process and found it out. 

[Solution]
----------

Ensure the numa_post_machine_init func in the appropriate location.

After the patches:

(qemu) info numa 
4 nodes
node 0 cpus: 0 1
node 0 size: 256 MB
node 1 cpus: 2 3
node 1 size: 256 MB
node 2 cpus: 4 5
node 2 size: 256 MB
node 3 cpus: 6 7
node 3 size: 256 MB

Dou Liyang (2):
  vl: Ensure the numa_post_machine_init func in the appropriate location
  vl: Ensure the cpu_synchronize_all_post_init func in the appropriate
    location

 vl.c | 9 +++++----
 1 file changed, 5 insertions(+), 4 deletions(-)

-- 
2.5.5

[Qemu-devel] [PATCH 1/2] vl: Ensure the numa_post_machine_init func in the appropriate location

[Dou Liyang]

In the numa_post_machine_init(), we use CPU_FOREACH macro to set all
CPUs' namu_node. So, we should make sure that we call it after Qemu
has already initialied all the CPUs.

As we all know, the CPUs can be created by "-smp"(pc_new_cpu) or
"-device"(qdev_device_add) command. But, before the device init,
Qemu execute the numa_post_machine_init earlier. It makes the mapping
of NUMA nodes and CPUs incorrect.

The patch move the numa_post_machine_init func in the appropriate
location.

Signed-off-by: Dou Liyang <douly.fnst@cn.fujitsu.com>
---
 vl.c | 5 +++--
 1 file changed, 3 insertions(+), 2 deletions(-)

diff --git a/vl.c b/vl.c
index c643d3f..f38b0e2 100644
--- a/vl.c
+++ b/vl.c
@@ -4549,8 +4549,6 @@ int main(int argc, char **argv, char **envp)
 
     cpu_synchronize_all_post_init();
 
-    numa_post_machine_init();
-
     if (qemu_opts_foreach(qemu_find_opts("fw_cfg"),
                           parse_fw_cfg, fw_cfg_find(), NULL) != 0) {
         exit(1);
@@ -4571,6 +4569,9 @@ int main(int argc, char **argv, char **envp)
                           device_init_func, NULL, NULL)) {
         exit(1);
     }
+
+    numa_post_machine_init();
+
     rom_reset_order_override();
 
     /* Did we create any drives that we failed to create a device for? */
-- 
2.5.5

Re: [Qemu-devel] [PATCH 1/2] vl: Ensure the numa_post_machine_init func in the appropriate location

[Eduardo Habkost]

On Tue, Jan 17, 2017 at 10:42:31PM +0800, Dou Liyang wrote:
> In the numa_post_machine_init(), we use CPU_FOREACH macro to set all
> CPUs' namu_node. So, we should make sure that we call it after Qemu
> has already initialied all the CPUs.
> 
> As we all know, the CPUs can be created by "-smp"(pc_new_cpu) or
> "-device"(qdev_device_add) command. But, before the device init,
> Qemu execute the numa_post_machine_init earlier. It makes the mapping
> of NUMA nodes and CPUs incorrect.
> 
> The patch move the numa_post_machine_init func in the appropriate
> location.
> 
> Signed-off-by: Dou Liyang <douly.fnst@cn.fujitsu.com>

I would like to move cpu_index initialization to
qom/cpu.c:cpu_common_realizefn(), and remove
numa_post_machine_init() completely. But this fixes the bug while
we don't do that.

Reviewed-by: Eduardo Habkost <ehabkost@redhat.com>

Queued on numa-next. Thanks!

> ---
>  vl.c | 5 +++--
>  1 file changed, 3 insertions(+), 2 deletions(-)
> 
> diff --git a/vl.c b/vl.c
> index c643d3f..f38b0e2 100644
> --- a/vl.c
> +++ b/vl.c
> @@ -4549,8 +4549,6 @@ int main(int argc, char **argv, char **envp)
>  
>      cpu_synchronize_all_post_init();
>  
> -    numa_post_machine_init();
> -
>      if (qemu_opts_foreach(qemu_find_opts("fw_cfg"),
>                            parse_fw_cfg, fw_cfg_find(), NULL) != 0) {
>          exit(1);
> @@ -4571,6 +4569,9 @@ int main(int argc, char **argv, char **envp)
>                            device_init_func, NULL, NULL)) {
>          exit(1);
>      }
> +
> +    numa_post_machine_init();
> +
>      rom_reset_order_override();
>  
>      /* Did we create any drives that we failed to create a device for? */
> -- 
> 2.5.5
> 
> 
> 

-- 
Eduardo

Re: [Qemu-devel] [PATCH 1/2] vl: Ensure the numa_post_machine_init func in the appropriate location

[Dou Liyang]

Hi, Eduardo

At 01/18/2017 04:09 AM, Eduardo Habkost wrote:
> On Tue, Jan 17, 2017 at 10:42:31PM +0800, Dou Liyang wrote:
>> In the numa_post_machine_init(), we use CPU_FOREACH macro to set all
>> CPUs' namu_node. So, we should make sure that we call it after Qemu
>> has already initialied all the CPUs.
>>
>> As we all know, the CPUs can be created by "-smp"(pc_new_cpu) or
>> "-device"(qdev_device_add) command. But, before the device init,
>> Qemu execute the numa_post_machine_init earlier. It makes the mapping
>> of NUMA nodes and CPUs incorrect.
>>
>> The patch move the numa_post_machine_init func in the appropriate
>> location.
>>
>> Signed-off-by: Dou Liyang <douly.fnst@cn.fujitsu.com>
>
> I would like to move cpu_index initialization to
> qom/cpu.c:cpu_common_realizefn(), and remove
> numa_post_machine_init() completely.

Thanks, it is a good idea. I will try it later.

But, I hope to know that:

If you may want to say the cpu->**numa_node** initialization?
Because the **cpu_index** initialization is in pc_cpu_pre_plug()
currently, like that: cs->cpu_index = idx;

OR

You want to move both of them in cpu_common_realizefn() ?

Thanks,
	Liyang.

  But this fixes the bug while
> we don't do that.
>
> Reviewed-by: Eduardo Habkost <ehabkost@redhat.com>
>
> Queued on numa-next. Thanks!
>
>> ---
>>  vl.c | 5 +++--
>>  1 file changed, 3 insertions(+), 2 deletions(-)
>>
>> diff --git a/vl.c b/vl.c
>> index c643d3f..f38b0e2 100644
>> --- a/vl.c
>> +++ b/vl.c
>> @@ -4549,8 +4549,6 @@ int main(int argc, char **argv, char **envp)
>>
>>      cpu_synchronize_all_post_init();
>>
>> -    numa_post_machine_init();
>> -
>>      if (qemu_opts_foreach(qemu_find_opts("fw_cfg"),
>>                            parse_fw_cfg, fw_cfg_find(), NULL) != 0) {
>>          exit(1);
>> @@ -4571,6 +4569,9 @@ int main(int argc, char **argv, char **envp)
>>                            device_init_func, NULL, NULL)) {
>>          exit(1);
>>      }
>> +
>> +    numa_post_machine_init();
>> +
>>      rom_reset_order_override();
>>
>>      /* Did we create any drives that we failed to create a device for? */
>> --
>> 2.5.5
>>
>>
>>
>

Re: [Qemu-devel] [PATCH 1/2] vl: Ensure the numa_post_machine_init func in the appropriate location

[Eduardo Habkost]

On Wed, Jan 18, 2017 at 12:02:35PM +0800, Dou Liyang wrote:
> Hi, Eduardo
> 
> At 01/18/2017 04:09 AM, Eduardo Habkost wrote:
> > On Tue, Jan 17, 2017 at 10:42:31PM +0800, Dou Liyang wrote:
> > > In the numa_post_machine_init(), we use CPU_FOREACH macro to set all
> > > CPUs' namu_node. So, we should make sure that we call it after Qemu
> > > has already initialied all the CPUs.
> > > 
> > > As we all know, the CPUs can be created by "-smp"(pc_new_cpu) or
> > > "-device"(qdev_device_add) command. But, before the device init,
> > > Qemu execute the numa_post_machine_init earlier. It makes the mapping
> > > of NUMA nodes and CPUs incorrect.
> > > 
> > > The patch move the numa_post_machine_init func in the appropriate
> > > location.
> > > 
> > > Signed-off-by: Dou Liyang <douly.fnst@cn.fujitsu.com>
> > 
> > I would like to move cpu_index initialization to
> > qom/cpu.c:cpu_common_realizefn(), and remove
> > numa_post_machine_init() completely.
> 
> Thanks, it is a good idea. I will try it later.
> 
> But, I hope to know that:
> 
> If you may want to say the cpu->**numa_node** initialization?
> Because the **cpu_index** initialization is in pc_cpu_pre_plug()
> currently, like that: cs->cpu_index = idx;

Oops, I meant the numa_node initialization. :)

-- 
Eduardo

[Qemu-devel] [PATCH 2/2] vl: Ensure the cpu_synchronize_all_post_init func in the appropriate location

[Dou Liyang]

As the commit a4088c3eecb5f said, In the cpu_synchronize_all_post_init(),
we also use CPU_FOREACH macro to set all CPUs' namu_node. So, we should
make sure that we call it after Qemu has already initialied all the CPUs.

The patch move the numa_post_machine_init func in the appropriate
location.

Signed-off-by: Dou Liyang <douly.fnst@cn.fujitsu.com>
---
 vl.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/vl.c b/vl.c
index f38b0e2..75adc2a 100644
--- a/vl.c
+++ b/vl.c
@@ -4547,8 +4547,6 @@ int main(int argc, char **argv, char **envp)
 
     audio_init();
 
-    cpu_synchronize_all_post_init();
-
     if (qemu_opts_foreach(qemu_find_opts("fw_cfg"),
                           parse_fw_cfg, fw_cfg_find(), NULL) != 0) {
         exit(1);
@@ -4570,6 +4568,8 @@ int main(int argc, char **argv, char **envp)
         exit(1);
     }
 
+    cpu_synchronize_all_post_init();
+
     numa_post_machine_init();
 
     rom_reset_order_override();
-- 
2.5.5

Re: [Qemu-devel] [PATCH 2/2] vl: Ensure the cpu_synchronize_all_post_init func in the appropriate location

[Eduardo Habkost]

On Tue, Jan 17, 2017 at 10:42:32PM +0800, Dou Liyang wrote:
> As the commit a4088c3eecb5f said, In the cpu_synchronize_all_post_init(),
> we also use CPU_FOREACH macro to set all CPUs' namu_node.

I can't find commit a4088c3eecb5f, is it the commit ID of your
previous patch on your local tree? I don't see any NUMA-related
code triggered cpu_synchronize_all_post_init().

>                                                           So, we should
> make sure that we call it after Qemu has already initialied all the CPUs.

> 
> The patch move the numa_post_machine_init func in the appropriate
> location.

This patch moves cpu_synchronize_all_post_init(), not
numa_post_machine_init(). Could you describe which bug you are
fixing, exactly? It doesn't seem to be related to NUMA.

> 
> Signed-off-by: Dou Liyang <douly.fnst@cn.fujitsu.com>
> ---
>  vl.c | 4 ++--
>  1 file changed, 2 insertions(+), 2 deletions(-)
> 
> diff --git a/vl.c b/vl.c
> index f38b0e2..75adc2a 100644
> --- a/vl.c
> +++ b/vl.c
> @@ -4547,8 +4547,6 @@ int main(int argc, char **argv, char **envp)
>  
>      audio_init();
>  
> -    cpu_synchronize_all_post_init();
> -
>      if (qemu_opts_foreach(qemu_find_opts("fw_cfg"),
>                            parse_fw_cfg, fw_cfg_find(), NULL) != 0) {
>          exit(1);
> @@ -4570,6 +4568,8 @@ int main(int argc, char **argv, char **envp)
>          exit(1);
>      }
>  
> +    cpu_synchronize_all_post_init();
> +
>      numa_post_machine_init();
>  
>      rom_reset_order_override();
> -- 
> 2.5.5
> 
> 
> 

-- 
Eduardo

Re: [Qemu-devel] [PATCH 2/2] vl: Ensure the cpu_synchronize_all_post_init func in the appropriate location

[Dou Liyang]

Hi, Eduardo

To clarify:

This patch is no related to the bug about NUMA.

This patch makes sure that all CPUs can run cpu state synchronization
on target vcpu thread, not just the CPUs which created by "-smp 2..."
at the beginning.

It is also caused by the execution sequence, So I put it here.

At 01/18/2017 04:21 AM, Eduardo Habkost wrote:
> On Tue, Jan 17, 2017 at 10:42:32PM +0800, Dou Liyang wrote:
>> As the commit a4088c3eecb5f said, In the cpu_synchronize_all_post_init(),
>> we also use CPU_FOREACH macro to set all CPUs' namu_node.
>
> I can't find commit a4088c3eecb5f, is it the commit ID of your
> previous patch on your local tree? I don't see any NUMA-related
> code triggered cpu_synchronize_all_post_init().
>

Oops, yes. It is the previous patch 1 in my local tree. I will modify
it.

>>                                                           So, we should
>> make sure that we call it after Qemu has already initialied all the CPUs.
>
>>
>> The patch move the numa_post_machine_init func in the appropriate
>> location.
>
> This patch moves cpu_synchronize_all_post_init(), not
> numa_post_machine_init(). Could you describe which bug you are
> fixing, exactly? It doesn't seem to be related to NUMA.
>

Yes, I will describe it exactly in the next version.

Thanks,
	Liyang
>>
>> Signed-off-by: Dou Liyang <douly.fnst@cn.fujitsu.com>
>> ---
>>  vl.c | 4 ++--
>>  1 file changed, 2 insertions(+), 2 deletions(-)
>>
>> diff --git a/vl.c b/vl.c
>> index f38b0e2..75adc2a 100644
>> --- a/vl.c
>> +++ b/vl.c
>> @@ -4547,8 +4547,6 @@ int main(int argc, char **argv, char **envp)
>>
>>      audio_init();
>>
>> -    cpu_synchronize_all_post_init();
>> -
>>      if (qemu_opts_foreach(qemu_find_opts("fw_cfg"),
>>                            parse_fw_cfg, fw_cfg_find(), NULL) != 0) {
>>          exit(1);
>> @@ -4570,6 +4568,8 @@ int main(int argc, char **argv, char **envp)
>>          exit(1);
>>      }
>>
>> +    cpu_synchronize_all_post_init();
>> +
>>      numa_post_machine_init();
>>
>>      rom_reset_order_override();
>> --
>> 2.5.5
>>
>>
>>
>