[ltt-dev] [PATCH] Fix initial state bug

[ltt-dev] [PATCH] Fix initial state bug

[francis.giraldeau]

From: Francis Giraldeau <francis.giraldeau@usherbrooke.ca>

This patch fixes a bug with the initial state in the control flow viewer. When
a process was started before starting the trace, in some situation all the
states of this process was wrong, because of a unknown initial state.

The patch adds string tables that matches the enums sequence in lttng-modules,
to convert enum value in the state dump to quark.

Update :
    * more specific macro name
    * move static string tables to state.c
---
 lttv/lttv/state.c |   55 +++++++++++++++++++++++++++++++++++++++-------------
 1 files changed, 41 insertions(+), 14 deletions(-)

diff --git a/lttv/lttv/state.c b/lttv/lttv/state.c
index bd3f64d..32d68f2 100644
--- a/lttv/lttv/state.c
+++ b/lttv/lttv/state.c
@@ -46,6 +46,43 @@
 
 #define PREALLOCATED_EXECUTION_STACK 10
 
+/* get a given quark from lttng module enum value */
+#define ltt_enum_quark(e, f, names) (g_quark_from_string(names[ltt_event_get_unsigned(e, f)]))
+
+/*
+ * Quark strings that matches lttng module enums found in statedump
+ * Matching enums are from lttng-modules/ltt-statedump.c
+ */
+static const char *const lttng_thread_type_names[] = {
+        "USER_THREAD",     // LTTNG_USER_THREAD
+        "KERNEL_THREAD"    // LTTNG_KERNEL_THREAD
+};
+
+static const char *const lttng_execution_mode_names[] = {
+        "USER_MODE",       // LTTNG_USER_MODE
+        "SYSCALL",         // LTTNG_SYSCALL
+        "TRAP",            // LTTNG_TRAP
+        "IRQ",             // LTTNG_IRQ
+        "SOFTIRQ",         // LTTNG_SOFTIRQ
+        "UNKNOWN"          // LTTNG_MODE_UNKNOWN
+};
+
+static const char *const lttng_execution_submode_names[] = {
+        "UNKNOWN",         // LTTNG_NONE
+        "NONE"             // LTTNG_UNKNOWN
+};
+
+static const char *const lttng_process_status_names[] = {
+        "",                 // LTTNG_UNNAMED
+        "WAIT_FORK",        // LTTNG_WAIT_FORK
+        "WAIT_CPU",         // LTTNG_WAIT_CPU
+        "EXIT",             // LTTNG_EXIT
+        "ZOMBIE",           // LTTNG_ZOMBIE
+        "WAIT",             // LTTNG_WAIT
+        "RUN",              // LTTNG_RUN
+        "DEAD"              // LTTNG_DEAD
+};
+
 /* Channel Quarks */
 
 GQuark
@@ -3440,21 +3477,19 @@ static gboolean enum_process_state(void *hook_data, void *call_data)
 
 	/* type */
 	f = lttv_trace_get_hook_field(th, 3);
-	type = ltt_enum_string_get(f, ltt_event_get_unsigned(e, f));
-
-	//FIXME: type is rarely used, enum must match possible types.
+	type = ltt_enum_quark(e, f, lttng_thread_type_names);
 
 	/* mode */
 	f = lttv_trace_get_hook_field(th, 4);
-	mode = ltt_enum_string_get(f,ltt_event_get_unsigned(e, f));
+	mode = ltt_enum_quark(e, f, lttng_execution_mode_names);
 
 	/* submode */
 	f = lttv_trace_get_hook_field(th, 5);
-	submode = ltt_enum_string_get(f, ltt_event_get_unsigned(e, f));
+	submode = ltt_enum_quark(e, f, lttng_execution_submode_names);
 
 	/* status */
 	f = lttv_trace_get_hook_field(th, 6);
-	status = ltt_enum_string_get(f, ltt_event_get_unsigned(e, f));
+	status = ltt_enum_quark(e, f, lttng_process_status_names);
 
 	/* TGID */
 	f = lttv_trace_get_hook_field(th, 7);
@@ -3503,11 +3538,9 @@ static gboolean enum_process_state(void *hook_data, void *call_data)
 				es->t = LTTV_STATE_MODE_UNKNOWN;
 				es->s = LTTV_STATE_UNNAMED;
 				es->n = LTTV_STATE_SUBMODE_UNKNOWN;
-#if 0
 				es->t = LTTV_STATE_SYSCALL;
 				es->s = status;
 				es->n = submode;
-#endif //0
 			} else {
 				/* User space process :
 				 * bottom : user mode
@@ -3528,13 +3561,10 @@ static gboolean enum_process_state(void *hook_data, void *call_data)
 				es->t = LTTV_STATE_MODE_UNKNOWN;
 				es->s = LTTV_STATE_UNNAMED;
 				es->n = LTTV_STATE_SUBMODE_UNKNOWN;
-	#if 0
 				es->t = LTTV_STATE_USER_MODE;
 				es->s = status;
 				es->n = submode;
-	#endif //0
 			}
-	#if 0
 			/* UNKNOWN STATE */
 			{
 				es = process->state = &g_array_index(process->execution_stack,
@@ -3543,7 +3573,6 @@ static gboolean enum_process_state(void *hook_data, void *call_data)
 				es->s = LTTV_STATE_UNNAMED;
 				es->n = LTTV_STATE_SUBMODE_UNKNOWN;
 			}
-	#endif //0
 		} else {
 			/* The process has already been created :
 			 * Probably was forked while dumping the process state or
@@ -3554,14 +3583,12 @@ static gboolean enum_process_state(void *hook_data, void *call_data)
 			process->name = g_quark_from_string(command);
 			process->type = type;
 			es = &g_array_index(process->execution_stack, LttvExecutionState, 0);
-#if 0
 			if(es->t == LTTV_STATE_MODE_UNKNOWN) {
 				if(type == LTTV_STATE_KERNEL_THREAD)
 					es->t = LTTV_STATE_SYSCALL;
 				else
 					es->t = LTTV_STATE_USER_MODE;
 			}
-#endif //0
 			/* Don't mess around with the stack, it will eventually become
 			 * ok after the end of state dump. */
 		}
-- 
1.7.1

[ltt-dev] [PATCH] Fix initial state bug

[Mathieu Desnoyers]

* francis.giraldeau at usherbrooke.ca (francis.giraldeau at usherbrooke.ca) wrote:
> From: Francis Giraldeau <francis.giraldeau@usherbrooke.ca>
> 
> This patch fixes a bug with the initial state in the control flow viewer. When
> a process was started before starting the trace, in some situation all the
> states of this process was wrong, because of a unknown initial state.
> 
> The patch adds string tables that matches the enums sequence in lttng-modules,
> to convert enum value in the state dump to quark.
> 
> Update :
>     * more specific macro name
>     * move static string tables to state.c
> ---
>  lttv/lttv/state.c |   55 +++++++++++++++++++++++++++++++++++++++-------------
>  1 files changed, 41 insertions(+), 14 deletions(-)
> 
> diff --git a/lttv/lttv/state.c b/lttv/lttv/state.c
> index bd3f64d..32d68f2 100644
> --- a/lttv/lttv/state.c
> +++ b/lttv/lttv/state.c
> @@ -46,6 +46,43 @@
>  
>  #define PREALLOCATED_EXECUTION_STACK 10
>  
> +/* get a given quark from lttng module enum value */
> +#define ltt_enum_quark(e, f, names) (g_quark_from_string(names[ltt_event_get_unsigned(e, f)]))

Please use a static inline rather than a preprocessor macro for this. It
comes with type checking and removes odd macro corner-cases.

> +
> +/*
> + * Quark strings that matches lttng module enums found in statedump
> + * Matching enums are from lttng-modules/ltt-statedump.c
> + */
> +static const char *const lttng_thread_type_names[] = {
> +        "USER_THREAD",     // LTTNG_USER_THREAD
> +        "KERNEL_THREAD"    // LTTNG_KERNEL_THREAD
> +};
> +
> +static const char *const lttng_execution_mode_names[] = {
> +        "USER_MODE",       // LTTNG_USER_MODE
> +        "SYSCALL",         // LTTNG_SYSCALL
> +        "TRAP",            // LTTNG_TRAP
> +        "IRQ",             // LTTNG_IRQ
> +        "SOFTIRQ",         // LTTNG_SOFTIRQ
> +        "UNKNOWN"          // LTTNG_MODE_UNKNOWN
> +};
> +
> +static const char *const lttng_execution_submode_names[] = {
> +        "UNKNOWN",         // LTTNG_NONE
> +        "NONE"             // LTTNG_UNKNOWN
> +};
> +
> +static const char *const lttng_process_status_names[] = {
> +        "",                 // LTTNG_UNNAMED
> +        "WAIT_FORK",        // LTTNG_WAIT_FORK
> +        "WAIT_CPU",         // LTTNG_WAIT_CPU
> +        "EXIT",             // LTTNG_EXIT
> +        "ZOMBIE",           // LTTNG_ZOMBIE
> +        "WAIT",             // LTTNG_WAIT
> +        "RUN",              // LTTNG_RUN
> +        "DEAD"              // LTTNG_DEAD
> +};
> +
>  /* Channel Quarks */
>  
>  GQuark
> @@ -3440,21 +3477,19 @@ static gboolean enum_process_state(void *hook_data, void *call_data)
>  
>  	/* type */
>  	f = lttv_trace_get_hook_field(th, 3);
> -	type = ltt_enum_string_get(f, ltt_event_get_unsigned(e, f));
> -
> -	//FIXME: type is rarely used, enum must match possible types.
> +	type = ltt_enum_quark(e, f, lttng_thread_type_names);
>  
>  	/* mode */
>  	f = lttv_trace_get_hook_field(th, 4);
> -	mode = ltt_enum_string_get(f,ltt_event_get_unsigned(e, f));
> +	mode = ltt_enum_quark(e, f, lttng_execution_mode_names);
>  
>  	/* submode */
>  	f = lttv_trace_get_hook_field(th, 5);
> -	submode = ltt_enum_string_get(f, ltt_event_get_unsigned(e, f));
> +	submode = ltt_enum_quark(e, f, lttng_execution_submode_names);
>  
>  	/* status */
>  	f = lttv_trace_get_hook_field(th, 6);
> -	status = ltt_enum_string_get(f, ltt_event_get_unsigned(e, f));
> +	status = ltt_enum_quark(e, f, lttng_process_status_names);
>  
>  	/* TGID */
>  	f = lttv_trace_get_hook_field(th, 7);
> @@ -3503,11 +3538,9 @@ static gboolean enum_process_state(void *hook_data, void *call_data)
>  				es->t = LTTV_STATE_MODE_UNKNOWN;
>  				es->s = LTTV_STATE_UNNAMED;
>  				es->n = LTTV_STATE_SUBMODE_UNKNOWN;
> -#if 0
>  				es->t = LTTV_STATE_SYSCALL;
>  				es->s = status;
>  				es->n = submode;
> -#endif //0

Hrm ? you're leaving the unknown and then changing the state ? What for ?

How do you know the thread is not in interrupt or trap ? (see comment
above)

>  			} else {
>  				/* User space process :
>  				 * bottom : user mode
> @@ -3528,13 +3561,10 @@ static gboolean enum_process_state(void *hook_data, void *call_data)
>  				es->t = LTTV_STATE_MODE_UNKNOWN;
>  				es->s = LTTV_STATE_UNNAMED;
>  				es->n = LTTV_STATE_SUBMODE_UNKNOWN;
> -	#if 0
>  				es->t = LTTV_STATE_USER_MODE;
>  				es->s = status;
>  				es->n = submode;
> -	#endif //0

Same here (about double-change).

Also, how would a thread, running in user-mode, and interrupted, be
listed by the state dump ? I think marking it as "user mode" is wrong
here. We don't know this for sure.

Julien looked at these things in great length lately, you might want to
discuss with him.

Thanks,

Mathieu

>  			}
> -	#if 0
>  			/* UNKNOWN STATE */
>  			{
>  				es = process->state = &g_array_index(process->execution_stack,
> @@ -3543,7 +3573,6 @@ static gboolean enum_process_state(void *hook_data, void *call_data)
>  				es->s = LTTV_STATE_UNNAMED;
>  				es->n = LTTV_STATE_SUBMODE_UNKNOWN;
>  			}
> -	#endif //0
>  		} else {
>  			/* The process has already been created :
>  			 * Probably was forked while dumping the process state or
> @@ -3554,14 +3583,12 @@ static gboolean enum_process_state(void *hook_data, void *call_data)
>  			process->name = g_quark_from_string(command);
>  			process->type = type;
>  			es = &g_array_index(process->execution_stack, LttvExecutionState, 0);
> -#if 0
>  			if(es->t == LTTV_STATE_MODE_UNKNOWN) {
>  				if(type == LTTV_STATE_KERNEL_THREAD)
>  					es->t = LTTV_STATE_SYSCALL;
>  				else
>  					es->t = LTTV_STATE_USER_MODE;
>  			}
> -#endif //0
>  			/* Don't mess around with the stack, it will eventually become
>  			 * ok after the end of state dump. */
>  		}
> -- 
> 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

[ltt-dev] [PATCH] Fix initial state bug

[Francis Giraldeau]

Mathieu Desnoyers <compudj at krystal.dyndns.org> a ?crit?:
>> +/* get a given quark from lttng module enum value */
>> +#define ltt_enum_quark(e, f, names)  
>> (g_quark_from_string(names[ltt_event_get_unsigned(e, f)]))
>
> Please use a static inline rather than a preprocessor macro for this. It
> comes with type checking and removes odd macro corner-cases.

Ok, done.

>> @@ -3503,11 +3538,9 @@ static gboolean enum_process_state(void  
>> *hook_data, void *call_data)
>>  				es->t = LTTV_STATE_MODE_UNKNOWN;
>>  				es->s = LTTV_STATE_UNNAMED;
>>  				es->n = LTTV_STATE_SUBMODE_UNKNOWN;
>> -#if 0
>>  				es->t = LTTV_STATE_SYSCALL;
>>  				es->s = status;
>>  				es->n = submode;
>> -#endif //0
>
> Hrm ? you're leaving the unknown and then changing the state ? What for ?
>
> How do you know the thread is not in interrupt or trap ? (see comment
> above)

Double state change is obviously wrong, my mistake. I removed those  
changes. As I understand, we don't know for sure the initial state  
from the state dump. The actual initial process state is known at the  
first occurrence of an event in the trace, isn't?

Francis

[ltt-dev] [PATCH] Fix initial state bug

[Mathieu Desnoyers]

* Francis Giraldeau (Francis.Giraldeau at USherbrooke.ca) wrote:
> Mathieu Desnoyers <compudj at krystal.dyndns.org> a ?crit?:
>>> +/* get a given quark from lttng module enum value */
>>> +#define ltt_enum_quark(e, f, names)  
>>> (g_quark_from_string(names[ltt_event_get_unsigned(e, f)]))
>>
>> Please use a static inline rather than a preprocessor macro for this. It
>> comes with type checking and removes odd macro corner-cases.
>
> Ok, done.
>
>>> @@ -3503,11 +3538,9 @@ static gboolean enum_process_state(void  
>>> *hook_data, void *call_data)
>>>  				es->t = LTTV_STATE_MODE_UNKNOWN;
>>>  				es->s = LTTV_STATE_UNNAMED;
>>>  				es->n = LTTV_STATE_SUBMODE_UNKNOWN;
>>> -#if 0
>>>  				es->t = LTTV_STATE_SYSCALL;
>>>  				es->s = status;
>>>  				es->n = submode;
>>> -#endif //0
>>
>> Hrm ? you're leaving the unknown and then changing the state ? What for ?
>>
>> How do you know the thread is not in interrupt or trap ? (see comment
>> above)
>
> Double state change is obviously wrong, my mistake. I removed those  
> changes. As I understand, we don't know for sure the initial state from 
> the state dump. The actual initial process state is known at the first 
> occurrence of an event in the trace, isn't?

Partly, yes. And some state is known by a combination of the state dump
process listing and the fact that we wait for a "grace period" to occur
before we issue the "statedump_end" event. This ensures that a kernel
worker thread has run on every CPU on the system.

We should take time to discuss all this, ideally with Julien. This is
definitely not an easy problem. We might have to generate a few
synthetic workloads under tracing to figure out the scenarios we have to
deal with.

Thanks,

Mathieu

>
> Francis
>
>
>
> _______________________________________________
> 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

[ltt-dev] [PATCH] Fix initial state bug

[Mathieu Desnoyers]

* Yannick Brosseau (yannick.brosseau at gmail.com) wrote:
> On 11/25/2010 10:29 AM, Francis Giraldeau wrote:
>> This patch fixes a bug with the initial state in the control flow  
>> viewer. When
>> a process was started before starting the trace, in some situation all  
>> the
>> states of this process was wrong, because of a unknown initial state.
>
> +/* get a given quark from lttng module enum value */
> +#define enum_quark(e, f, names) (g_quark_from_string(names[ltt_event_get_unsigned(e, f)]))
> +
>  /* Channel Quarks */
>
> The enum_quark function name sounds too generic to me. Maybe ltt_event_enum_quark or ltt_enum_quark
>
>
> +/*
> + * Quark strings that matches lttng module enums found in statedump
> + * Matching enums are from lttng-modules/ltt-statedump.c
> + */
> +static const char *const lttng_thread_type_names[] = {
> +        "USER_THREAD",     // LTTNG_USER_THREAD
> +        "KERNEL_THREAD"    // LTTNG_KERNEL_THREAD
> +};
>
> Maybe these should be in their own header file to ease the maintenance
> of the list ?

Defining a static const in a header file is wrong. It should be declared
in state.c. Also, shouldn't we declare enums in state.h to match those
in ltt-statedump.c ?

Also, we should change the occurences of e.g. "USER_THREAD" in state.c
for:

  LTTV_STATE_USER_THREAD =
        g_quark_from_static_string(lttng_thread_type_names[ENUM_USER_THREAD]);

and do the same for all these duplicated strings.

Thanks,

Mathieu

>
>
> Yannick
>
>
> _______________________________________________
> 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

[ltt-dev] [PATCH] Fix initial state bug

[Yannick Brosseau]

On 11/25/2010 10:29 AM, Francis Giraldeau wrote:
> This patch fixes a bug with the initial state in the control flow 
> viewer. When
> a process was started before starting the trace, in some situation all 
> the
> states of this process was wrong, because of a unknown initial state.

+/* get a given quark from lttng module enum value */
+#define enum_quark(e, f, names) (g_quark_from_string(names[ltt_event_get_unsigned(e, f)]))
+
  /* Channel Quarks */

The enum_quark function name sounds too generic to me. Maybe ltt_event_enum_quark or ltt_enum_quark


+/*
+ * Quark strings that matches lttng module enums found in statedump
+ * Matching enums are from lttng-modules/ltt-statedump.c
+ */
+static const char *const lttng_thread_type_names[] = {
+        "USER_THREAD",     // LTTNG_USER_THREAD
+        "KERNEL_THREAD"    // LTTNG_KERNEL_THREAD
+};

Maybe these should be in their own header file to ease the maintenance of the list ?


Yannick

[ltt-dev] [PATCH] Fix initial state bug

[Francis Giraldeau]

This patch fixes a bug with the initial state in the control flow viewer. When
a process was started before starting the trace, in some situation all the
states of this process was wrong, because of a unknown initial state.

The patch adds string tables that matches the enums sequence in lttng-modules,
to convert enum value in the state dump to quark.
---
  lttv/lttv/state.c |   21 +++++++--------------
  lttv/lttv/state.h |   34 ++++++++++++++++++++++++++++++++++
  2 files changed, 41 insertions(+), 14 deletions(-)

-------------- next part --------------
A non-text attachment was scrubbed...
Name: 0001-Fix-initial-state-bug.patch
Type: text/x-patch
Size: 5108 bytes
Desc: not available
URL: <http://lists.casi.polymtl.ca/pipermail/ltt-dev/attachments/20101125/8de7be66/attachment-0001.bin>

[ltt-dev] [PATCH] Fix initial state bug

[Francis Giraldeau]

This patch fixes a bug with the initial state in the control flow viewer. When
a process was started before starting the trace, in some situation all the
states of this process was wrong, because of a unknown initial state.

The patch adds string tables that matches the enums sequence in lttng-modules,
to convert enum value in the state dump to quark.
---
  lttv/lttv/state.c |   21 +++++++--------------
  lttv/lttv/state.h |   34 ++++++++++++++++++++++++++++++++++
  2 files changed, 41 insertions(+), 14 deletions(-)

-------------- next part --------------
A non-text attachment was scrubbed...
Name: 0001-Fix-initial-state-bug.patch
Type: text/x-patch
Size: 5108 bytes
Desc: not available
URL: <http://lists.casi.polymtl.ca/pipermail/lttng-dev/attachments/20101125/8de7be66/attachment-0002.bin>

[ltt-dev] [PATCH] Fix initial state bug

[Francis Giraldeau]

This patch fixes a bug with the initial state in the control flow viewer. When
a process was started before starting the trace, in some situation all the
states of this process was wrong, because of a unknown initial state.

The patch adds string tables that matches the enums sequence in lttng-modules,
to convert enum value in the state dump to quark.
---
  lttv/lttv/state.c |   21 +++++++--------------
  lttv/lttv/state.h |   34 ++++++++++++++++++++++++++++++++++
  2 files changed, 41 insertions(+), 14 deletions(-)

-------------- next part --------------
A non-text attachment was scrubbed...
Name: 0001-Fix-initial-state-bug.patch
Type: text/x-patch
Size: 5108 bytes
Desc: not available
URL: <http://lists.casi.polymtl.ca/pipermail/lttng-dev/attachments/20101125/8de7be66/attachment-0003.bin>