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;
+}
+
_______________________________________________
dev mailing list
[email protected]
https://mail.openvswitch.org/mailman/listinfo/ovs-dev
