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