[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:07:41 UTC 2018


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.
>>
>>>> +        id = atomic_count_inc(&next_id);
>>>> +        *ovsthread_id_get() = id;
>>>> +    }
>>>> +
>>>> +    return id;
>>>> +}
>>>> +
>>>>



More information about the dev mailing list