| Message ID | 1511732875-8514-1-git-send-email-bhanuprakash.bodireddy@intel.com |
|---|---|
| State | Changes Requested |
| Delegated to: | Ian Stokes |
| Headers | show |
| Series | [ovs-dev] dpif-netdev: Allocate dp_netdev_pmd_thread struct by xzalloc_cacheline. | expand |
> 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? 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.maximets@samsung.com> > Signed-off-by: Bhanuprakash Bodireddy <bhanuprakash.bodireddy@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 > dev@openvswitch.org > https://mail.openvswitch.org/mailman/listinfo/ovs-dev
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(). > > 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.maximets@samsung.com> >> Signed-off-by: Bhanuprakash Bodireddy <bhanuprakash.bodireddy@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 >> dev@openvswitch.org >> https://mail.openvswitch.org/mailman/listinfo/ovs-dev > > >
> >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.maximets@samsung.com> >>> Signed-off-by: Bhanuprakash Bodireddy >>> <bhanuprakash.bodireddy@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 >>> dev@openvswitch.org >>> https://mail.openvswitch.org/mailman/listinfo/ovs-dev >> >> >>
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 <i.maximets@samsung.com> >>>> Signed-off-by: Bhanuprakash Bodireddy >>>> <bhanuprakash.bodireddy@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 >>>> dev@openvswitch.org >>>> https://mail.openvswitch.org/mailman/listinfo/ovs-dev >>> >>> >>>
> >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. Ok, that makes sense I would use the xzalloc_cacheline() for allocating memory for non_pmd too.. Bhanuprakash. > >> >> 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.maximets@samsung.com> >>>>> Signed-off-by: Bhanuprakash Bodireddy >>>>> <bhanuprakash.bodireddy@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 >>>>> dev@openvswitch.org >>>>> https://mail.openvswitch.org/mailman/listinfo/ovs-dev >>>> >>>> >>>>
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',
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. 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.maximets@samsung.com> Signed-off-by: Bhanuprakash Bodireddy <bhanuprakash.bodireddy@intel.com> --- lib/dpif-netdev.c | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-)