[ltt-dev] [UST PATCH] libust: Fix multiple fd close during fork

[Mathieu Desnoyers]

* Nils Carlson (nils.carlson at ericsson.com) wrote:
>
>
> On Wed, 23 Feb 2011, Mathieu Desnoyers wrote:
>
>> * Nils Carlson (nils.carlson at ericsson.com) wrote:
>>> On Wed, 23 Feb 2011, Mathieu Desnoyers wrote:
>>>
>>>> * Nils Carlson (nils.carlson at ericsson.com) 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.

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
>>>>> ltt-dev at lists.casi.polymtl.ca
>>>>> 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