>
>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.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
