[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: [Qemu-devel] [PATCH v7 02/11] numa: move numa global variable nb_num
From: |
Eduardo Habkost |
Subject: |
Re: [Qemu-devel] [PATCH v7 02/11] numa: move numa global variable nb_numa_nodes into MachineState |
Date: |
Fri, 26 Jul 2019 11:17:42 -0300 |
On Fri, Jul 26, 2019 at 03:43:43PM +0200, Igor Mammedov wrote:
> On Wed, 24 Jul 2019 15:15:28 -0300
> Eduardo Habkost <address@hidden> 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 <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.
> >
> > 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.
--
Eduardo
- Re: [Qemu-devel] [PATCH v7 01/11] hw/arm: simplify arm_load_dtb, (continued)
- [Qemu-devel] [PATCH v7 02/11] numa: move numa global variable nb_numa_nodes into MachineState, Tao Xu, 2019/07/16
- Re: [Qemu-devel] [PATCH v7 02/11] numa: move numa global variable nb_numa_nodes into MachineState, Igor Mammedov, 2019/07/23
- Re: [Qemu-devel] [PATCH v7 02/11] numa: move numa global variable nb_numa_nodes into MachineState, Eduardo Habkost, 2019/07/23
- Re: [Qemu-devel] [PATCH v7 02/11] numa: move numa global variable nb_numa_nodes into MachineState, Igor Mammedov, 2019/07/24
- Re: [Qemu-devel] [PATCH v7 02/11] numa: move numa global variable nb_numa_nodes into MachineState, Eduardo Habkost, 2019/07/24
- Re: [Qemu-devel] [PATCH v7 02/11] numa: move numa global variable nb_numa_nodes into MachineState, Igor Mammedov, 2019/07/24
- Re: [Qemu-devel] [PATCH v7 02/11] numa: move numa global variable nb_numa_nodes into MachineState, Eduardo Habkost, 2019/07/24
- Re: [Qemu-devel] [PATCH v7 02/11] numa: move numa global variable nb_numa_nodes into MachineState, Igor Mammedov, 2019/07/26
- Re: [Qemu-devel] [PATCH v7 02/11] numa: move numa global variable nb_numa_nodes into MachineState,
Eduardo Habkost <=
- Re: [Qemu-devel] [PATCH v7 02/11] numa: move numa global variable nb_numa_nodes into MachineState, Tao Xu, 2019/07/29
[Qemu-devel] [PATCH v7 04/11] numa: move numa global variable numa_info into MachineState, Tao Xu, 2019/07/16
[Qemu-devel] [PATCH v7 05/11] numa: Extend CLI to provide initiator information for numa nodes, Tao Xu, 2019/07/16
[Qemu-devel] [PATCH v7 06/11] hmat acpi: Build Memory Proximity Domain Attributes Structure(s), Tao Xu, 2019/07/16
[Qemu-devel] [PATCH v7 08/11] hmat acpi: Build Memory Side Cache Information Structure(s), Tao Xu, 2019/07/16
[Qemu-devel] [PATCH v7 07/11] hmat acpi: Build System Locality Latency and Bandwidth Information Structure(s), Tao Xu, 2019/07/16
[Qemu-devel] [PATCH v7 09/11] numa: Extend the CLI to provide memory latency and bandwidth information, Tao Xu, 2019/07/16
[Qemu-devel] [PATCH v7 11/11] tests/bios-tables-test: add test cases for ACPI HMAT, Tao Xu, 2019/07/16