Re: [Qemu-devel] [PATCH v7 02/11] numa: move numa global variable nb_numa_nodes into MachineState

[Igor Mammedov]

On Wed, 24 Jul 2019 12:02:41 -0300
Eduardo Habkost <address@hidden> 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 <address@hidden> 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 <address@hidden> 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 <address@hidden>
> > > > > Suggested-by: Eduardo Habkost <address@hidden>
> > > > > Signed-off-by: Tao Xu <address@hidden>
> > > > > ---
> > > > > 
> > > > > 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.
>