| Message ID | 20220120213151.220418-1-mmichels@redhat.com |
|---|---|
| State | Accepted |
| Headers | show |
| Series | [ovs-dev] ovn-parallel-hmap: Fix NUMA and core detection. | expand |
| Context | Check | Description |
|---|---|---|
| ovsrobot/apply-robot | success | apply and check: success |
| ovsrobot/github-robot-_Build_and_Test | fail | github build: failed |
| ovsrobot/github-robot-_ovn-kubernetes | success | github build: passed |
Hi Mark Thanks for fixing this. Nice catch... Should the --dummy-numa option not also be added to northd documentation? Does the --dummy-numa option have any other effect than setting the number of threads? I mean does it have for instance any "numa" or pinning effect? e.g. is there any difference between --dummy-numa=(0,0,0,0) and dummy-numa=(0,1,0,1,0,1,0,1) ? I do not think so, but I think it should be documented as --dummy-numa might seem a complex option for specifying the number of threads. Thanks Xavier On Thu, Jan 20, 2022 at 10:32 PM Mark Michelson <mmichels@redhat.com> wrote: > ovn-parallel-hmap attempted to determine the number of threads to > allocate for its pool based on the NUMA and core count reported by OVS. > The problem is that since ovn-northd never called ovs_numa_init(), OVS > would always return OVS_NUMA_UNSPEC for the number of NUMAs, and > OVS_CORE_UNSPEC for the number of cores. Parallelization still was > operational because we would fall back to a CPU core count. On a single > NUMA machine, this is identical to what was expected, so everything > seemed fine. > > In 37ad427cfb78a9e59d7e7ef249b2f2b3ac68c65a, the --dumy-numa option was > added to ovn-northd to allow for parallel operation on machines with > fewer NUMAs and cores than the parallelization code otherwise would > require. However, because of the missing call to ovs_numa_init(), this > option did not function as intended. > > This commit adds a call to ovs_numa_init() when setting up the parallel > hmap thread pool so that accurate NUMA and core counts are reported, and > dummy NUMA and core counts can be used. > > The referenced bugzilla is not actually reporting this as an issue, but > this particular fix will enable the functionality requested. > > Reported-at: https://bugzilla.redhat.com/show_bug.cgi?id=1975345 > Signed-off-by: Mark Michelson <mmichels@redhat.com> > --- > lib/ovn-parallel-hmap.c | 1 + > 1 file changed, 1 insertion(+) > > diff --git a/lib/ovn-parallel-hmap.c b/lib/ovn-parallel-hmap.c > index 56ceed8e8..7edc4c0b6 100644 > --- a/lib/ovn-parallel-hmap.c > +++ b/lib/ovn-parallel-hmap.c > @@ -404,6 +404,7 @@ static void > setup_worker_pools(bool force) { > int cores, nodes; > > + ovs_numa_init(); > nodes = ovs_numa_get_n_numas(); > if (nodes == OVS_NUMA_UNSPEC || nodes <= 0) { > nodes = 1; > -- > 2.31.1 > > _______________________________________________ > dev mailing list > dev@openvswitch.org > https://mail.openvswitch.org/mailman/listinfo/ovs-dev > >
On Thu, Jan 27, 2022 at 6:38 AM Xavier Simonart <xsimonar@redhat.com> wrote: > > Hi Mark > > Thanks for fixing this. Nice catch... > Should the --dummy-numa option not also be added to northd documentation? +1 to document this option in northd. I'm fine if you want to document it in a separate patch. Acked-by: Numan Siddique <numans@ovn.org> Numan > Does the --dummy-numa option have any other effect than setting the number > of threads? I mean does it have for instance any "numa" or pinning effect? > e.g. is there any difference between --dummy-numa=(0,0,0,0) and > dummy-numa=(0,1,0,1,0,1,0,1) ? > I do not think so, but I think it should be documented as --dummy-numa > might seem a complex option for specifying the number of threads. > Thanks > Xavier > > On Thu, Jan 20, 2022 at 10:32 PM Mark Michelson <mmichels@redhat.com> wrote: > > > ovn-parallel-hmap attempted to determine the number of threads to > > allocate for its pool based on the NUMA and core count reported by OVS. > > The problem is that since ovn-northd never called ovs_numa_init(), OVS > > would always return OVS_NUMA_UNSPEC for the number of NUMAs, and > > OVS_CORE_UNSPEC for the number of cores. Parallelization still was > > operational because we would fall back to a CPU core count. On a single > > NUMA machine, this is identical to what was expected, so everything > > seemed fine. > > > > In 37ad427cfb78a9e59d7e7ef249b2f2b3ac68c65a, the --dumy-numa option was > > added to ovn-northd to allow for parallel operation on machines with > > fewer NUMAs and cores than the parallelization code otherwise would > > require. However, because of the missing call to ovs_numa_init(), this > > option did not function as intended. > > > > This commit adds a call to ovs_numa_init() when setting up the parallel > > hmap thread pool so that accurate NUMA and core counts are reported, and > > dummy NUMA and core counts can be used. > > > > The referenced bugzilla is not actually reporting this as an issue, but > > this particular fix will enable the functionality requested. > > > > Reported-at: https://bugzilla.redhat.com/show_bug.cgi?id=1975345 > > Signed-off-by: Mark Michelson <mmichels@redhat.com> > > --- > > lib/ovn-parallel-hmap.c | 1 + > > 1 file changed, 1 insertion(+) > > > > diff --git a/lib/ovn-parallel-hmap.c b/lib/ovn-parallel-hmap.c > > index 56ceed8e8..7edc4c0b6 100644 > > --- a/lib/ovn-parallel-hmap.c > > +++ b/lib/ovn-parallel-hmap.c > > @@ -404,6 +404,7 @@ static void > > setup_worker_pools(bool force) { > > int cores, nodes; > > > > + ovs_numa_init(); > > nodes = ovs_numa_get_n_numas(); > > if (nodes == OVS_NUMA_UNSPEC || nodes <= 0) { > > nodes = 1; > > -- > > 2.31.1 > > > > _______________________________________________ > > dev mailing list > > dev@openvswitch.org > > https://mail.openvswitch.org/mailman/listinfo/ovs-dev > > > > > _______________________________________________ > dev mailing list > dev@openvswitch.org > https://mail.openvswitch.org/mailman/listinfo/ovs-dev >
On 2/1/22 17:04, Numan Siddique wrote: > On Thu, Jan 27, 2022 at 6:38 AM Xavier Simonart <xsimonar@redhat.com> wrote: >> >> Hi Mark >> >> Thanks for fixing this. Nice catch... >> Should the --dummy-numa option not also be added to northd documentation? > > +1 to document this option in northd. > > I'm fine if you want to document it in a separate patch. > > Acked-by: Numan Siddique <numans@ovn.org> > > Numan Thanks fellows, I pushed this change to main and branch-21.12. I also just posted a review that adds --dummy-numa documentation. > >> Does the --dummy-numa option have any other effect than setting the number >> of threads? I mean does it have for instance any "numa" or pinning effect? >> e.g. is there any difference between --dummy-numa=(0,0,0,0) and >> dummy-numa=(0,1,0,1,0,1,0,1) ? >> I do not think so, but I think it should be documented as --dummy-numa >> might seem a complex option for specifying the number of threads. > >> Thanks >> Xavier >> >> On Thu, Jan 20, 2022 at 10:32 PM Mark Michelson <mmichels@redhat.com> wrote: >> >>> ovn-parallel-hmap attempted to determine the number of threads to >>> allocate for its pool based on the NUMA and core count reported by OVS. >>> The problem is that since ovn-northd never called ovs_numa_init(), OVS >>> would always return OVS_NUMA_UNSPEC for the number of NUMAs, and >>> OVS_CORE_UNSPEC for the number of cores. Parallelization still was >>> operational because we would fall back to a CPU core count. On a single >>> NUMA machine, this is identical to what was expected, so everything >>> seemed fine. >>> >>> In 37ad427cfb78a9e59d7e7ef249b2f2b3ac68c65a, the --dumy-numa option was >>> added to ovn-northd to allow for parallel operation on machines with >>> fewer NUMAs and cores than the parallelization code otherwise would >>> require. However, because of the missing call to ovs_numa_init(), this >>> option did not function as intended. >>> >>> This commit adds a call to ovs_numa_init() when setting up the parallel >>> hmap thread pool so that accurate NUMA and core counts are reported, and >>> dummy NUMA and core counts can be used. >>> >>> The referenced bugzilla is not actually reporting this as an issue, but >>> this particular fix will enable the functionality requested. >>> >>> Reported-at: https://bugzilla.redhat.com/show_bug.cgi?id=1975345 >>> Signed-off-by: Mark Michelson <mmichels@redhat.com> >>> --- >>> lib/ovn-parallel-hmap.c | 1 + >>> 1 file changed, 1 insertion(+) >>> >>> diff --git a/lib/ovn-parallel-hmap.c b/lib/ovn-parallel-hmap.c >>> index 56ceed8e8..7edc4c0b6 100644 >>> --- a/lib/ovn-parallel-hmap.c >>> +++ b/lib/ovn-parallel-hmap.c >>> @@ -404,6 +404,7 @@ static void >>> setup_worker_pools(bool force) { >>> int cores, nodes; >>> >>> + ovs_numa_init(); >>> nodes = ovs_numa_get_n_numas(); >>> if (nodes == OVS_NUMA_UNSPEC || nodes <= 0) { >>> nodes = 1; >>> -- >>> 2.31.1 >>> >>> _______________________________________________ >>> dev mailing list >>> dev@openvswitch.org >>> https://mail.openvswitch.org/mailman/listinfo/ovs-dev >>> >>> >> _______________________________________________ >> dev mailing list >> dev@openvswitch.org >> https://mail.openvswitch.org/mailman/listinfo/ovs-dev >> >
diff --git a/lib/ovn-parallel-hmap.c b/lib/ovn-parallel-hmap.c index 56ceed8e8..7edc4c0b6 100644 --- a/lib/ovn-parallel-hmap.c +++ b/lib/ovn-parallel-hmap.c @@ -404,6 +404,7 @@ static void setup_worker_pools(bool force) { int cores, nodes; + ovs_numa_init(); nodes = ovs_numa_get_n_numas(); if (nodes == OVS_NUMA_UNSPEC || nodes <= 0) { nodes = 1;
ovn-parallel-hmap attempted to determine the number of threads to allocate for its pool based on the NUMA and core count reported by OVS. The problem is that since ovn-northd never called ovs_numa_init(), OVS would always return OVS_NUMA_UNSPEC for the number of NUMAs, and OVS_CORE_UNSPEC for the number of cores. Parallelization still was operational because we would fall back to a CPU core count. On a single NUMA machine, this is identical to what was expected, so everything seemed fine. In 37ad427cfb78a9e59d7e7ef249b2f2b3ac68c65a, the --dumy-numa option was added to ovn-northd to allow for parallel operation on machines with fewer NUMAs and cores than the parallelization code otherwise would require. However, because of the missing call to ovs_numa_init(), this option did not function as intended. This commit adds a call to ovs_numa_init() when setting up the parallel hmap thread pool so that accurate NUMA and core counts are reported, and dummy NUMA and core counts can be used. The referenced bugzilla is not actually reporting this as an issue, but this particular fix will enable the functionality requested. Reported-at: https://bugzilla.redhat.com/show_bug.cgi?id=1975345 Signed-off-by: Mark Michelson <mmichels@redhat.com> --- lib/ovn-parallel-hmap.c | 1 + 1 file changed, 1 insertion(+)