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! _______________________________________________ dev mailing list [email protected] https://mail.openvswitch.org/mailman/listinfo/ovs-dev
