[dpdk-dev] pthread_barrier_deadlock in -rc1
Maxime Coquelin
maxime.coquelin at redhat.com
Wed May 2 11:41:47 CEST 2018
On 05/02/2018 11:32 AM, Tan, Jianfeng wrote:
> Hi Maxime and Olivier,
>
> [...]
>>>> Below patch can fix another strange sigsegv issue in my VM. Please
>>>> check
>>>> if it works for you. I doubt it's use-after-free problem which could
>>>> lead to different issues in different env. Please have a try.
>>>>
>>>>
>>>> diff --git a/lib/librte_eal/common/eal_common_thread.c
>>>> b/lib/librte_eal/common/eal_common_thread.c
>>>> index de69452..d91b67d 100644
>>>> --- a/lib/librte_eal/common/eal_common_thread.c
>>>> +++ b/lib/librte_eal/common/eal_common_thread.c
>>>> @@ -205,6 +205,7 @@ rte_ctrl_thread_create(pthread_t *thread, const
>>>> char
>>>> *name,
>>>> goto fail;
>>>>
>>>> pthread_barrier_wait(¶ms->configured);
>>>> + pthread_barrier_destroy(¶ms->configured);
>>> Thanks Jianfeng, that fixes my issue.
>>> For correctness, I wonder whether we should check pthread_barrier_wait
>>> return, and only call destroy() if PTHREAD_BARRIER_SERIAL_THREAD?
>>> And so also do same the same thing in rte_thread_init().
>>>
>>> What do you think?
>>> Thanks,
>>> Maxime
>>
>> Thanks for the update. I also have a patch that replaces the barrier by
>> a lock which could also work, but if Jianfeng's one fixes the issue, I
>> think it is better.
>>
>> About the PTHREAD_BARRIER_SERIAL_THREAD, not sure it will change
>> something:
>>
>> Upon successful completion, the pthread_barrier_wait() function
>> shall return PTHREAD_BARRIER_SERIAL_THREAD for a single
>> (arbitrary) thread synchronized at the barrier and zero for each
>> of the other threads. Otherwise, an error number shall be
>> returned to indicate the error.
>>
>> I understand that it will ensure that only one barrier will return
>> PTHREAD_BARRIER_SERIAL_THREAD, but not necessarily the last one. So
>> if destroy() is called in the parent thread, it should be the same, no?
>>
>> By the way, there is also a small memory leak that was introduced by
>> the previous patch, maybe you can add the fix too:
>>
>> - if (ret != 0)
>> + if (ret != 0) {
>> + free(params);
>> return ret;
>> + }
>
> How about: the thread who gets PTHREAD_BARRIER_SERIAL_THREAD returned,
> is responsible for the destroy and free(params)?
I agree with your suggestion.
Thanks,
Maxime
> Thanks,
> Jianfeng
More information about the dev
mailing list