[ovs-dev] [PATCH v2 1/1] ovs-thread: Fix thread id for threads not started with ovs_thread_create()

Eelco Chaudron echaudro at redhat.com
Fri Jun 1 11:55:26 UTC 2018


On 01/06/18 13:48, Ilya Maximets wrote:
> On 01.06.2018 14:07, Eelco Chaudron wrote:
>> On 01/06/18 12:51, Ilya Maximets wrote:
>>> On 29.05.2018 11:23, Eelco Chaudron wrote:
>>>> On 05/28/2018 03:42 PM, Ilya Maximets wrote:
>>>>> On 28.05.2018 14:06, Eelco Chaudron wrote:
>>>> <SNIP>
>>>>>> +unsigned int
>>>>>> +ovsthread_id_init(void)
>>>>>> +{
>>>>>> +    static atomic_count next_id = ATOMIC_COUNT_INIT(0);
>>>>>> +
>>>>>> +    unsigned int id = *ovsthread_id_get();
>>>>>> +
>>>>>> +    if (id == OVSTHREAD_ID_UNSET) {
>>>>> I'm not sure, if we need to check the id here. I think, 'init' function
>>>>> should not be called twice. Also, you're checking the id before calling.
>>>> I don't think checking before calling should be an issue, it will return the default.
>>> I'm just a bit uncomfortable with checking inside ovsthread_id_init() because:
>>> 1. We always check before the call, except for trivial cases.
>>> 2. 'init' function should never be called twice for the same thread because
>>>      it's logically incorrect.
>>>
>>>>> How about just adding the assert?
>>>> I'm not a big fan of assert, especially in recoverable situations. This way calling the function twice will not hurt. Also, this function should never be called by the application code directly.
>>>>
>>>> But if assert is a common practice in OVS, I can go ahead and change it.
>>> As I wrote above, the call to 'ovsthread_id_init()' from the same thread twice
>>> is logically wrong. If this happens, it's likely an error on the caller side
>>> and it should be investigated. That's why I think it'll be good to assert here.
>> So you do want to keep the check, and assert like this?
>>
>> +unsigned int
>> +ovsthread_id_init(void)
>> +{
>> +    static atomic_count next_id = ATOMIC_COUNT_INIT(0);
>> +
>> +    unsigned int id = *ovsthread_id_get();
>> +    assert(id == OVSTHREAD_ID_UNSET)
>> +    id = atomic_count_inc(&next_id);
>> +    *ovsthread_id_get() = id;
>> +
>> +    return id;
>> +}
>> +
>>
>> Or remove it all together:
>>
>> +unsigned int
>> +ovsthread_id_init(void)
>> +{
>> +    static atomic_count next_id = ATOMIC_COUNT_INIT(0);
>> +
>> +    unsigned int id = atomic_count_inc(&next_id);
>> +
>> +    *ovsthread_id_get() = id;
>> +
>> +    return id;
>> +}
>> +
>>
>> My preference would be the first one, as this should only be called once on thread initialization, so the extra check/assert should not hurt.
> Sure, the first one. Additionally, I think that you could remove local 'id'
> variable by checking ovsthread_id_get() directly like this:
>
> unsigned int
> ovsthread_id_init(void)
> {
>      static atomic_count next_id = ATOMIC_COUNT_INIT(0);
>
>      assert(*ovsthread_id_get() == OVSTHREAD_ID_UNSET);
>      return *ovsthread_id_get() = atomic_count_inc(&next_id);
> }
Makes sense, I'll send out a v3 shortly...
>>>>>> +        id = atomic_count_inc(&next_id);
>>>>>> +        *ovsthread_id_get() = id;
>>>>>> +    }
>>>>>> +
>>>>>> +    return id;
>>>>>> +}
>>>>>> +
>>>>>>
>>
>>
>>



More information about the dev mailing list