Re: [Qemu-devel] [PATCH v7 02/11] numa: move numa global variable nb_numa_nodes into MachineState
On 7/26/2019 10:17 PM, Eduardo Habkost wrote:
On Fri, Jul 26, 2019 at 03:43:43PM +0200, Igor Mammedov wrote:
On Wed, 24 Jul 2019 15:15:28 -0300
Eduardo Habkost wrote:
On Wed, Jul 24, 2019 at 05:48:11PM +0200, Igor Mammedov wrote:
On Wed, 24 Jul 2019 12:02:41 -0300
Eduardo Habkost wrote:
On Wed, Jul 24, 2019 at 04:27:21PM +0200, Igor Mammedov wrote:
On Tue, 23 Jul 2019 12:23:57 -0300
Eduardo Habkost wrote:
On Tue, Jul 23, 2019 at 04:56:41PM +0200, Igor Mammedov wrote:
On Tue, 16 Jul 2019 22:51:12 +0800
Tao Xu wrote:
Add struct NumaState in MachineState and move existing numa global
nb_numa_nodes(renamed as "num_nodes") into NumaState. And add variable
numa_support into MachineClass to decide which submachines support NUMA.
Suggested-by: Igor Mammedov
Suggested-by: Eduardo Habkost
Signed-off-by: Tao Xu
---
No changes in v7.
Changes in v6:
- Rebase to upstream, move globals in arm/sbsa-ref and use
numa_mem_supported
- When used once or twice in the function, use
ms->numa_state->num_nodes directly
- Correct some mistakes
- Use once monitor_printf in hmp_info_numa
---
[...]
if (pxb->numa_node != NUMA_NODE_UNASSIGNED &&
-pxb->numa_node >= nb_numa_nodes) {
+pxb->numa_node >= ms->numa_state->num_nodes) {
this will crash if user tries to use device on machine that doesn't support numa
check that numa_state is not NULL before dereferencing
That's exactly why the machine_num_numa_nodes() was created in
v5, but then you asked for its removal.
V4 to more precise.
I dislike small wrappers because they usually doesn't simplify code and make it
more obscure,
forcing to jump around to see what's really going on.
Like it's implemented in this patch it's obvious what's wrong right away.
In that particular case machine_num_numa_nodes() was also misused since only a
handful
of places (6) really need NULL check while majority (48) can directly access
ms->numa_state->num_nodes.
without NULL check.
I strongly disagree, here. Avoiding a ms->numa_state==NULL check
is pointless optimization,
I see it not as optimization (compiler probably would manage to optimize out
most of them)
but as rather properly self documented code. Doing check in places where it's
not needed is confusing at best and can mask/introduce later subtle bugs at
worst.
and leads to hard to spot bugs like
the one you saw above.
That one was actually easy to spot because of the way it's written in this
patch.
When somebody is looking at a line of code containing
"ms->numa_state->num_nodes", how exactly are they supposed to
know if ms->numa_state is already guaranteed to be non-NULL, or
not?
read the code/patch
(at least I don't review just by looking at one line. And less time
I have to spend, on reading extra code and finding answers why it's
written the way it's, the better)
In this patch code touching ms->numa_state, is divided in 2 categories
generic code (memory API, CLI entry point, generic machine call
site for numa specific code, devices, monitor/qmp) and numa aware code
(huma parser and numa aware machines). The later one is majority of
affected code where ms->numa_state != NULL.
Even after I forget how this works and read code later, it would be
easy to do educated guess/check where NULL check is not need seeing
related code.
It's even easier to not have to check if/when numa_state can be
NULL because the code is safe on either case.
You don't review code by looking at a single line, but you don't
need to make it harder than it is.
With machine_num_numa_nodes() would have to look for answer why we
are doing it (unless we add a comment that check is there for noreason
in most cases and it's exercise for reader to find out where
it it's really need).
Sorry, your justification doesn't make sense to me. You don't
need to look for any answer at all, if the code makes it not
matter if numa_state is NULL. Having a single caller with
numa_state==NULL would be enough justification for the check.
I don't see any justification for wrapper this case,
could we stop bikeshedding and just let author to move on with fixing bugs, pls?
The author can move on and decide what to do, as I won't block a
patch only because of presence or absence of the wrapper.
Thank you Eduardo and Igor, I have submit v8 to fix the bugs. Both way
is Okay for me. And if there are bugs later, I will fix it ASAP.
Re: [Qemu-devel] [PATCH v7 02/11] numa: move numa global variable nb_numa_nodes into MachineState
On Fri, Jul 26, 2019 at 03:43:43PM +0200, Igor Mammedov wrote:
> On Wed, 24 Jul 2019 15:15:28 -0300
> Eduardo Habkost wrote:
>
> > On Wed, Jul 24, 2019 at 05:48:11PM +0200, Igor Mammedov wrote:
> > > On Wed, 24 Jul 2019 12:02:41 -0300
> > > Eduardo Habkost wrote:
> > >
> > > > On Wed, Jul 24, 2019 at 04:27:21PM +0200, Igor Mammedov wrote:
> > > > > On Tue, 23 Jul 2019 12:23:57 -0300
> > > > > Eduardo Habkost wrote:
> > > > >
> > > > > > On Tue, Jul 23, 2019 at 04:56:41PM +0200, Igor Mammedov wrote:
> > > > > > > On Tue, 16 Jul 2019 22:51:12 +0800
> > > > > > > Tao Xu wrote:
> > > > > > >
> > > > > > > > Add struct NumaState in MachineState and move existing numa
> > > > > > > > global
> > > > > > > > nb_numa_nodes(renamed as "num_nodes") into NumaState. And add
> > > > > > > > variable
> > > > > > > > numa_support into MachineClass to decide which submachines
> > > > > > > > support NUMA.
> > > > > > > >
> > > > > > > > Suggested-by: Igor Mammedov
> > > > > > > > Suggested-by: Eduardo Habkost
> > > > > > > > Signed-off-by: Tao Xu
> > > > > > > > ---
> > > > > > > >
> > > > > > > > No changes in v7.
> > > > > > > >
> > > > > > > > Changes in v6:
> > > > > > > > - Rebase to upstream, move globals in arm/sbsa-ref and use
> > > > > > > > numa_mem_supported
> > > > > > > > - When used once or twice in the function, use
> > > > > > > > ms->numa_state->num_nodes directly
> > > > > > > > - Correct some mistakes
> > > > > > > > - Use once monitor_printf in hmp_info_numa
> > > > > > > > ---
> > > > > > [...]
> > > > > > > > if (pxb->numa_node != NUMA_NODE_UNASSIGNED &&
> > > > > > > > -pxb->numa_node >= nb_numa_nodes) {
> > > > > > > > +pxb->numa_node >= ms->numa_state->num_nodes) {
> > > > > > > this will crash if user tries to use device on machine that
> > > > > > > doesn't support numa
> > > > > > > check that numa_state is not NULL before dereferencing
> > > > > >
> > > > > > That's exactly why the machine_num_numa_nodes() was created in
> > > > > > v5, but then you asked for its removal.
> > > > > V4 to more precise.
> > > > > I dislike small wrappers because they usually doesn't simplify code
> > > > > and make it more obscure,
> > > > > forcing to jump around to see what's really going on.
> > > > > Like it's implemented in this patch it's obvious what's wrong right
> > > > > away.
> > > > >
> > > > > In that particular case machine_num_numa_nodes() was also misused
> > > > > since only a handful
> > > > > of places (6) really need NULL check while majority (48) can directly
> > > > > access ms->numa_state->num_nodes.
> > > > > without NULL check.
> > > >
> > > > I strongly disagree, here. Avoiding a ms->numa_state==NULL check
> > > > is pointless optimization,
> > > I see it not as optimization (compiler probably would manage to optimize
> > > out most of them)
> > > but as rather properly self documented code. Doing check in places where
> > > it's
> > > not needed is confusing at best and can mask/introduce later subtle bugs
> > > at worst.
> > >
> > > > and leads to hard to spot bugs like
> > > > the one you saw above.
> > > That one was actually easy to spot because of the way it's written in
> > > this patch.
> >
> > When somebody is looking at a line of code containing
> > "ms->numa_state->num_nodes", how exactly are they supposed to
> > know if ms->numa_state is already guaranteed to be non-NULL, or
> > not?
> read the code/patch
> (at least I don't review just by looking at one line. And less time
> I have to spend, on reading extra code and finding answers why it's
> written the way it's, the better)
>
> In this patch code touching ms->numa_state, is divided in 2 categories
> generic code (memory API, CLI entry point, generic machine call
> site for numa specific code, devices, monitor/qmp) and numa aware code
> (huma parser and numa aware machines). The later one is majority of
> affected code where ms->numa_state != NULL.
>
> Even after I forget how this works and read code later, it would be
> easy to do educated guess/check where NULL check is not need seeing
> related code.
It's even easier to not have to check if/when numa_state can be
NULL because the code is safe on either case.
You don't review code by looking at a single line, but you don't
need to make it harder than it is.
> With machine_num_numa_nodes() would have to look for answer why we
> are doing it (unless we add a comment that check is there for noreason
> in most cases and it's exercise for reader to find out where
> it it's really need).
Sorry, your justification doesn't make sense to me. You don't
need to look for any answer at all, if the code makes it not
matter if numa_state is NULL. Having a single caller with
numa_state==NULL would be enough justification for the check.
>
> I don't see any justification for wrapper this case,
> could we stop bikeshedding and just let author to move on with fixing
Re: [Qemu-devel] [PATCH v7 02/11] numa: move numa global variable nb_numa_nodes into MachineState
On Wed, 24 Jul 2019 15:15:28 -0300
Eduardo Habkost wrote:
> On Wed, Jul 24, 2019 at 05:48:11PM +0200, Igor Mammedov wrote:
> > On Wed, 24 Jul 2019 12:02:41 -0300
> > Eduardo Habkost wrote:
> >
> > > On Wed, Jul 24, 2019 at 04:27:21PM +0200, Igor Mammedov wrote:
> > > > On Tue, 23 Jul 2019 12:23:57 -0300
> > > > Eduardo Habkost wrote:
> > > >
> > > > > On Tue, Jul 23, 2019 at 04:56:41PM +0200, Igor Mammedov wrote:
> > > > > > On Tue, 16 Jul 2019 22:51:12 +0800
> > > > > > Tao Xu wrote:
> > > > > >
> > > > > > > Add struct NumaState in MachineState and move existing numa global
> > > > > > > nb_numa_nodes(renamed as "num_nodes") into NumaState. And add
> > > > > > > variable
> > > > > > > numa_support into MachineClass to decide which submachines
> > > > > > > support NUMA.
> > > > > > >
> > > > > > > Suggested-by: Igor Mammedov
> > > > > > > Suggested-by: Eduardo Habkost
> > > > > > > Signed-off-by: Tao Xu
> > > > > > > ---
> > > > > > >
> > > > > > > No changes in v7.
> > > > > > >
> > > > > > > Changes in v6:
> > > > > > > - Rebase to upstream, move globals in arm/sbsa-ref and use
> > > > > > > numa_mem_supported
> > > > > > > - When used once or twice in the function, use
> > > > > > > ms->numa_state->num_nodes directly
> > > > > > > - Correct some mistakes
> > > > > > > - Use once monitor_printf in hmp_info_numa
> > > > > > > ---
> > > > > [...]
> > > > > > > if (pxb->numa_node != NUMA_NODE_UNASSIGNED &&
> > > > > > > -pxb->numa_node >= nb_numa_nodes) {
> > > > > > > +pxb->numa_node >= ms->numa_state->num_nodes) {
> > > > > > this will crash if user tries to use device on machine that doesn't
> > > > > > support numa
> > > > > > check that numa_state is not NULL before dereferencing
> > > > >
> > > > > That's exactly why the machine_num_numa_nodes() was created in
> > > > > v5, but then you asked for its removal.
> > > > V4 to more precise.
> > > > I dislike small wrappers because they usually doesn't simplify code and
> > > > make it more obscure,
> > > > forcing to jump around to see what's really going on.
> > > > Like it's implemented in this patch it's obvious what's wrong right
> > > > away.
> > > >
> > > > In that particular case machine_num_numa_nodes() was also misused since
> > > > only a handful
> > > > of places (6) really need NULL check while majority (48) can directly
> > > > access ms->numa_state->num_nodes.
> > > > without NULL check.
> > >
> > > I strongly disagree, here. Avoiding a ms->numa_state==NULL check
> > > is pointless optimization,
> > I see it not as optimization (compiler probably would manage to optimize
> > out most of them)
> > but as rather properly self documented code. Doing check in places where
> > it's
> > not needed is confusing at best and can mask/introduce later subtle bugs at
> > worst.
> >
> > > and leads to hard to spot bugs like
> > > the one you saw above.
> > That one was actually easy to spot because of the way it's written in this
> > patch.
>
> When somebody is looking at a line of code containing
> "ms->numa_state->num_nodes", how exactly are they supposed to
> know if ms->numa_state is already guaranteed to be non-NULL, or
> not?
read the code/patch
(at least I don't review just by looking at one line. And less time
I have to spend, on reading extra code and finding answers why it's
written the way it's, the better)
In this patch code touching ms->numa_state, is divided in 2 categories
generic code (memory API, CLI entry point, generic machine call
site for numa specific code, devices, monitor/qmp) and numa aware code
(huma parser and numa aware machines). The later one is majority of
affected code where ms->numa_state != NULL.
Even after I forget how this works and read code later, it would be
easy to do educated guess/check where NULL check is not need seeing
related code.
With machine_num_numa_nodes() would have to look for answer why we
are doing it (unless we add a comment that check is there for noreason
in most cases and it's exercise for reader to find out where
it it's really need).
I don't see any justification for wrapper this case,
could we stop bikeshedding and just let author to move on with fixing bugs, pls?
Re: [Qemu-devel] [PATCH v7 02/11] numa: move numa global variable nb_numa_nodes into MachineState
On Wed, Jul 24, 2019 at 05:48:11PM +0200, Igor Mammedov wrote:
> On Wed, 24 Jul 2019 12:02:41 -0300
> Eduardo Habkost wrote:
>
> > On Wed, Jul 24, 2019 at 04:27:21PM +0200, Igor Mammedov wrote:
> > > On Tue, 23 Jul 2019 12:23:57 -0300
> > > Eduardo Habkost wrote:
> > >
> > > > On Tue, Jul 23, 2019 at 04:56:41PM +0200, Igor Mammedov wrote:
> > > > > On Tue, 16 Jul 2019 22:51:12 +0800
> > > > > Tao Xu wrote:
> > > > >
> > > > > > Add struct NumaState in MachineState and move existing numa global
> > > > > > nb_numa_nodes(renamed as "num_nodes") into NumaState. And add
> > > > > > variable
> > > > > > numa_support into MachineClass to decide which submachines support
> > > > > > NUMA.
> > > > > >
> > > > > > Suggested-by: Igor Mammedov
> > > > > > Suggested-by: Eduardo Habkost
> > > > > > Signed-off-by: Tao Xu
> > > > > > ---
> > > > > >
> > > > > > No changes in v7.
> > > > > >
> > > > > > Changes in v6:
> > > > > > - Rebase to upstream, move globals in arm/sbsa-ref and use
> > > > > > numa_mem_supported
> > > > > > - When used once or twice in the function, use
> > > > > > ms->numa_state->num_nodes directly
> > > > > > - Correct some mistakes
> > > > > > - Use once monitor_printf in hmp_info_numa
> > > > > > ---
> > > > [...]
> > > > > > if (pxb->numa_node != NUMA_NODE_UNASSIGNED &&
> > > > > > -pxb->numa_node >= nb_numa_nodes) {
> > > > > > +pxb->numa_node >= ms->numa_state->num_nodes) {
> > > > > this will crash if user tries to use device on machine that doesn't
> > > > > support numa
> > > > > check that numa_state is not NULL before dereferencing
> > > >
> > > > That's exactly why the machine_num_numa_nodes() was created in
> > > > v5, but then you asked for its removal.
> > > V4 to more precise.
> > > I dislike small wrappers because they usually doesn't simplify code and
> > > make it more obscure,
> > > forcing to jump around to see what's really going on.
> > > Like it's implemented in this patch it's obvious what's wrong right away.
> > >
> > > In that particular case machine_num_numa_nodes() was also misused since
> > > only a handful
> > > of places (6) really need NULL check while majority (48) can directly
> > > access ms->numa_state->num_nodes.
> > > without NULL check.
> >
> > I strongly disagree, here. Avoiding a ms->numa_state==NULL check
> > is pointless optimization,
> I see it not as optimization (compiler probably would manage to optimize out
> most of them)
> but as rather properly self documented code. Doing check in places where it's
> not needed is confusing at best and can mask/introduce later subtle bugs at
> worst.
>
> > and leads to hard to spot bugs like
> > the one you saw above.
> That one was actually easy to spot because of the way it's written in this
> patch.
When somebody is looking at a line of code containing
"ms->numa_state->num_nodes", how exactly are they supposed to
know if ms->numa_state is already guaranteed to be non-NULL, or
not?
--
Eduardo
Re: [Qemu-devel] [PATCH v7 02/11] numa: move numa global variable nb_numa_nodes into MachineState
On Wed, 24 Jul 2019 12:02:41 -0300
Eduardo Habkost wrote:
> On Wed, Jul 24, 2019 at 04:27:21PM +0200, Igor Mammedov wrote:
> > On Tue, 23 Jul 2019 12:23:57 -0300
> > Eduardo Habkost wrote:
> >
> > > On Tue, Jul 23, 2019 at 04:56:41PM +0200, Igor Mammedov wrote:
> > > > On Tue, 16 Jul 2019 22:51:12 +0800
> > > > Tao Xu wrote:
> > > >
> > > > > Add struct NumaState in MachineState and move existing numa global
> > > > > nb_numa_nodes(renamed as "num_nodes") into NumaState. And add variable
> > > > > numa_support into MachineClass to decide which submachines support
> > > > > NUMA.
> > > > >
> > > > > Suggested-by: Igor Mammedov
> > > > > Suggested-by: Eduardo Habkost
> > > > > Signed-off-by: Tao Xu
> > > > > ---
> > > > >
> > > > > No changes in v7.
> > > > >
> > > > > Changes in v6:
> > > > > - Rebase to upstream, move globals in arm/sbsa-ref and use
> > > > > numa_mem_supported
> > > > > - When used once or twice in the function, use
> > > > > ms->numa_state->num_nodes directly
> > > > > - Correct some mistakes
> > > > > - Use once monitor_printf in hmp_info_numa
> > > > > ---
> > > [...]
> > > > > if (pxb->numa_node != NUMA_NODE_UNASSIGNED &&
> > > > > -pxb->numa_node >= nb_numa_nodes) {
> > > > > +pxb->numa_node >= ms->numa_state->num_nodes) {
> > > > this will crash if user tries to use device on machine that doesn't
> > > > support numa
> > > > check that numa_state is not NULL before dereferencing
> > >
> > > That's exactly why the machine_num_numa_nodes() was created in
> > > v5, but then you asked for its removal.
> > V4 to more precise.
> > I dislike small wrappers because they usually doesn't simplify code and
> > make it more obscure,
> > forcing to jump around to see what's really going on.
> > Like it's implemented in this patch it's obvious what's wrong right away.
> >
> > In that particular case machine_num_numa_nodes() was also misused since
> > only a handful
> > of places (6) really need NULL check while majority (48) can directly
> > access ms->numa_state->num_nodes.
> > without NULL check.
>
> I strongly disagree, here. Avoiding a ms->numa_state==NULL check
> is pointless optimization,
I see it not as optimization (compiler probably would manage to optimize out
most of them)
but as rather properly self documented code. Doing check in places where it's
not needed is confusing at best and can mask/introduce later subtle bugs at
worst.
> and leads to hard to spot bugs like
> the one you saw above.
That one was actually easy to spot because of the way it's written in this
patch.
> Although I won't reject a patch just because it doesn't have a
> machine_num_numa_nodes() wrapper, I insist we use one for clarity
> and safety.
>
Re: [Qemu-devel] [PATCH v7 02/11] numa: move numa global variable nb_numa_nodes into MachineState
On Wed, Jul 24, 2019 at 04:27:21PM +0200, Igor Mammedov wrote:
> On Tue, 23 Jul 2019 12:23:57 -0300
> Eduardo Habkost wrote:
>
> > On Tue, Jul 23, 2019 at 04:56:41PM +0200, Igor Mammedov wrote:
> > > On Tue, 16 Jul 2019 22:51:12 +0800
> > > Tao Xu wrote:
> > >
> > > > Add struct NumaState in MachineState and move existing numa global
> > > > nb_numa_nodes(renamed as "num_nodes") into NumaState. And add variable
> > > > numa_support into MachineClass to decide which submachines support NUMA.
> > > >
> > > > Suggested-by: Igor Mammedov
> > > > Suggested-by: Eduardo Habkost
> > > > Signed-off-by: Tao Xu
> > > > ---
> > > >
> > > > No changes in v7.
> > > >
> > > > Changes in v6:
> > > > - Rebase to upstream, move globals in arm/sbsa-ref and use
> > > > numa_mem_supported
> > > > - When used once or twice in the function, use
> > > > ms->numa_state->num_nodes directly
> > > > - Correct some mistakes
> > > > - Use once monitor_printf in hmp_info_numa
> > > > ---
> > [...]
> > > > if (pxb->numa_node != NUMA_NODE_UNASSIGNED &&
> > > > -pxb->numa_node >= nb_numa_nodes) {
> > > > +pxb->numa_node >= ms->numa_state->num_nodes) {
> > > this will crash if user tries to use device on machine that doesn't
> > > support numa
> > > check that numa_state is not NULL before dereferencing
> >
> > That's exactly why the machine_num_numa_nodes() was created in
> > v5, but then you asked for its removal.
> V4 to more precise.
> I dislike small wrappers because they usually doesn't simplify code and make
> it more obscure,
> forcing to jump around to see what's really going on.
> Like it's implemented in this patch it's obvious what's wrong right away.
>
> In that particular case machine_num_numa_nodes() was also misused since only
> a handful
> of places (6) really need NULL check while majority (48) can directly access
> ms->numa_state->num_nodes.
> without NULL check.
I strongly disagree, here. Avoiding a ms->numa_state==NULL check
is pointless optimization, and leads to hard to spot bugs like
the one you saw above.
Although I won't reject a patch just because it doesn't have a
machine_num_numa_nodes() wrapper, I insist we use one for clarity
and safety.
--
Eduardo
Re: [Qemu-devel] [PATCH v7 02/11] numa: move numa global variable nb_numa_nodes into MachineState
On Tue, 23 Jul 2019 12:23:57 -0300
Eduardo Habkost wrote:
> On Tue, Jul 23, 2019 at 04:56:41PM +0200, Igor Mammedov wrote:
> > On Tue, 16 Jul 2019 22:51:12 +0800
> > Tao Xu wrote:
> >
> > > Add struct NumaState in MachineState and move existing numa global
> > > nb_numa_nodes(renamed as "num_nodes") into NumaState. And add variable
> > > numa_support into MachineClass to decide which submachines support NUMA.
> > >
> > > Suggested-by: Igor Mammedov
> > > Suggested-by: Eduardo Habkost
> > > Signed-off-by: Tao Xu
> > > ---
> > >
> > > No changes in v7.
> > >
> > > Changes in v6:
> > > - Rebase to upstream, move globals in arm/sbsa-ref and use
> > > numa_mem_supported
> > > - When used once or twice in the function, use
> > > ms->numa_state->num_nodes directly
> > > - Correct some mistakes
> > > - Use once monitor_printf in hmp_info_numa
> > > ---
> [...]
> > > if (pxb->numa_node != NUMA_NODE_UNASSIGNED &&
> > > -pxb->numa_node >= nb_numa_nodes) {
> > > +pxb->numa_node >= ms->numa_state->num_nodes) {
> > this will crash if user tries to use device on machine that doesn't support
> > numa
> > check that numa_state is not NULL before dereferencing
>
> That's exactly why the machine_num_numa_nodes() was created in
> v5, but then you asked for its removal.
V4 to more precise.
I dislike small wrappers because they usually doesn't simplify code and make it
more obscure,
forcing to jump around to see what's really going on.
Like it's implemented in this patch it's obvious what's wrong right away.
In that particular case machine_num_numa_nodes() was also misused since only a
handful
of places (6) really need NULL check while majority (48) can directly access
ms->numa_state->num_nodes.
without NULL check.
Re: [Qemu-devel] [PATCH v7 02/11] numa: move numa global variable nb_numa_nodes into MachineState
On Tue, Jul 23, 2019 at 04:56:41PM +0200, Igor Mammedov wrote:
> On Tue, 16 Jul 2019 22:51:12 +0800
> Tao Xu wrote:
>
> > Add struct NumaState in MachineState and move existing numa global
> > nb_numa_nodes(renamed as "num_nodes") into NumaState. And add variable
> > numa_support into MachineClass to decide which submachines support NUMA.
> >
> > Suggested-by: Igor Mammedov
> > Suggested-by: Eduardo Habkost
> > Signed-off-by: Tao Xu
> > ---
> >
> > No changes in v7.
> >
> > Changes in v6:
> > - Rebase to upstream, move globals in arm/sbsa-ref and use
> > numa_mem_supported
> > - When used once or twice in the function, use
> > ms->numa_state->num_nodes directly
> > - Correct some mistakes
> > - Use once monitor_printf in hmp_info_numa
> > ---
[...]
> > if (pxb->numa_node != NUMA_NODE_UNASSIGNED &&
> > -pxb->numa_node >= nb_numa_nodes) {
> > +pxb->numa_node >= ms->numa_state->num_nodes) {
> this will crash if user tries to use device on machine that doesn't support
> numa
> check that numa_state is not NULL before dereferencing
That's exactly why the machine_num_numa_nodes() was created in
v5, but then you asked for its removal.
--
Eduardo
Re: [Qemu-devel] [PATCH v7 02/11] numa: move numa global variable nb_numa_nodes into MachineState
On Tue, 16 Jul 2019 22:51:12 +0800
Tao Xu wrote:
> Add struct NumaState in MachineState and move existing numa global
> nb_numa_nodes(renamed as "num_nodes") into NumaState. And add variable
> numa_support into MachineClass to decide which submachines support NUMA.
>
> Suggested-by: Igor Mammedov
> Suggested-by: Eduardo Habkost
> Signed-off-by: Tao Xu
> ---
>
> No changes in v7.
>
> Changes in v6:
> - Rebase to upstream, move globals in arm/sbsa-ref and use
> numa_mem_supported
> - When used once or twice in the function, use
> ms->numa_state->num_nodes directly
> - Correct some mistakes
> - Use once monitor_printf in hmp_info_numa
> ---
> exec.c | 5 ++-
> hw/acpi/aml-build.c | 3 +-
> hw/arm/boot.c | 4 +-
> hw/arm/sbsa-ref.c | 4 +-
> hw/arm/virt-acpi-build.c| 10 +++--
> hw/arm/virt.c | 4 +-
> hw/core/machine-hmp-cmds.c | 12 --
> hw/core/machine.c | 14 +--
> hw/core/numa.c | 60 +
> hw/i386/acpi-build.c| 2 +-
> hw/i386/pc.c| 9 +++--
> hw/mem/pc-dimm.c| 2 +
> hw/pci-bridge/pci_expander_bridge.c | 3 +-
> hw/ppc/spapr.c | 23 +--
> include/hw/acpi/aml-build.h | 2 +-
> include/hw/boards.h | 1 +
> include/sysemu/numa.h | 10 -
> 17 files changed, 107 insertions(+), 61 deletions(-)
>
> diff --git a/exec.c b/exec.c
> index 50ea9c5aaa..b6b75d2ad5 100644
> --- a/exec.c
> +++ b/exec.c
> @@ -1736,6 +1736,7 @@ long qemu_minrampagesize(void)
> long hpsize = LONG_MAX;
> long mainrampagesize;
> Object *memdev_root;
> +MachineState *ms = MACHINE(qdev_get_machine());
>
> mainrampagesize = qemu_mempath_getpagesize(mem_path);
>
> @@ -1763,7 +1764,9 @@ long qemu_minrampagesize(void)
> * so if its page size is smaller we have got to report that size
> instead.
> */
> if (hpsize > mainrampagesize &&
> -(nb_numa_nodes == 0 || numa_info[0].node_memdev == NULL)) {
> +(ms->numa_state == NULL ||
> + ms->numa_state->num_nodes == 0 ||
> + numa_info[0].node_memdev == NULL)) {
> static bool warned;
> if (!warned) {
> error_report("Huge page support disabled (n/a for main
> memory).");
> diff --git a/hw/acpi/aml-build.c b/hw/acpi/aml-build.c
> index 555c24f21d..63c1cae8c9 100644
> --- a/hw/acpi/aml-build.c
> +++ b/hw/acpi/aml-build.c
> @@ -1726,10 +1726,11 @@ void build_srat_memory(AcpiSratMemoryAffinity
> *numamem, uint64_t base,
> * ACPI spec 5.2.17 System Locality Distance Information Table
> * (Revision 2.0 or later)
> */
> -void build_slit(GArray *table_data, BIOSLinker *linker)
> +void build_slit(GArray *table_data, BIOSLinker *linker, MachineState *ms)
> {
> int slit_start, i, j;
> slit_start = table_data->len;
> +int nb_numa_nodes = ms->numa_state->num_nodes;
>
> acpi_data_push(table_data, sizeof(AcpiTableHeader));
>
> diff --git a/hw/arm/boot.c b/hw/arm/boot.c
> index a90151f465..e28daa5278 100644
> --- a/hw/arm/boot.c
> +++ b/hw/arm/boot.c
> @@ -598,9 +598,9 @@ int arm_load_dtb(hwaddr addr, const struct arm_boot_info
> *binfo,
> }
> g_strfreev(node_path);
>
> -if (nb_numa_nodes > 0) {
> +if (ms->numa_state != NULL && ms->numa_state->num_nodes > 0) {
> mem_base = binfo->loader_start;
> -for (i = 0; i < nb_numa_nodes; i++) {
> +for (i = 0; i < ms->numa_state->num_nodes; i++) {
> mem_len = numa_info[i].node_mem;
> rc = fdt_add_memory_node(fdt, acells, mem_base,
> scells, mem_len, i);
> diff --git a/hw/arm/sbsa-ref.c b/hw/arm/sbsa-ref.c
> index 2aba3c58c5..22847909bf 100644
> --- a/hw/arm/sbsa-ref.c
> +++ b/hw/arm/sbsa-ref.c
> @@ -144,6 +144,7 @@ static void create_fdt(SBSAMachineState *sms)
> {
> void *fdt = create_device_tree(>fdt_size);
> const MachineState *ms = MACHINE(sms);
> +int nb_numa_nodes = ms->numa_state->num_nodes;
> int cpu;
>
> if (!fdt) {
> @@ -760,7 +761,7 @@ sbsa_ref_cpu_index_to_props(MachineState *ms, unsigned
> cpu_index)
> static int64_t
> sbsa_ref_get_default_cpu_node_id(const MachineState *ms, int idx)
> {
> -return idx % nb_numa_nodes;
> +return idx % ms->numa_state->num_nodes;
> }
>
> static void sbsa_ref_instance_init(Object *obj)
> @@ -787,6 +788,7 @@ static void sbsa_ref_class_init(ObjectClass *oc, void
> *data)
> mc->possible_cpu_arch_ids = sbsa_ref_possible_cpu_arch_ids;
> mc->cpu_index_to_instance_props = sbsa_ref_cpu_index_to_props;
> mc->get_default_cpu_node_id = sbsa_ref_get_default_cpu_node_id;
> +mc->numa_mem_supported = true;
> }
>
> static const TypeInfo sbsa_ref_info = {
> diff --git
[Qemu-devel] [PATCH v7 02/11] numa: move numa global variable nb_numa_nodes into MachineState
Add struct NumaState in MachineState and move existing numa global
nb_numa_nodes(renamed as "num_nodes") into NumaState. And add variable
numa_support into MachineClass to decide which submachines support NUMA.
Suggested-by: Igor Mammedov
Suggested-by: Eduardo Habkost
Signed-off-by: Tao Xu
---
No changes in v7.
Changes in v6:
- Rebase to upstream, move globals in arm/sbsa-ref and use
numa_mem_supported
- When used once or twice in the function, use
ms->numa_state->num_nodes directly
- Correct some mistakes
- Use once monitor_printf in hmp_info_numa
---
exec.c | 5 ++-
hw/acpi/aml-build.c | 3 +-
hw/arm/boot.c | 4 +-
hw/arm/sbsa-ref.c | 4 +-
hw/arm/virt-acpi-build.c| 10 +++--
hw/arm/virt.c | 4 +-
hw/core/machine-hmp-cmds.c | 12 --
hw/core/machine.c | 14 +--
hw/core/numa.c | 60 +
hw/i386/acpi-build.c| 2 +-
hw/i386/pc.c| 9 +++--
hw/mem/pc-dimm.c| 2 +
hw/pci-bridge/pci_expander_bridge.c | 3 +-
hw/ppc/spapr.c | 23 +--
include/hw/acpi/aml-build.h | 2 +-
include/hw/boards.h | 1 +
include/sysemu/numa.h | 10 -
17 files changed, 107 insertions(+), 61 deletions(-)
diff --git a/exec.c b/exec.c
index 50ea9c5aaa..b6b75d2ad5 100644
--- a/exec.c
+++ b/exec.c
@@ -1736,6 +1736,7 @@ long qemu_minrampagesize(void)
long hpsize = LONG_MAX;
long mainrampagesize;
Object *memdev_root;
+MachineState *ms = MACHINE(qdev_get_machine());
mainrampagesize = qemu_mempath_getpagesize(mem_path);
@@ -1763,7 +1764,9 @@ long qemu_minrampagesize(void)
* so if its page size is smaller we have got to report that size instead.
*/
if (hpsize > mainrampagesize &&
-(nb_numa_nodes == 0 || numa_info[0].node_memdev == NULL)) {
+(ms->numa_state == NULL ||
+ ms->numa_state->num_nodes == 0 ||
+ numa_info[0].node_memdev == NULL)) {
static bool warned;
if (!warned) {
error_report("Huge page support disabled (n/a for main memory).");
diff --git a/hw/acpi/aml-build.c b/hw/acpi/aml-build.c
index 555c24f21d..63c1cae8c9 100644
--- a/hw/acpi/aml-build.c
+++ b/hw/acpi/aml-build.c
@@ -1726,10 +1726,11 @@ void build_srat_memory(AcpiSratMemoryAffinity *numamem,
uint64_t base,
* ACPI spec 5.2.17 System Locality Distance Information Table
* (Revision 2.0 or later)
*/
-void build_slit(GArray *table_data, BIOSLinker *linker)
+void build_slit(GArray *table_data, BIOSLinker *linker, MachineState *ms)
{
int slit_start, i, j;
slit_start = table_data->len;
+int nb_numa_nodes = ms->numa_state->num_nodes;
acpi_data_push(table_data, sizeof(AcpiTableHeader));
diff --git a/hw/arm/boot.c b/hw/arm/boot.c
index a90151f465..e28daa5278 100644
--- a/hw/arm/boot.c
+++ b/hw/arm/boot.c
@@ -598,9 +598,9 @@ int arm_load_dtb(hwaddr addr, const struct arm_boot_info
*binfo,
}
g_strfreev(node_path);
-if (nb_numa_nodes > 0) {
+if (ms->numa_state != NULL && ms->numa_state->num_nodes > 0) {
mem_base = binfo->loader_start;
-for (i = 0; i < nb_numa_nodes; i++) {
+for (i = 0; i < ms->numa_state->num_nodes; i++) {
mem_len = numa_info[i].node_mem;
rc = fdt_add_memory_node(fdt, acells, mem_base,
scells, mem_len, i);
diff --git a/hw/arm/sbsa-ref.c b/hw/arm/sbsa-ref.c
index 2aba3c58c5..22847909bf 100644
--- a/hw/arm/sbsa-ref.c
+++ b/hw/arm/sbsa-ref.c
@@ -144,6 +144,7 @@ static void create_fdt(SBSAMachineState *sms)
{
void *fdt = create_device_tree(>fdt_size);
const MachineState *ms = MACHINE(sms);
+int nb_numa_nodes = ms->numa_state->num_nodes;
int cpu;
if (!fdt) {
@@ -760,7 +761,7 @@ sbsa_ref_cpu_index_to_props(MachineState *ms, unsigned
cpu_index)
static int64_t
sbsa_ref_get_default_cpu_node_id(const MachineState *ms, int idx)
{
-return idx % nb_numa_nodes;
+return idx % ms->numa_state->num_nodes;
}
static void sbsa_ref_instance_init(Object *obj)
@@ -787,6 +788,7 @@ static void sbsa_ref_class_init(ObjectClass *oc, void *data)
mc->possible_cpu_arch_ids = sbsa_ref_possible_cpu_arch_ids;
mc->cpu_index_to_instance_props = sbsa_ref_cpu_index_to_props;
mc->get_default_cpu_node_id = sbsa_ref_get_default_cpu_node_id;
+mc->numa_mem_supported = true;
}
static const TypeInfo sbsa_ref_info = {
diff --git a/hw/arm/virt-acpi-build.c b/hw/arm/virt-acpi-build.c
index 0afb372769..a2cc4b84fe 100644
--- a/hw/arm/virt-acpi-build.c
+++ b/hw/arm/virt-acpi-build.c
@@ -516,7 +516,8 @@ build_srat(GArray *table_data, BIOSLinker *linker,
VirtMachineState *vms)
int i, srat_start;
uint64_t mem_base;
MachineClass *mc =
