From: Eduardo Habkost <ehabkost@redhat.com>
To: Igor Mammedov <imammedo@redhat.com>
Cc: jingqi.liu@intel.com, Tao Xu <tao3.xu@intel.com>,
fan.du@intel.com, qemu-devel@nongnu.org,
jonathan.cameron@huawei.com, dan.j.williams@intel.com
Subject: Re: [Qemu-devel] [PATCH v7 02/11] numa: move numa global variable nb_numa_nodes into MachineState
Date: Wed, 24 Jul 2019 12:02:41 -0300 [thread overview]
Message-ID: <20190724150241.GM11469@habkost.net> (raw)
In-Reply-To: <20190724162721.736f7efe@Igors-MacBook-Pro>
On Wed, Jul 24, 2019 at 04:27:21PM +0200, Igor Mammedov wrote:
> On Tue, 23 Jul 2019 12:23:57 -0300
> Eduardo Habkost <ehabkost@redhat.com> 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 <tao3.xu@intel.com> 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 <imammedo@redhat.com>
> > > > Suggested-by: Eduardo Habkost <ehabkost@redhat.com>
> > > > Signed-off-by: Tao Xu <tao3.xu@intel.com>
> > > > ---
> > > >
> > > > 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
next prev parent reply other threads:[~2019-07-24 15:03 UTC|newest]
Thread overview: 28+ messages / expand[flat|nested] mbox.gz Atom feed top
2019-07-16 14:51 [Qemu-devel] [PATCH v7 00/11] Build ACPI Heterogeneous Memory Attribute Table (HMAT) Tao Xu
2019-07-16 14:51 ` [Qemu-devel] [PATCH v7 01/11] hw/arm: simplify arm_load_dtb Tao Xu
2019-07-23 14:59 ` Igor Mammedov
2019-07-26 8:26 ` Tao Xu
2019-07-16 14:51 ` [Qemu-devel] [PATCH v7 02/11] numa: move numa global variable nb_numa_nodes into MachineState Tao Xu
2019-07-23 14:56 ` Igor Mammedov
2019-07-23 15:23 ` Eduardo Habkost
2019-07-24 14:27 ` Igor Mammedov
2019-07-24 15:02 ` Eduardo Habkost [this message]
2019-07-24 15:48 ` Igor Mammedov
2019-07-24 18:15 ` Eduardo Habkost
2019-07-26 13:43 ` Igor Mammedov
2019-07-26 14:17 ` Eduardo Habkost
2019-07-29 6:58 ` Tao Xu
2019-07-16 14:51 ` [Qemu-devel] [PATCH v7 03/11] numa: move numa global variable have_numa_distance " Tao Xu
2019-07-16 14:51 ` [Qemu-devel] [PATCH v7 04/11] numa: move numa global variable numa_info " Tao Xu
2019-07-24 14:46 ` Igor Mammedov
2019-07-16 14:51 ` [Qemu-devel] [PATCH v7 05/11] numa: Extend CLI to provide initiator information for numa nodes Tao Xu
2019-07-22 2:37 ` Liu, Jingqi
2019-07-16 14:51 ` [Qemu-devel] [PATCH v7 06/11] hmat acpi: Build Memory Proximity Domain Attributes Structure(s) Tao Xu
2019-07-16 14:51 ` [Qemu-devel] [PATCH v7 07/11] hmat acpi: Build System Locality Latency and Bandwidth Information Structure(s) Tao Xu
2019-07-16 14:51 ` [Qemu-devel] [PATCH v7 08/11] hmat acpi: Build Memory Side Cache " Tao Xu
2019-07-16 14:51 ` [Qemu-devel] [PATCH v7 09/11] numa: Extend the CLI to provide memory latency and bandwidth information Tao Xu
2019-07-16 14:51 ` [Qemu-devel] [PATCH v7 10/11] numa: Extend the CLI to provide memory side cache information Tao Xu
2019-07-16 14:51 ` [Qemu-devel] [PATCH v7 11/11] tests/bios-tables-test: add test cases for ACPI HMAT Tao Xu
2019-07-22 2:57 ` Liu, Jingqi
2019-07-17 13:17 ` [Qemu-devel] [PATCH v7 00/11] Build ACPI Heterogeneous Memory Attribute Table (HMAT) no-reply
2019-07-19 6:32 ` Tao Xu
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=20190724150241.GM11469@habkost.net \
--to=ehabkost@redhat.com \
--cc=dan.j.williams@intel.com \
--cc=fan.du@intel.com \
--cc=imammedo@redhat.com \
--cc=jingqi.liu@intel.com \
--cc=jonathan.cameron@huawei.com \
--cc=qemu-devel@nongnu.org \
--cc=tao3.xu@intel.com \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).