On May 23, 2014, at 10:11 AM, Ben Pfaff <[email protected]> wrote:
> On Thu, May 22, 2014 at 05:37:40PM -0700, Jarno Rajahalme wrote:
>> lib/ovs-rcu.h had some of the comments duplicated.
>>
>> Add ovsrcu_init() that can be used like ovsrcu_set() when the RCU
>> protected pointer is not yet visible any readers.
>>
>> Use OVS_CONSTRUCTOR to initialize the ovs-rcu module.
>>
>> Signed-off-by: Jarno Rajahalme <[email protected]>
>
> I get nervous to start to see modules using OVS_CONSTRUCTOR to do fairly
> complex work. Because there is no dependency system for constructors,
> the order in which they are invoked is unpredictable and can vary from
> one compiler or system to another. If we are not perfectly careful to
> make sure that there are no actual dependencies among them, then that
> can lead to surprising crashes at startup time.
Yeah, I have struggled with nasty bugs due to this issue in the past, but
already forgot about the pain, apparently :-)
in general, I like the idea of getting rid of checking for initialization from
all the possible entry points.
We already have a “module” system for VLOG, maybe we could generalize it to
include module initialization with some kind of dependency/priority system?
However, I totally see your point, we are calling non-trivial functions here,
so I am happy to roll this back for now. However, I’d like to use the name
“ovsrcu_init” for the new purpose, as it rhymes with “atomic_init”, which has
corresponding semantics. You fine with using ovsrcu_init_module() instead:
diff --git a/lib/ovs-rcu.c b/lib/ovs-rcu.c
index 7aed6db..62fe614 100644
--- a/lib/ovs-rcu.c
+++ b/lib/ovs-rcu.c
@@ -55,7 +55,7 @@ static struct ovs_mutex ovsrcu_threads_mutex;
static struct guarded_list flushed_cbsets;
static struct seq *flushed_cbsets_seq;
-static void ovsrcu_init(void);
+static void ovsrcu_init_module(void);
static void ovsrcu_flush_cbset(struct ovsrcu_perthread *);
static void ovsrcu_unregister__(struct ovsrcu_perthread *);
static bool ovsrcu_call_postponed(void);
@@ -67,7 +67,7 @@ ovsrcu_perthread_get(void)
{
struct ovsrcu_perthread *perthread;
- ovsrcu_init();
+ ovsrcu_init_module();
perthread = pthread_getspecific(perthread_key);
if (!perthread) {
@@ -121,7 +121,7 @@ ovsrcu_quiesce_start(void)
{
struct ovsrcu_perthread *perthread;
- ovsrcu_init();
+ ovsrcu_init_module();
perthread = pthread_getspecific(perthread_key);
if (perthread) {
pthread_setspecific(perthread_key, NULL);
@@ -136,7 +136,7 @@ ovsrcu_quiesce_start(void)
void
ovsrcu_quiesce(void)
{
- ovsrcu_init();
+ ovsrcu_init_module();
ovsrcu_perthread_get()->seqno = seq_read(global_seqno);
seq_change(global_seqno);
@@ -146,7 +146,7 @@ ovsrcu_quiesce(void)
bool
ovsrcu_is_quiescent(void)
{
- ovsrcu_init();
+ ovsrcu_init_module();
return pthread_getspecific(perthread_key) == NULL;
}
@@ -308,7 +308,7 @@ ovsrcu_thread_exit_cb(void *perthread)
}
static void
-ovsrcu_init(void)
+ovsrcu_init_module(void)
{
static struct ovsthread_once once = OVSTHREAD_ONCE_INITIALIZER;
if (ovsthread_once_start(&once)) {
>
> I'm happy with the comment update.
>
:-)
Jarno
> Thanks,
>
> Ben.
_______________________________________________
dev mailing list
[email protected]
http://openvswitch.org/mailman/listinfo/dev
