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

Message ID 1511732875-8514-1-git-send-email-bhanuprakash.bodireddy@intel.com
State New
Delegated to: Ian Stokes
Headers show
Series
  • [ovs-dev] dpif-netdev: Allocate dp_netdev_pmd_thread struct by xzalloc_cacheline.
Related show

Commit Message

Bodireddy, Bhanuprakash Nov. 26, 2017, 9:47 p.m.
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(-)

Comments

Stokes, Ian Dec. 8, 2017, 1:45 p.m. | #1
> 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
Ilya Maximets Dec. 8, 2017, 1:53 p.m. | #2
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
> 
> 
>
Bodireddy, Bhanuprakash Dec. 8, 2017, 3:44 p.m. | #3
>

>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

>>

>>

>>
Ilya Maximets Dec. 8, 2017, 4:06 p.m. | #4
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
>>>
>>>
>>>
Bodireddy, Bhanuprakash Dec. 8, 2017, 4:23 p.m. | #5
>

>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

>>>>

>>>>

>>>>

Patch

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