| Message ID | 20211217131709.30994.5953.stgit@dceara.remote.csb |
|---|---|
| State | Changes Requested |
| Headers | show |
| Series | Fix UndefinedBehaviorSanitizer reported issues and enable it in CI. | expand |
| Context | Check | Description |
|---|---|---|
| ovsrobot/apply-robot | success | apply and check: success |
| ovsrobot/github-robot-_Build_and_Test | success | github build: passed |
On 12/17/21 14:17, Dumitru Ceara wrote: > UB Sanitizer report: > lib/dpif-netdev.c:6758:13: runtime error: member access within misaligned address 0x7f7f24d25010 for type 'struct dp_netdev_pmd_thread', which requires 64 byte alignment > 0x7f7f24d25010: note: pointer points here > 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 > ^ > #0 0x5fbfde in dp_netdev_configure_pmd lib/dpif-netdev.c:6758 > #1 0x5fbde9 in dp_netdev_set_nonpmd lib/dpif-netdev.c:6715 > #2 0x5d6fdd in create_dp_netdev lib/dpif-netdev.c:1769 > #3 0x5d72d0 in dpif_netdev_open lib/dpif-netdev.c:1807 > #4 0x61c83f in do_open lib/dpif.c:347 > [...] > lib/dpif-netdev.c:1724:6: runtime error: member access within misaligned address 0x000002005eb0 for type 'struct dp_netdev', which requires 64 byte alignment > 0x000002005eb0: note: pointer points here > 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 > ^ > #0 0x5d6660 in create_dp_netdev lib/dpif-netdev.c:1724 > #1 0x5d72d0 in dpif_netdev_open lib/dpif-netdev.c:1807 > #2 0x61c846 in do_open lib/dpif.c:347 > #3 0x61ca9c in dpif_create lib/dpif.c:402 > #4 0x61cac9 in dpif_create_and_open lib/dpif.c:415 > #5 0x48f235 in open_dpif_backer ofproto/ofproto-dpif.c:776 > [...] Oh, wow. So, that is just yet another point for removing all the cache line alignments from these structures. They are largely pointless and only create holes in the data structure that is dynamically allocated and not aligned in the first place. And apparently create a huge unfulfilled alignment requirements. I'd suggest to remove OVS_ALIGNED_VAR(CACHE_LINE_SIZE) markers from these structures instead, because they doesn't do anything useful. The one in the dp_netdev_pmd_thread structure actually is trying to align a huge dfc_cache structure, there is no reason keeping that aligned as it is literally hundreds of KBs in size. Data around emc_insert_min doesn't seem to be written by multiple threads on a hot path, so there is no risk of false sharing. There is also possibility that performance will improve since smc_enable_db and emc_insert_min will have a chance to be in the same cache line, hence a little bit less memory accesses. Best regards, Ilya Maximets. > > Signed-off-by: Dumitru Ceara <dceara@redhat.com> > --- > lib/dpif-netdev.c | 10 +++++----- > 1 file changed, 5 insertions(+), 5 deletions(-) > > diff --git a/lib/dpif-netdev.c b/lib/dpif-netdev.c > index f62202a8b53d..25d8a3ff2009 100644 > --- a/lib/dpif-netdev.c > +++ b/lib/dpif-netdev.c > @@ -1718,7 +1718,7 @@ create_dp_netdev(const char *name, const struct dpif_class *class, > ovsthread_once_done(&tsc_freq_check); > } > > - dp = xzalloc(sizeof *dp); > + dp = xzalloc_cacheline(sizeof *dp); > shash_add(&dp_netdevs, name, dp); > > *CONST_CAST(const struct dpif_class **, &dp->class) = class; > @@ -1888,7 +1888,7 @@ dp_netdev_free(struct dp_netdev *dp) > > free(dp->pmd_cmask); > free(CONST_CAST(char *, dp->name)); > - free(dp); > + free_cacheline(dp); > } > > static void > @@ -5672,7 +5672,7 @@ reconfigure_pmd_threads(struct dp_netdev *dp) > if (!pmd) { > struct ds name = DS_EMPTY_INITIALIZER; > > - pmd = xzalloc(sizeof *pmd); > + pmd = xzalloc_cacheline(sizeof *pmd); > dp_netdev_configure_pmd(pmd, dp, core->core_id, core->numa_id); > > ds_put_format(&name, "pmd-c%02d/id:", core->core_id); > @@ -6713,7 +6713,7 @@ dp_netdev_set_nonpmd(struct dp_netdev *dp) > { > struct dp_netdev_pmd_thread *non_pmd; > > - non_pmd = xzalloc(sizeof *non_pmd); > + non_pmd = xzalloc_cacheline(sizeof *non_pmd); > dp_netdev_configure_pmd(non_pmd, dp, NON_PMD_CORE_ID, OVS_NUMA_UNSPEC); > } > > @@ -6830,7 +6830,7 @@ dp_netdev_destroy_pmd(struct dp_netdev_pmd_thread *pmd) > seq_destroy(pmd->reload_seq); > ovs_mutex_destroy(&pmd->port_mutex); > ovs_mutex_destroy(&pmd->bond_mutex); > - free(pmd); > + free_cacheline(pmd); > } > > /* Stops the pmd thread, removes it from the 'dp->poll_threads', >
On 12/17/21 15:15, Ilya Maximets wrote: > On 12/17/21 14:17, Dumitru Ceara wrote: >> UB Sanitizer report: >> lib/dpif-netdev.c:6758:13: runtime error: member access within misaligned address 0x7f7f24d25010 for type 'struct dp_netdev_pmd_thread', which requires 64 byte alignment >> 0x7f7f24d25010: note: pointer points here >> 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 >> ^ >> #0 0x5fbfde in dp_netdev_configure_pmd lib/dpif-netdev.c:6758 >> #1 0x5fbde9 in dp_netdev_set_nonpmd lib/dpif-netdev.c:6715 >> #2 0x5d6fdd in create_dp_netdev lib/dpif-netdev.c:1769 >> #3 0x5d72d0 in dpif_netdev_open lib/dpif-netdev.c:1807 >> #4 0x61c83f in do_open lib/dpif.c:347 >> [...] >> lib/dpif-netdev.c:1724:6: runtime error: member access within misaligned address 0x000002005eb0 for type 'struct dp_netdev', which requires 64 byte alignment >> 0x000002005eb0: note: pointer points here >> 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 >> ^ >> #0 0x5d6660 in create_dp_netdev lib/dpif-netdev.c:1724 >> #1 0x5d72d0 in dpif_netdev_open lib/dpif-netdev.c:1807 >> #2 0x61c846 in do_open lib/dpif.c:347 >> #3 0x61ca9c in dpif_create lib/dpif.c:402 >> #4 0x61cac9 in dpif_create_and_open lib/dpif.c:415 >> #5 0x48f235 in open_dpif_backer ofproto/ofproto-dpif.c:776 >> [...] > > Oh, wow. So, that is just yet another point for removing all the > cache line alignments from these structures. They are largely > pointless and only create holes in the data structure that is > dynamically allocated and not aligned in the first place. I don't know the history behind the cache alignment inside these structures but now that you mentioned it I tend to agree with you. :) > And apparently create a huge unfulfilled alignment requirements. Right. > > I'd suggest to remove OVS_ALIGNED_VAR(CACHE_LINE_SIZE) markers > from these structures instead, because they doesn't do anything > useful. I tried it out, it works fine. > > The one in the dp_netdev_pmd_thread structure actually is trying > to align a huge dfc_cache structure, there is no reason keeping > that aligned as it is literally hundreds of KBs in size. > Data around emc_insert_min doesn't seem to be written by multiple > threads on a hot path, so there is no risk of false sharing. > There is also possibility that performance will improve since > smc_enable_db and emc_insert_min will have a chance to be in the > same cache line, hence a little bit less memory accesses. I don't really have a setup to test the performance implications of such a change. Maybe someone from the OVS community can help out with that? In any case, I'll wait some more for review comments (on the other patches of the series too) before posting v2 with this suggested change. > > Best regards, Ilya Maximets. > Thanks, Dumitru
On 12/17/21 15:36, Dumitru Ceara wrote: > On 12/17/21 15:15, Ilya Maximets wrote: >> On 12/17/21 14:17, Dumitru Ceara wrote: >>> UB Sanitizer report: >>> lib/dpif-netdev.c:6758:13: runtime error: member access within misaligned address 0x7f7f24d25010 for type 'struct dp_netdev_pmd_thread', which requires 64 byte alignment >>> 0x7f7f24d25010: note: pointer points here >>> 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 >>> ^ >>> #0 0x5fbfde in dp_netdev_configure_pmd lib/dpif-netdev.c:6758 >>> #1 0x5fbde9 in dp_netdev_set_nonpmd lib/dpif-netdev.c:6715 >>> #2 0x5d6fdd in create_dp_netdev lib/dpif-netdev.c:1769 >>> #3 0x5d72d0 in dpif_netdev_open lib/dpif-netdev.c:1807 >>> #4 0x61c83f in do_open lib/dpif.c:347 >>> [...] >>> lib/dpif-netdev.c:1724:6: runtime error: member access within misaligned address 0x000002005eb0 for type 'struct dp_netdev', which requires 64 byte alignment >>> 0x000002005eb0: note: pointer points here >>> 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 >>> ^ >>> #0 0x5d6660 in create_dp_netdev lib/dpif-netdev.c:1724 >>> #1 0x5d72d0 in dpif_netdev_open lib/dpif-netdev.c:1807 >>> #2 0x61c846 in do_open lib/dpif.c:347 >>> #3 0x61ca9c in dpif_create lib/dpif.c:402 >>> #4 0x61cac9 in dpif_create_and_open lib/dpif.c:415 >>> #5 0x48f235 in open_dpif_backer ofproto/ofproto-dpif.c:776 >>> [...] >> >> Oh, wow. So, that is just yet another point for removing all the >> cache line alignments from these structures. They are largely >> pointless and only create holes in the data structure that is >> dynamically allocated and not aligned in the first place. > > I don't know the history behind the cache alignment inside these > structures but now that you mentioned it I tend to agree with you. :) > >> And apparently create a huge unfulfilled alignment requirements. > > Right. > >> >> I'd suggest to remove OVS_ALIGNED_VAR(CACHE_LINE_SIZE) markers >> from these structures instead, because they doesn't do anything >> useful. > > I tried it out, it works fine. > >> >> The one in the dp_netdev_pmd_thread structure actually is trying >> to align a huge dfc_cache structure, there is no reason keeping >> that aligned as it is literally hundreds of KBs in size. >> Data around emc_insert_min doesn't seem to be written by multiple >> threads on a hot path, so there is no risk of false sharing. >> There is also possibility that performance will improve since >> smc_enable_db and emc_insert_min will have a chance to be in the >> same cache line, hence a little bit less memory accesses. > > I don't really have a setup to test the performance implications of such > a change. Maybe someone from the OVS community can help out with that? > > In any case, I'll wait some more for review comments (on the other > patches of the series too) before posting v2 with this suggested change. > V2 posted: https://patchwork.ozlabs.org/project/openvswitch/list/?series=277900&state=* Thanks!
diff --git a/lib/dpif-netdev.c b/lib/dpif-netdev.c index f62202a8b53d..25d8a3ff2009 100644 --- a/lib/dpif-netdev.c +++ b/lib/dpif-netdev.c @@ -1718,7 +1718,7 @@ create_dp_netdev(const char *name, const struct dpif_class *class, ovsthread_once_done(&tsc_freq_check); } - dp = xzalloc(sizeof *dp); + dp = xzalloc_cacheline(sizeof *dp); shash_add(&dp_netdevs, name, dp); *CONST_CAST(const struct dpif_class **, &dp->class) = class; @@ -1888,7 +1888,7 @@ dp_netdev_free(struct dp_netdev *dp) free(dp->pmd_cmask); free(CONST_CAST(char *, dp->name)); - free(dp); + free_cacheline(dp); } static void @@ -5672,7 +5672,7 @@ reconfigure_pmd_threads(struct dp_netdev *dp) if (!pmd) { struct ds name = DS_EMPTY_INITIALIZER; - pmd = xzalloc(sizeof *pmd); + pmd = xzalloc_cacheline(sizeof *pmd); dp_netdev_configure_pmd(pmd, dp, core->core_id, core->numa_id); ds_put_format(&name, "pmd-c%02d/id:", core->core_id); @@ -6713,7 +6713,7 @@ dp_netdev_set_nonpmd(struct dp_netdev *dp) { struct dp_netdev_pmd_thread *non_pmd; - non_pmd = xzalloc(sizeof *non_pmd); + non_pmd = xzalloc_cacheline(sizeof *non_pmd); dp_netdev_configure_pmd(non_pmd, dp, NON_PMD_CORE_ID, OVS_NUMA_UNSPEC); } @@ -6830,7 +6830,7 @@ dp_netdev_destroy_pmd(struct dp_netdev_pmd_thread *pmd) seq_destroy(pmd->reload_seq); ovs_mutex_destroy(&pmd->port_mutex); ovs_mutex_destroy(&pmd->bond_mutex); - free(pmd); + free_cacheline(pmd); } /* Stops the pmd thread, removes it from the 'dp->poll_threads',
UB Sanitizer report: lib/dpif-netdev.c:6758:13: runtime error: member access within misaligned address 0x7f7f24d25010 for type 'struct dp_netdev_pmd_thread', which requires 64 byte alignment 0x7f7f24d25010: note: pointer points here 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 ^ #0 0x5fbfde in dp_netdev_configure_pmd lib/dpif-netdev.c:6758 #1 0x5fbde9 in dp_netdev_set_nonpmd lib/dpif-netdev.c:6715 #2 0x5d6fdd in create_dp_netdev lib/dpif-netdev.c:1769 #3 0x5d72d0 in dpif_netdev_open lib/dpif-netdev.c:1807 #4 0x61c83f in do_open lib/dpif.c:347 [...] lib/dpif-netdev.c:1724:6: runtime error: member access within misaligned address 0x000002005eb0 for type 'struct dp_netdev', which requires 64 byte alignment 0x000002005eb0: note: pointer points here 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 ^ #0 0x5d6660 in create_dp_netdev lib/dpif-netdev.c:1724 #1 0x5d72d0 in dpif_netdev_open lib/dpif-netdev.c:1807 #2 0x61c846 in do_open lib/dpif.c:347 #3 0x61ca9c in dpif_create lib/dpif.c:402 #4 0x61cac9 in dpif_create_and_open lib/dpif.c:415 #5 0x48f235 in open_dpif_backer ofproto/ofproto-dpif.c:776 [...] Signed-off-by: Dumitru Ceara <dceara@redhat.com> --- lib/dpif-netdev.c | 10 +++++----- 1 file changed, 5 insertions(+), 5 deletions(-)