* [ltt-dev] [UST PATCH] libust: Fix multiple fd close during fork
@ 2011-02-23 12:40 Nils Carlson
2011-02-23 12:43 ` Nils Carlson
0 siblings, 1 reply; 10+ messages in thread
From: Nils Carlson @ 2011-02-23 12:40 UTC (permalink / raw)
Remove superfluous fd closes during fork and also destroy all
traces instead of just "auto".
Reported-by: Yannick Brosseau <yannick.brosseau at gmail.com>
Signed-off-by: Nils Carlson <nils.carlson at ericsson.com>
---
libust/tracectl.c | 17 ++++++++---------
1 files changed, 8 insertions(+), 9 deletions(-)
diff --git a/libust/tracectl.c b/libust/tracectl.c
index 1bd7229..1fc0118 100644
--- a/libust/tracectl.c
+++ b/libust/tracectl.c
@@ -1569,6 +1569,7 @@ static void ust_fork(void)
{
struct ust_buffer *buf, *buf_tmp;
struct ustcomm_sock *sock, *sock_tmp;
+ struct ust_trace *trace, *trace_tmp;
int result;
/* FIXME: technically, the locks could have been taken before the fork */
@@ -1589,18 +1590,16 @@ static void ust_fork(void)
/* Delete all blocked consumers */
cds_list_for_each_entry_safe(buf, buf_tmp, &open_buffers_list,
open_buffers_list) {
- result = close(buf->data_ready_fd_read);
- if (result == -1) {
- PERROR("close");
- }
- result = close(buf->data_ready_fd_write);
- if (result == -1) {
- PERROR("close");
- }
cds_list_del(&buf->open_buffers_list);
}
- ltt_trace_destroy("auto", 1);
+ /*
+ * 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);
+ }
/* Clean up the listener socket and epoll, keeping the scoket file */
ustcomm_del_named_sock(listen_sock, 1);
--
1.7.1
^ permalink raw reply [flat|nested] 10+ messages in thread
* [ltt-dev] [UST PATCH] libust: Fix multiple fd close during fork
2011-02-23 12:40 [ltt-dev] [UST PATCH] libust: Fix multiple fd close during fork Nils Carlson
@ 2011-02-23 12:43 ` Nils Carlson
2011-02-23 14:46 ` Yannick Brosseau
0 siblings, 1 reply; 10+ messages in thread
From: Nils Carlson @ 2011-02-23 12:43 UTC (permalink / raw)
This sort of fixes things in a wider sense as with the original patch we
might leave trailing fds open and a non "auto" trace would have been
neither destroyed nor have its fds closed...
Hope its ok with you yannick. Keep up the good work!
/Nils
On Wed, 23 Feb 2011, Nils Carlson wrote:
> Remove superfluous fd closes during fork and also destroy all
> traces instead of just "auto".
>
> Reported-by: Yannick Brosseau <yannick.brosseau at gmail.com>
> Signed-off-by: Nils Carlson <nils.carlson at ericsson.com>
> ---
> libust/tracectl.c | 17 ++++++++---------
> 1 files changed, 8 insertions(+), 9 deletions(-)
>
> diff --git a/libust/tracectl.c b/libust/tracectl.c
> index 1bd7229..1fc0118 100644
> --- a/libust/tracectl.c
> +++ b/libust/tracectl.c
> @@ -1569,6 +1569,7 @@ static void ust_fork(void)
> {
> struct ust_buffer *buf, *buf_tmp;
> struct ustcomm_sock *sock, *sock_tmp;
> + struct ust_trace *trace, *trace_tmp;
> int result;
>
> /* FIXME: technically, the locks could have been taken before the fork */
> @@ -1589,18 +1590,16 @@ static void ust_fork(void)
> /* Delete all blocked consumers */
> cds_list_for_each_entry_safe(buf, buf_tmp, &open_buffers_list,
> open_buffers_list) {
> - result = close(buf->data_ready_fd_read);
> - if (result == -1) {
> - PERROR("close");
> - }
> - result = close(buf->data_ready_fd_write);
> - if (result == -1) {
> - PERROR("close");
> - }
> cds_list_del(&buf->open_buffers_list);
> }
>
> - ltt_trace_destroy("auto", 1);
> + /*
> + * 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);
> + }
>
> /* Clean up the listener socket and epoll, keeping the scoket file */
> ustcomm_del_named_sock(listen_sock, 1);
> --
> 1.7.1
>
>
^ permalink raw reply [flat|nested] 10+ messages in thread
* [ltt-dev] [UST PATCH] libust: Fix multiple fd close during fork
2011-02-23 12:43 ` Nils Carlson
@ 2011-02-23 14:46 ` Yannick Brosseau
2011-02-23 15:54 ` Nils Carlson
0 siblings, 1 reply; 10+ messages in thread
From: Yannick Brosseau @ 2011-02-23 14:46 UTC (permalink / raw)
On 2011-02-23 07:43, Nils Carlson wrote:
>
> This sort of fixes things in a wider sense as with the original patch
> we might leave trailing fds open and a non "auto" trace would have
> been neither destroyed nor have its fds closed...
>
> Hope its ok with you yannick. Keep up the good work!
that's fine with me. But shouldn't we also do this kind of loop for all
the "auto" in the ust_fork function?
>
> /Nils
>
> On Wed, 23 Feb 2011, Nils Carlson wrote:
>
>> Remove superfluous fd closes during fork and also destroy all
>> traces instead of just "auto".
>>
>> Reported-by: Yannick Brosseau <yannick.brosseau at gmail.com>
>> Signed-off-by: Nils Carlson <nils.carlson at ericsson.com>
>> ---
>> libust/tracectl.c | 17 ++++++++---------
>> 1 files changed, 8 insertions(+), 9 deletions(-)
>>
>> diff --git a/libust/tracectl.c b/libust/tracectl.c
>> index 1bd7229..1fc0118 100644
>> --- a/libust/tracectl.c
>> +++ b/libust/tracectl.c
>> @@ -1569,6 +1569,7 @@ static void ust_fork(void)
>> {
>> struct ust_buffer *buf, *buf_tmp;
>> struct ustcomm_sock *sock, *sock_tmp;
>> + struct ust_trace *trace, *trace_tmp;
>> int result;
>>
>> /* FIXME: technically, the locks could have been taken before the
>> fork */
>> @@ -1589,18 +1590,16 @@ static void ust_fork(void)
>> /* Delete all blocked consumers */
>> cds_list_for_each_entry_safe(buf, buf_tmp, &open_buffers_list,
>> open_buffers_list) {
>> - result = close(buf->data_ready_fd_read);
>> - if (result == -1) {
>> - PERROR("close");
>> - }
>> - result = close(buf->data_ready_fd_write);
>> - if (result == -1) {
>> - PERROR("close");
>> - }
>> cds_list_del(&buf->open_buffers_list);
>> }
>>
>> - ltt_trace_destroy("auto", 1);
>> + /*
>> + * 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);
>> + }
>>
>> /* Clean up the listener socket and epoll, keeping the scoket
>> file */
>> ustcomm_del_named_sock(listen_sock, 1);
>> --
>> 1.7.1
>>
>>
^ permalink raw reply [flat|nested] 10+ messages in thread
* [ltt-dev] [UST PATCH] libust: Fix multiple fd close during fork
2011-02-23 15:54 ` Nils Carlson
@ 2011-02-23 15:40 ` Mathieu Desnoyers
[not found] ` <BLU0-SMTP169B3ABCBCF5E7D9DB528B96DB0@phx.gbl>
1 sibling, 0 replies; 10+ messages in thread
From: Mathieu Desnoyers @ 2011-02-23 15:40 UTC (permalink / raw)
* 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 ?
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
^ permalink raw reply [flat|nested] 10+ messages in thread
* [ltt-dev] [UST PATCH] libust: Fix multiple fd close during fork
2011-02-23 16:03 ` Nils Carlson
@ 2011-02-23 15:49 ` Mathieu Desnoyers
[not found] ` <BLU0-SMTP63BB495BA90023E1C54C0E96DB0@phx.gbl>
1 sibling, 0 replies; 10+ messages in thread
From: Mathieu Desnoyers @ 2011-02-23 15:49 UTC (permalink / raw)
* 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.
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.
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
^ permalink raw reply [flat|nested] 10+ messages in thread
* [ltt-dev] [UST PATCH] libust: Fix multiple fd close during fork
2011-02-23 14:46 ` Yannick Brosseau
@ 2011-02-23 15:54 ` Nils Carlson
2011-02-23 15:40 ` Mathieu Desnoyers
[not found] ` <BLU0-SMTP169B3ABCBCF5E7D9DB528B96DB0@phx.gbl>
0 siblings, 2 replies; 10+ messages in thread
From: Nils Carlson @ 2011-02-23 15:54 UTC (permalink / raw)
Yep. fixed. merged.
/Nils
On Wed, 23 Feb 2011, Yannick Brosseau wrote:
> On 2011-02-23 07:43, Nils Carlson wrote:
>>
>> This sort of fixes things in a wider sense as with the original patch
>> we might leave trailing fds open and a non "auto" trace would have
>> been neither destroyed nor have its fds closed...
>>
>> Hope its ok with you yannick. Keep up the good work!
>
> that's fine with me. But shouldn't we also do this kind of loop for all
> the "auto" in the ust_fork function?
>
>>
>> /Nils
>>
>> On Wed, 23 Feb 2011, Nils Carlson wrote:
>>
>>> Remove superfluous fd closes during fork and also destroy all
>>> traces instead of just "auto".
>>>
>>> Reported-by: Yannick Brosseau <yannick.brosseau at gmail.com>
>>> Signed-off-by: Nils Carlson <nils.carlson at ericsson.com>
>>> ---
>>> libust/tracectl.c | 17 ++++++++---------
>>> 1 files changed, 8 insertions(+), 9 deletions(-)
>>>
>>> diff --git a/libust/tracectl.c b/libust/tracectl.c
>>> index 1bd7229..1fc0118 100644
>>> --- a/libust/tracectl.c
>>> +++ b/libust/tracectl.c
>>> @@ -1569,6 +1569,7 @@ static void ust_fork(void)
>>> {
>>> struct ust_buffer *buf, *buf_tmp;
>>> struct ustcomm_sock *sock, *sock_tmp;
>>> + struct ust_trace *trace, *trace_tmp;
>>> int result;
>>>
>>> /* FIXME: technically, the locks could have been taken before the
>>> fork */
>>> @@ -1589,18 +1590,16 @@ static void ust_fork(void)
>>> /* Delete all blocked consumers */
>>> cds_list_for_each_entry_safe(buf, buf_tmp, &open_buffers_list,
>>> open_buffers_list) {
>>> - result = close(buf->data_ready_fd_read);
>>> - if (result == -1) {
>>> - PERROR("close");
>>> - }
>>> - result = close(buf->data_ready_fd_write);
>>> - if (result == -1) {
>>> - PERROR("close");
>>> - }
>>> cds_list_del(&buf->open_buffers_list);
>>> }
>>>
>>> - ltt_trace_destroy("auto", 1);
>>> + /*
>>> + * 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);
>>> + }
>>>
>>> /* Clean up the listener socket and epoll, keeping the scoket
>>> file */
>>> ustcomm_del_named_sock(listen_sock, 1);
>>> --
>>> 1.7.1
>>>
>>>
>
>
^ permalink raw reply [flat|nested] 10+ messages in thread
* [ltt-dev] [UST PATCH] libust: Fix multiple fd close during fork
[not found] ` <BLU0-SMTP169B3ABCBCF5E7D9DB528B96DB0@phx.gbl>
@ 2011-02-23 16:03 ` Nils Carlson
2011-02-23 15:49 ` Mathieu Desnoyers
[not found] ` <BLU0-SMTP63BB495BA90023E1C54C0E96DB0@phx.gbl>
0 siblings, 2 replies; 10+ messages in thread
From: Nils Carlson @ 2011-02-23 16:03 UTC (permalink / raw)
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.
/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
>
^ permalink raw reply [flat|nested] 10+ messages in thread
* [ltt-dev] [UST PATCH] libust: Fix multiple fd close during fork
[not found] ` <BLU0-SMTP63BB495BA90023E1C54C0E96DB0@phx.gbl>
@ 2011-02-23 16:14 ` Nils Carlson
2011-02-23 16:16 ` Mathieu Desnoyers
[not found] ` <BLU0-SMTP763331DEBE2782CAA18D5F96DB0@phx.gbl>
0 siblings, 2 replies; 10+ messages in thread
From: Nils Carlson @ 2011-02-23 16:14 UTC (permalink / raw)
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.
/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
>
^ permalink raw reply [flat|nested] 10+ messages in thread
* [ltt-dev] [UST PATCH] libust: Fix multiple fd close during fork
2011-02-23 16:14 ` Nils Carlson
@ 2011-02-23 16:16 ` Mathieu Desnoyers
[not found] ` <BLU0-SMTP763331DEBE2782CAA18D5F96DB0@phx.gbl>
1 sibling, 0 replies; 10+ messages in thread
From: Mathieu Desnoyers @ 2011-02-23 16:16 UTC (permalink / raw)
* 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
^ permalink raw reply [flat|nested] 10+ messages in thread
* [ltt-dev] [UST PATCH] libust: Fix multiple fd close during fork
[not found] ` <BLU0-SMTP763331DEBE2782CAA18D5F96DB0@phx.gbl>
@ 2011-02-23 18:54 ` Nils Carlson
0 siblings, 0 replies; 10+ messages in thread
From: Nils Carlson @ 2011-02-23 18:54 UTC (permalink / raw)
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:
>>>> 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.
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
>>>>>> 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
>
^ permalink raw reply [flat|nested] 10+ messages in thread
end of thread, other threads:[~2011-02-23 18:54 UTC | newest]
Thread overview: 10+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2011-02-23 12:40 [ltt-dev] [UST PATCH] libust: Fix multiple fd close during fork Nils Carlson
2011-02-23 12:43 ` Nils Carlson
2011-02-23 14:46 ` Yannick Brosseau
2011-02-23 15:54 ` Nils Carlson
2011-02-23 15:40 ` Mathieu Desnoyers
[not found] ` <BLU0-SMTP169B3ABCBCF5E7D9DB528B96DB0@phx.gbl>
2011-02-23 16:03 ` Nils Carlson
2011-02-23 15:49 ` Mathieu Desnoyers
[not found] ` <BLU0-SMTP63BB495BA90023E1C54C0E96DB0@phx.gbl>
2011-02-23 16:14 ` Nils Carlson
2011-02-23 16:16 ` Mathieu Desnoyers
[not found] ` <BLU0-SMTP763331DEBE2782CAA18D5F96DB0@phx.gbl>
2011-02-23 18:54 ` Nils Carlson
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox