[ovs-dev] [PATCH] dpif-netdev: Allocate dp_netdev_pmd_thread struct by xzalloc_cacheline.

Bodireddy, Bhanuprakash bhanuprakash.bodireddy at intel.com
Fri Dec 8 15:44:55 UTC 2017


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