[ovs-dev] [PATCH] dpif-netdev: Allocate dp_netdev_pmd_thread struct by xzalloc_cacheline.
Ilya Maximets
i.maximets at samsung.com
Fri Dec 8 16:06:26 UTC 2017
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 at samsung.com>
>>>> Signed-off-by: Bhanuprakash Bodireddy
>>>> <bhanuprakash.bodireddy at 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 at openvswitch.org
>>>> https://mail.openvswitch.org/mailman/listinfo/ovs-dev
>>>
>>>
>>>
More information about the dev
mailing list