From mboxrd@z Thu Jan 1 00:00:00 1970 From: compudj@krystal.dyndns.org (Mathieu Desnoyers) Date: Wed, 23 Feb 2011 11:16:49 -0500 Subject: [ltt-dev] [UST PATCH] libust: Fix multiple fd close during fork In-Reply-To: References: <1298464816-19832-1-git-send-email-nils.carlson@ericsson.com> <4D651DD0.8070805@gmail.com> Message-ID: * 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, <t_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