Re: [dpdk-dev] [dpdk-stable] [PATCH] vdpa/mlx5: fix configuration mutex cleanup

[Maxime Coquelin <maxime.coquelin@redhat.com>]

On 1/26/21 11:45 AM, Matan Azrad wrote:
> 
> 
> From: Maxime Coquelin
>>> From: Maxime Coquelin
>>>> On 1/14/21 4:23 PM, Matan Azrad wrote:
>>>>>
>>>>>
>>>>> From: Maxime Coquelin
>>>>>> On 1/14/21 2:09 PM, Matan Azrad wrote:
>>>>>>>
>>>>>>>
>>>>>>> From: Maxime Coquelin
>>>>>>>> Hi Matan,
>>>>>>>>
>>>>>>>> On 1/14/21 12:49 PM, Matan Azrad wrote:
>>>>>>>>> Hi Maxime and David
>>>>>>>>>
>>>>>>>>> Thank you for Review.
>>>>>>>>>
>>>>>>>>> From: David Marchand
>>>>>>>>>> On Fri, Jan 8, 2021 at 9:48 AM David Marchand
>>>>>>>>>> <david.marchand@redhat.com> wrote:
>>>>>>>>>>>> I wonder if it would be possible and cleaner to disable
>>>>>>>>>>>> cancellation on the thread while the mutex is held?
>>>>>>>>>
>>>>>>>>> Yes, we can cause thread to return by some global variable sync.
>>>>>>>>> It is the same logic.
>>>>>>>>
>>>>>>>> No, that was not my suggestion. My suggestion is to block the
>>>>>>>> thread cancellation while in the critical section, using
>>>> pthread_setcancelstate().
>>>>>>>
>>>>>>> Yes, Generally it is better to let the thread control his
>>>>>>> cancellation, either
>>>>>> cancel itself or enabling\disabling cancellations.
>>>>>>>
>>>>>>> I don't see a reason to wait for the thread in current logic - the
>>>>>>> critical section
>>>>>> is not important to be completed here.
>>>>>>
>>>>>> The reason I see is there are quite a few things done in this
>>>>>> critical section. And if tomorrow someone add new things in it, he
>>>>>> may not know the thread can be cancelled at any time, which could
>>>>>> cause
>>>> hard to debug issues.
>>>>>
>>>>> As I said, here it is not needed, this thread designed just to cause
>>>>> guest
>>>> notifications.
>>>>>
>>>>> The optional future developer mistake can be done also outside the
>>>>> critical
>>>> section in in any other place - we cannot protect it.
>>>>>
>>>>> The design choice is to close the thread fast.
>>>>
>>>> But why is it so urgent that it cannot been stopped cleanly?
>>>> I don't think it would add seconds delay by doing it in a clean way.
>>>
>>> We have system calls there per queue.
>>> No need this optional delay just because of mutex cleaning.
>>
>> OK, up to you...
>>
>> And what about the timer lock?
> 
> Existing code initiates it before reusing...

Ok, so why not applying same logic for both mutexes?

> Thanks.
> 
>>
>>>
>>>
>>>> Thanks,
>>>> Maxime
>>>>
>>>>>>> We just want to close the thread and to clean the mutex.
>>>>>>>
>>>>>>>>>>> +1
>>>>>>>>>>
>>>>>>>>>> IEEE Std 1003.1-2001/Cor 2-2004, item XBD/TC2/D6/26 is applied,
>>>>>>>>>> adding pthread_t to the list of types that are not required to
>>>>>>>>>> be arithmetic types, thus allowing pthread_t to be defined as a
>> structure.
>>>>>>>>>>
>>>>>>>>>> It would be better to leave pthread_t alone and not interpret it:
>>>>>>>>>>
>>>>>>>>>> if (priv->timer_tid) {
>>>>>>>>>>     pthread_cancel(priv->timer_tid);
>>>>>>>>>>     pthread_join(priv->timer_tid, &status); }
>>>>>>>>>> priv->timer_tid = 0;
>>>>>>>>>
>>>>>>>>>
>>>>>>>>> I'm not sure why you think it is better in this specific case.
>>>>>>>>> The cancellation will close the thread in faster way, no need to
>>>>>>>>> wait for the
>>>>>>>> thread to close itself.
>>>>>>>>>
>>>>>>>>>
>>>>>>>>>>
>>>>>>>>>> --
>>>>>>>>>> David Marchand
>>>>>>>>>
>>>>>>>
>>>>>
>>>
>