[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 10:51:48 UTC 2018


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.

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


More information about the dev mailing list