On 08.12.2017 18:44, Bodireddy, Bhanuprakash wrote: >> >> 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.
This should happen in dp_netdev_free() on ovs exit or deletion of the datapath. I guess, you need following patch to reproduce: https://mail.openvswitch.org/pipermail/ovs-dev/2017-December/341617.html Ian is going to include it to the closest pull request. Even if it's not reproducible you have to fix memory allocation for non_pmd anyway. Current code logically wrong. > > 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 <[email protected]> >>>> Signed-off-by: Bhanuprakash Bodireddy >>>> <[email protected]> >>>> --- >>>> 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 >>>> [email protected] >>>> https://mail.openvswitch.org/mailman/listinfo/ovs-dev >>> >>> >>> _______________________________________________ dev mailing list [email protected] https://mail.openvswitch.org/mailman/listinfo/ovs-dev
