> >On 08.12.2017 16:45, Stokes, Ian wrote: >>> All instances of struct dp_netdev_pmd_thread are allocated by xzalloc >>> and therefore doesn't guarantee memory allocation aligned on >>> CACHE_LINE_SIZE boundary. Due to this any padding done inside the >>> structure with this assumption might create holes. >>> >>> This commit replaces xzalloc, free with xzalloc_cacheline and >>> free_cacheline. With the changes the memory is 64 byte aligned. >> >> Thanks for this Bhanu, >> >> I think this looks OK and I'm considering pushing to the DPDK_Merge branch >but as there has been a fair bit of debate lately regarding memory and cache >alignment I want to flag to others who have engaged to date to have their say >before I apply it as there has been no input yet for the patch. >> >> @Jan/Ilya, are you ok with this change? > >OVS will likely crash on destroying non_pmd thread because it still allocated >by >usual xzalloc, but freed with others by free_cacheline().
Are you sure OvS crashes in this case and reproducible? Firstly I didn't see a crash and to double check this I enabled a DBG in dp_netdev_destroy_pmd() to see if free_cacheline() is called for the non pmd thread (whose core_id is NON_PMD_CORE_ID) and that doesn't seem to be hitting and gets hit only for pmd threads having valid core_ids. Also AFAIK, non pmd thread is nothing but vswitchd thread and I don’t see how that can be freed from the above function. Also I started wondering where the memory allocated for non_pmd thread is getting freed now. Let me know the steps if you can reproduce the crash as you mentioned. - Bhanuprakash. > >> >> Thanks >> Ian >> >>> >>> Before: >>> With xzalloc, all the memory is 16 byte aligned. >>> >>> (gdb) p pmd >>> $1 = (struct dp_netdev_pmd_thread *) 0x7eff8a813010 >>> (gdb) p &pmd->cacheline0 >>> $2 = (OVS_CACHE_LINE_MARKER *) 0x7eff8a813010 >>> (gdb) p &pmd->cacheline1 >>> $3 = (OVS_CACHE_LINE_MARKER *) 0x7eff8a813050 >>> (gdb) p &pmd->flow_cache >>> $4 = (struct emc_cache *) 0x7eff8a813090 >>> (gdb) p &pmd->flow_table >>> $5 = (struct cmap *) 0x7eff8acb30d0 >>> (gdb) p &pmd->stats >>> $6 = (struct dp_netdev_pmd_stats *) 0x7eff8acb3110 >>> (gdb) p &pmd->port_mutex >>> $7 = (struct ovs_mutex *) 0x7eff8acb3150 >>> (gdb) p &pmd->poll_list >>> $8 = (struct hmap *) 0x7eff8acb3190 >>> (gdb) p &pmd->tnl_port_cache >>> $9 = (struct hmap *) 0x7eff8acb31d0 >>> (gdb) p &pmd->stats_zero >>> $10 = (unsigned long long (*)[5]) 0x7eff8acb3210 >>> >>> After: >>> With xzalloc_cacheline, all the memory is 64 byte aligned. >>> >>> (gdb) p pmd >>> $1 = (struct dp_netdev_pmd_thread *) 0x7f39e2365040 >>> (gdb) p &pmd->cacheline0 >>> $2 = (OVS_CACHE_LINE_MARKER *) 0x7f39e2365040 >>> (gdb) p &pmd->cacheline1 >>> $3 = (OVS_CACHE_LINE_MARKER *) 0x7f39e2365080 >>> (gdb) p &pmd->flow_cache >>> $4 = (struct emc_cache *) 0x7f39e23650c0 >>> (gdb) p &pmd->flow_table >>> $5 = (struct cmap *) 0x7f39e2805100 >>> (gdb) p &pmd->stats >>> $6 = (struct dp_netdev_pmd_stats *) 0x7f39e2805140 >>> (gdb) p &pmd->port_mutex >>> $7 = (struct ovs_mutex *) 0x7f39e2805180 >>> (gdb) p &pmd->poll_list >>> $8 = (struct hmap *) 0x7f39e28051c0 >>> (gdb) p &pmd->tnl_port_cache >>> $9 = (struct hmap *) 0x7f39e2805200 >>> (gdb) p &pmd->stats_zero >>> $10 = (unsigned long long (*)[5]) 0x7f39e2805240 >>> >>> Reported-by: Ilya Maximets <i.maxim...@samsung.com> >>> Signed-off-by: Bhanuprakash Bodireddy >>> <bhanuprakash.bodire...@intel.com> >>> --- >>> lib/dpif-netdev.c | 4 ++-- >>> 1 file changed, 2 insertions(+), 2 deletions(-) >>> >>> diff --git a/lib/dpif-netdev.c b/lib/dpif-netdev.c index >>> db78318..3e281ae >>> 100644 >>> --- a/lib/dpif-netdev.c >>> +++ b/lib/dpif-netdev.c >>> @@ -3646,7 +3646,7 @@ reconfigure_pmd_threads(struct dp_netdev >*dp) >>> FOR_EACH_CORE_ON_DUMP(core, pmd_cores) { >>> pmd = dp_netdev_get_pmd(dp, core->core_id); >>> if (!pmd) { >>> - pmd = xzalloc(sizeof *pmd); >>> + pmd = xzalloc_cacheline(sizeof *pmd); >>> dp_netdev_configure_pmd(pmd, dp, core->core_id, core- >>>> numa_id); >>> pmd->thread = ovs_thread_create("pmd", pmd_thread_main, >pmd); >>> VLOG_INFO("PMD thread on numa_id: %d, core id: %2d >>> created.", @@ -4574,7 +4574,7 @@ dp_netdev_destroy_pmd(struct >>> dp_netdev_pmd_thread >>> *pmd) >>> xpthread_cond_destroy(&pmd->cond); >>> ovs_mutex_destroy(&pmd->cond_mutex); >>> ovs_mutex_destroy(&pmd->port_mutex); >>> - free(pmd); >>> + free_cacheline(pmd); >>> } >>> >>> /* Stops the pmd thread, removes it from the 'dp->poll_threads', >>> -- >>> 2.4.11 >>> >>> _______________________________________________ >>> dev mailing list >>> d...@openvswitch.org >>> https://mail.openvswitch.org/mailman/listinfo/ovs-dev >> >> >> _______________________________________________ dev mailing list d...@openvswitch.org https://mail.openvswitch.org/mailman/listinfo/ovs-dev