| Message ID | 1484664152-24446-2-git-send-email-douly.fnst@cn.fujitsu.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
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 > > >
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 >> >> >> >
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. :)
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? */
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(-)