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

Ilya Maximets i.maximets at samsung.com
Fri Jun 1 11:48:29 UTC 2018


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);
}

>>>
>>>>> +        id = atomic_count_inc(&next_id);
>>>>> +        *ovsthread_id_get() = id;
>>>>> +    }
>>>>> +
>>>>> +    return id;
>>>>> +}
>>>>> +
>>>>>
> 
> 
> 
> 


More information about the dev mailing list