On Wed, 23 Feb 2011, Mathieu Desnoyers wrote:

* Nils Carlson ([email protected]) wrote:


On Wed, 23 Feb 2011, Mathieu Desnoyers wrote:

* Nils Carlson ([email protected]) wrote:
On Wed, 23 Feb 2011, Mathieu Desnoyers wrote:

* Nils Carlson ([email protected]) wrote:
Yep. fixed. merged.

+    /*
+     * FIXME: This could be prettier, we loop over the list twice and
+     * following good locking practice should lock around the loop
+     */
+    cds_list_for_each_entry_safe(trace, trace_tmp, &ltt_traces.head,
list) {
+        ltt_trace_destroy(trace->trace_name, 1);
+    }

So what' up with these missing locks ?


They are actually taken by each ltt_trace_destroy. Also, this function is
run as part of the ust_fork so locking in the child is a non-issue at
this point. Its mostly an aesthetic thing that we should be consistent in
dealing with locks.

Most of the ust_fork code should actually be protected by a mutex to
deal with the fact that we might have a concurrent libust thread running
in the parent at the exact point we do the fork, thus letting the child
in a state where locks are taken, and waits on the locks forever,
because the libust thread is not present in the child. The concurrent
libust thread in the parent should be kept quiescent while we do the
fork.


ust_fork is only called by the child after the fork. The function name is
a bit misleading.

I don't think this is handled at the moment, and can leave us with
various of the other locks in a "held" state when we run in the child.

As far as locking is concerned we're completely safe today, as we are
completely serialised. If we ever decide on multiple control threads or
some such we will probably have to recheck all the locking.

Let's look at:

ustfork.c:

Parent                           Child

fork()
 ust_before_fork()
   - disable signals
 plibc_func(); (real fork)
 ust_after_fork_parent()
   - restore signals
                                ust_after_fork_child()
                                  - ust_fork()
                                      ltt_unlock_traces(); <- bug (never locked 
?)
                                  - reenable signals

So if we have a libust listener thread running in the parent in parallel
with the fork(), we possibly leave many locks in "held" state when the
child starts, and we don't want that, right ? It looks like a can of
worms ready to be opened. Ideally, we should exclude the libust listener
thread by taking a mutex across the fork in ust_before_fork (after
disabling signals) and releasing it in both ust_after_fork_parent and
ust_after_fork_child.

I can have a look tomorrow. We don't have all that many locks though. Most of my focus has been on not causing deadlocks. I've sort of ducked the fork issue this far as their are some things to consider.

I think your solution of a cross-fork mutex is a good one. This would allow us to avoid the current pitfalls with memory allocation (currently if you fork before saving a reference to the allocated memory somewhere it will be lost resulting in a leak.)

Yepp. Think thats a good solution. Will think about it tonight. :-)

/Nils


Thoughts ?

Mathieu



/Nils


Thoughts ?

Mathieu


/Nils

Mathieu


    /* Clean up the listener socket and epoll, keeping the scoket
file */
    ustcomm_del_named_sock(listen_sock, 1);
--
1.7.1





_______________________________________________
ltt-dev mailing list
[email protected]
http://lists.casi.polymtl.ca/cgi-bin/mailman/listinfo/ltt-dev


--
Mathieu Desnoyers
Operating System Efficiency R&D Consultant
EfficiOS Inc.
http://www.efficios.com



--
Mathieu Desnoyers
Operating System Efficiency R&D Consultant
EfficiOS Inc.
http://www.efficios.com



--
Mathieu Desnoyers
Operating System Efficiency R&D Consultant
EfficiOS Inc.
http://www.efficios.com


_______________________________________________
ltt-dev mailing list
[email protected]
http://lists.casi.polymtl.ca/cgi-bin/mailman/listinfo/ltt-dev

Reply via email to