[PATCH v2 1/2] cobalt/trace: dovetail: improve xntrace_user_stop and xntrace_user_start

[PATCH v2 1/2] cobalt/trace: dovetail: improve xntrace_user_stop and xntrace_user_start

[Hongzhan Chen]

xntrace_user_stop is widely similar to xntrace_user_freeze, call
trace_cobalt_trigger to improve it and also make xntrace_user_start visible
to balance them.

Signed-off-by: Hongzhan Chen <hongzhan.chen@intel.com>
---
 include/cobalt/kernel/dovetail/pipeline/trace.h | 5 +++--
 1 file changed, 3 insertions(+), 2 deletions(-)

diff --git a/include/cobalt/kernel/dovetail/pipeline/trace.h b/include/cobalt/kernel/dovetail/pipeline/trace.h
index 513d9b48b..479191e0e 100644
--- a/include/cobalt/kernel/dovetail/pipeline/trace.h
+++ b/include/cobalt/kernel/dovetail/pipeline/trace.h
@@ -45,13 +45,14 @@ static inline int xntrace_max_reset(void)
 
 static inline int xntrace_user_start(void)
 {
-	TODO();
+	trace_cobalt_trigger("user-start");
 	return 0;
 }
 
 static inline int xntrace_user_stop(unsigned long v)
 {
-	TODO();
+	trace_cobalt_trace_longval(0, v);
+	trace_cobalt_trigger("user-stop");
 	return 0;
 }
 
-- 
2.17.1

[PATCH v2 2/2] cobalt/trace: dovetail: remove TODO and return -ENOSYS for unportable features

[Hongzhan Chen]

Some of the features are unportable because ftrace does not
provide equivalent functionality to the ipipe tracer (namely double
buffering so that you can continue to record in flight-recorder mode
while a previous recording remains available).

We remove TODOs to pass allyesconfig make.

Signed-off-by: Hongzhan Chen <hongzhan.chen@intel.com>
---
 include/cobalt/kernel/dovetail/pipeline/trace.h | 15 +++++----------
 1 file changed, 5 insertions(+), 10 deletions(-)

diff --git a/include/cobalt/kernel/dovetail/pipeline/trace.h b/include/cobalt/kernel/dovetail/pipeline/trace.h
index 479191e0e..c7df3be79 100644
--- a/include/cobalt/kernel/dovetail/pipeline/trace.h
+++ b/include/cobalt/kernel/dovetail/pipeline/trace.h
@@ -27,20 +27,17 @@
 
 static inline int xntrace_max_begin(unsigned long v)
 {
-	TODO();
-	return 0;
+	return -ENOSYS;
 }
 
 static inline int xntrace_max_end(unsigned long v)
 {
-	TODO();
-	return 0;
+	return -ENOSYS;
 }
 
 static inline int xntrace_max_reset(void)
 {
-	TODO();
-	return 0;
+	return -ENOSYS;
 }
 
 static inline int xntrace_user_start(void)
@@ -96,14 +93,12 @@ static inline int xntrace_tick(unsigned long delay_ticks) /* ns */
 
 static inline int xntrace_panic_freeze(void)
 {
-	TODO();
-	return 0;
+	return -ENOSYS;
 }
 
 static inline int xntrace_panic_dump(void)
 {
-	TODO();
-	return 0;
+	return -ENOSYS;
 }
 
 static inline bool xntrace_enabled(void)
-- 
2.17.1

Re: [PATCH v2 1/2] cobalt/trace: dovetail: improve xntrace_user_stop and xntrace_user_start

[Jan Kiszka]

On 29.11.21 02:03, Hongzhan Chen via Xenomai wrote:
> xntrace_user_stop is widely similar to xntrace_user_freeze, call
> trace_cobalt_trigger to improve it and also make xntrace_user_start visible
> to balance them.
> 
> Signed-off-by: Hongzhan Chen <hongzhan.chen@intel.com>
> ---
>  include/cobalt/kernel/dovetail/pipeline/trace.h | 5 +++--
>  1 file changed, 3 insertions(+), 2 deletions(-)
> 
> diff --git a/include/cobalt/kernel/dovetail/pipeline/trace.h b/include/cobalt/kernel/dovetail/pipeline/trace.h
> index 513d9b48b..479191e0e 100644
> --- a/include/cobalt/kernel/dovetail/pipeline/trace.h
> +++ b/include/cobalt/kernel/dovetail/pipeline/trace.h
> @@ -45,13 +45,14 @@ static inline int xntrace_max_reset(void)
>  
>  static inline int xntrace_user_start(void)
>  {
> -	TODO();
> +	trace_cobalt_trigger("user-start");
>  	return 0;
>  }
>  
>  static inline int xntrace_user_stop(unsigned long v)
>  {
> -	TODO();
> +	trace_cobalt_trace_longval(0, v);
> +	trace_cobalt_trigger("user-stop");
>  	return 0;
>  }
>  
> 

Thanks, applied.

Jan

-- 
Siemens AG, T RDA IOT
Corporate Competence Center Embedded Linux

Re: [PATCH v2 2/2] cobalt/trace: dovetail: remove TODO and return -ENOSYS for unportable features

[Jan Kiszka]

On 29.11.21 02:03, Hongzhan Chen via Xenomai wrote:
> Some of the features are unportable because ftrace does not
> provide equivalent functionality to the ipipe tracer (namely double
> buffering so that you can continue to record in flight-recorder mode
> while a previous recording remains available).
> 
> We remove TODOs to pass allyesconfig make.
> 
> Signed-off-by: Hongzhan Chen <hongzhan.chen@intel.com>
> ---
>  include/cobalt/kernel/dovetail/pipeline/trace.h | 15 +++++----------
>  1 file changed, 5 insertions(+), 10 deletions(-)
> 
> diff --git a/include/cobalt/kernel/dovetail/pipeline/trace.h b/include/cobalt/kernel/dovetail/pipeline/trace.h
> index 479191e0e..c7df3be79 100644
> --- a/include/cobalt/kernel/dovetail/pipeline/trace.h
> +++ b/include/cobalt/kernel/dovetail/pipeline/trace.h
> @@ -27,20 +27,17 @@
>  
>  static inline int xntrace_max_begin(unsigned long v)
>  {
> -	TODO();
> -	return 0;
> +	return -ENOSYS;
>  }
>  
>  static inline int xntrace_max_end(unsigned long v)
>  {
> -	TODO();
> -	return 0;
> +	return -ENOSYS;
>  }
>  
>  static inline int xntrace_max_reset(void)
>  {
> -	TODO();
> -	return 0;
> +	return -ENOSYS;
>  }
>  
>  static inline int xntrace_user_start(void)
> @@ -96,14 +93,12 @@ static inline int xntrace_tick(unsigned long delay_ticks) /* ns */
>  
>  static inline int xntrace_panic_freeze(void)
>  {
> -	TODO();
> -	return 0;
> +	return -ENOSYS;
>  }
>  
>  static inline int xntrace_panic_dump(void)
>  {
> -	TODO();
> -	return 0;
> +	return -ENOSYS;
>  }
>  

Looking at these two panic "abstractions": They are only called by
dovetail-specific handle_oob_trap_entry. There, we should rather use
kernel native tracing functions to stop and dump a trace.

And for I-pipe, we can fold the two abstractions into handle_exception()
there as well.

Jan

-- 
Siemens AG, T RDA IOT
Corporate Competence Center Embedded Linux

RE: [PATCH v2 2/2] cobalt/trace: dovetail: remove TODO and return -ENOSYS for unportable features

[Chen, Hongzhan]

>-----Original Message-----
>From: Jan Kiszka <jan.kiszka@siemens.com> 
>Sent: Monday, November 29, 2021 3:58 PM
>To: Chen, Hongzhan <hongzhan.chen@intel.com>; xenomai@xenomai.org
>Subject: Re: [PATCH v2 2/2] cobalt/trace: dovetail: remove TODO and return -ENOSYS for unportable features
>
>On 29.11.21 02:03, Hongzhan Chen via Xenomai wrote:
>> Some of the features are unportable because ftrace does not
>> provide equivalent functionality to the ipipe tracer (namely double
>> buffering so that you can continue to record in flight-recorder mode
>> while a previous recording remains available).
>> 
>> We remove TODOs to pass allyesconfig make.
>> 
>> Signed-off-by: Hongzhan Chen <hongzhan.chen@intel.com>
>> ---
>>  include/cobalt/kernel/dovetail/pipeline/trace.h | 15 +++++----------
>>  1 file changed, 5 insertions(+), 10 deletions(-)
>> 
>> diff --git a/include/cobalt/kernel/dovetail/pipeline/trace.h b/include/cobalt/kernel/dovetail/pipeline/trace.h
>> index 479191e0e..c7df3be79 100644
>> --- a/include/cobalt/kernel/dovetail/pipeline/trace.h
>> +++ b/include/cobalt/kernel/dovetail/pipeline/trace.h
>> @@ -27,20 +27,17 @@
>>  
>>  static inline int xntrace_max_begin(unsigned long v)
>>  {
>> -	TODO();
>> -	return 0;
>> +	return -ENOSYS;
>>  }
>>  
>>  static inline int xntrace_max_end(unsigned long v)
>>  {
>> -	TODO();
>> -	return 0;
>> +	return -ENOSYS;
>>  }
>>  
>>  static inline int xntrace_max_reset(void)
>>  {
>> -	TODO();
>> -	return 0;
>> +	return -ENOSYS;
>>  }
>>  
>>  static inline int xntrace_user_start(void)
>> @@ -96,14 +93,12 @@ static inline int xntrace_tick(unsigned long delay_ticks) /* ns */
>>  
>>  static inline int xntrace_panic_freeze(void)
>>  {
>> -	TODO();
>> -	return 0;
>> +	return -ENOSYS;
>>  }
>>  
>>  static inline int xntrace_panic_dump(void)
>>  {
>> -	TODO();
>> -	return 0;
>> +	return -ENOSYS;
>>  }
>>  
>
>Looking at these two panic "abstractions": They are only called by
>dovetail-specific handle_oob_trap_entry. There, we should rather use
>kernel native tracing functions to stop and dump a trace.

I found that in EVL implementation of  handle_oob_trap_entry ,especially 
for note_trap implementation , it have not implemented stop and dump
a trace and just try to printk warning (please refer to kernel/evl/thread.c of evl branch).
handle_oob_trap_entry would try to demote current stage and leave in-band to handle
following thing. If we do native thing here, is there duplicated thing happen?

Regards

Hongzhan Chen


>
>And for I-pipe, we can fold the two abstractions into handle_exception()
>there as well.
>
>Jan
>
>-- 
>Siemens AG, T RDA IOT
>Corporate Competence Center Embedded Linux
>
>
>
>

Re: [PATCH v2 2/2] cobalt/trace: dovetail: remove TODO and return -ENOSYS for unportable features

[Jan Kiszka]

On 30.11.21 02:07, Chen, Hongzhan wrote:
> 
> 
>> -----Original Message-----
>> From: Jan Kiszka <jan.kiszka@siemens.com> 
>> Sent: Monday, November 29, 2021 3:58 PM
>> To: Chen, Hongzhan <hongzhan.chen@intel.com>; xenomai@xenomai.org
>> Subject: Re: [PATCH v2 2/2] cobalt/trace: dovetail: remove TODO and return -ENOSYS for unportable features
>>
>> On 29.11.21 02:03, Hongzhan Chen via Xenomai wrote:
>>> Some of the features are unportable because ftrace does not
>>> provide equivalent functionality to the ipipe tracer (namely double
>>> buffering so that you can continue to record in flight-recorder mode
>>> while a previous recording remains available).
>>>
>>> We remove TODOs to pass allyesconfig make.
>>>
>>> Signed-off-by: Hongzhan Chen <hongzhan.chen@intel.com>
>>> ---
>>>  include/cobalt/kernel/dovetail/pipeline/trace.h | 15 +++++----------
>>>  1 file changed, 5 insertions(+), 10 deletions(-)
>>>
>>> diff --git a/include/cobalt/kernel/dovetail/pipeline/trace.h b/include/cobalt/kernel/dovetail/pipeline/trace.h
>>> index 479191e0e..c7df3be79 100644
>>> --- a/include/cobalt/kernel/dovetail/pipeline/trace.h
>>> +++ b/include/cobalt/kernel/dovetail/pipeline/trace.h
>>> @@ -27,20 +27,17 @@
>>>  
>>>  static inline int xntrace_max_begin(unsigned long v)
>>>  {
>>> -	TODO();
>>> -	return 0;
>>> +	return -ENOSYS;
>>>  }
>>>  
>>>  static inline int xntrace_max_end(unsigned long v)
>>>  {
>>> -	TODO();
>>> -	return 0;
>>> +	return -ENOSYS;
>>>  }
>>>  
>>>  static inline int xntrace_max_reset(void)
>>>  {
>>> -	TODO();
>>> -	return 0;
>>> +	return -ENOSYS;
>>>  }
>>>  
>>>  static inline int xntrace_user_start(void)
>>> @@ -96,14 +93,12 @@ static inline int xntrace_tick(unsigned long delay_ticks) /* ns */
>>>  
>>>  static inline int xntrace_panic_freeze(void)
>>>  {
>>> -	TODO();
>>> -	return 0;
>>> +	return -ENOSYS;
>>>  }
>>>  
>>>  static inline int xntrace_panic_dump(void)
>>>  {
>>> -	TODO();
>>> -	return 0;
>>> +	return -ENOSYS;
>>>  }
>>>  
>>
>> Looking at these two panic "abstractions": They are only called by
>> dovetail-specific handle_oob_trap_entry. There, we should rather use
>> kernel native tracing functions to stop and dump a trace.
> 
> I found that in EVL implementation of  handle_oob_trap_entry ,especially 
> for note_trap implementation , it have not implemented stop and dump
> a trace and just try to printk warning (please refer to kernel/evl/thread.c of evl branch).
> handle_oob_trap_entry would try to demote current stage and leave in-band to handle
> following thing. If we do native thing here, is there duplicated thing happen?
> 

This is targeting a real error case: fault in kernel space when
debugging is on. But if we really oops, ftrace_dump_on_panic may just do
the job. So, yes, we could also simply drop this.

Jan

-- 
Siemens AG, T RDA IOT
Corporate Competence Center Embedded Linux

RE: [PATCH v2 2/2] cobalt/trace: dovetail: remove TODO and return -ENOSYS for unportable features

[Chen, Hongzhan]

>-----Original Message-----
>From: Jan Kiszka <jan.kiszka@siemens.com> 
>Sent: Tuesday, November 30, 2021 2:41 PM
>To: Chen, Hongzhan <hongzhan.chen@intel.com>; xenomai@xenomai.org
>Subject: Re: [PATCH v2 2/2] cobalt/trace: dovetail: remove TODO and return -ENOSYS for unportable features
>
>On 30.11.21 02:07, Chen, Hongzhan wrote:
>> 
>> 
>>> -----Original Message-----
>>> From: Jan Kiszka <jan.kiszka@siemens.com> 
>>> Sent: Monday, November 29, 2021 3:58 PM
>>> To: Chen, Hongzhan <hongzhan.chen@intel.com>; xenomai@xenomai.org
>>> Subject: Re: [PATCH v2 2/2] cobalt/trace: dovetail: remove TODO and return -ENOSYS for unportable features
>>>
>>> On 29.11.21 02:03, Hongzhan Chen via Xenomai wrote:
>>>> Some of the features are unportable because ftrace does not
>>>> provide equivalent functionality to the ipipe tracer (namely double
>>>> buffering so that you can continue to record in flight-recorder mode
>>>> while a previous recording remains available).
>>>>
>>>> We remove TODOs to pass allyesconfig make.
>>>>
>>>> Signed-off-by: Hongzhan Chen <hongzhan.chen@intel.com>
>>>> ---
>>>>  include/cobalt/kernel/dovetail/pipeline/trace.h | 15 +++++----------
>>>>  1 file changed, 5 insertions(+), 10 deletions(-)
>>>>
>>>> diff --git a/include/cobalt/kernel/dovetail/pipeline/trace.h b/include/cobalt/kernel/dovetail/pipeline/trace.h
>>>> index 479191e0e..c7df3be79 100644
>>>> --- a/include/cobalt/kernel/dovetail/pipeline/trace.h
>>>> +++ b/include/cobalt/kernel/dovetail/pipeline/trace.h
>>>> @@ -27,20 +27,17 @@
>>>>  
>>>>  static inline int xntrace_max_begin(unsigned long v)
>>>>  {
>>>> -	TODO();
>>>> -	return 0;
>>>> +	return -ENOSYS;
>>>>  }
>>>>  
>>>>  static inline int xntrace_max_end(unsigned long v)
>>>>  {
>>>> -	TODO();
>>>> -	return 0;
>>>> +	return -ENOSYS;
>>>>  }
>>>>  
>>>>  static inline int xntrace_max_reset(void)
>>>>  {
>>>> -	TODO();
>>>> -	return 0;
>>>> +	return -ENOSYS;
>>>>  }
>>>>  
>>>>  static inline int xntrace_user_start(void)
>>>> @@ -96,14 +93,12 @@ static inline int xntrace_tick(unsigned long delay_ticks) /* ns */
>>>>  
>>>>  static inline int xntrace_panic_freeze(void)
>>>>  {
>>>> -	TODO();
>>>> -	return 0;
>>>> +	return -ENOSYS;
>>>>  }
>>>>  
>>>>  static inline int xntrace_panic_dump(void)
>>>>  {
>>>> -	TODO();
>>>> -	return 0;
>>>> +	return -ENOSYS;
>>>>  }
>>>>  
>>>
>>> Looking at these two panic "abstractions": They are only called by
>>> dovetail-specific handle_oob_trap_entry. There, we should rather use
>>> kernel native tracing functions to stop and dump a trace.
>> 
>> I found that in EVL implementation of  handle_oob_trap_entry ,especially 
>> for note_trap implementation , it have not implemented stop and dump
>> a trace and just try to printk warning (please refer to kernel/evl/thread.c of evl branch).
>> handle_oob_trap_entry would try to demote current stage and leave in-band to handle
>> following thing. If we do native thing here, is there duplicated thing happen?
>> 
>
>This is targeting a real error case: fault in kernel space when
>debugging is on. But if we really oops, ftrace_dump_on_panic may just do
>the job. So, yes, we could also simply drop this.

So there is no need to do further modification for xntrace_panic_freeze and xntrace_panic_dump based 
on this patch? Would you apply it?

Regards

Hongzhan Chen

>
>Jan
>
>-- 
>Siemens AG, T RDA IOT
>Corporate Competence Center Embedded Linux
>
>
>
>

Re: [PATCH v2 2/2] cobalt/trace: dovetail: remove TODO and return -ENOSYS for unportable features

[Jan Kiszka]

On 30.11.21 07:56, Chen, Hongzhan wrote:
> 
> 
>> -----Original Message-----
>> From: Jan Kiszka <jan.kiszka@siemens.com> 
>> Sent: Tuesday, November 30, 2021 2:41 PM
>> To: Chen, Hongzhan <hongzhan.chen@intel.com>; xenomai@xenomai.org
>> Subject: Re: [PATCH v2 2/2] cobalt/trace: dovetail: remove TODO and return -ENOSYS for unportable features
>>
>> On 30.11.21 02:07, Chen, Hongzhan wrote:
>>>
>>>
>>>> -----Original Message-----
>>>> From: Jan Kiszka <jan.kiszka@siemens.com> 
>>>> Sent: Monday, November 29, 2021 3:58 PM
>>>> To: Chen, Hongzhan <hongzhan.chen@intel.com>; xenomai@xenomai.org
>>>> Subject: Re: [PATCH v2 2/2] cobalt/trace: dovetail: remove TODO and return -ENOSYS for unportable features
>>>>
>>>> On 29.11.21 02:03, Hongzhan Chen via Xenomai wrote:
>>>>> Some of the features are unportable because ftrace does not
>>>>> provide equivalent functionality to the ipipe tracer (namely double
>>>>> buffering so that you can continue to record in flight-recorder mode
>>>>> while a previous recording remains available).
>>>>>
>>>>> We remove TODOs to pass allyesconfig make.
>>>>>
>>>>> Signed-off-by: Hongzhan Chen <hongzhan.chen@intel.com>
>>>>> ---
>>>>>  include/cobalt/kernel/dovetail/pipeline/trace.h | 15 +++++----------
>>>>>  1 file changed, 5 insertions(+), 10 deletions(-)
>>>>>
>>>>> diff --git a/include/cobalt/kernel/dovetail/pipeline/trace.h b/include/cobalt/kernel/dovetail/pipeline/trace.h
>>>>> index 479191e0e..c7df3be79 100644
>>>>> --- a/include/cobalt/kernel/dovetail/pipeline/trace.h
>>>>> +++ b/include/cobalt/kernel/dovetail/pipeline/trace.h
>>>>> @@ -27,20 +27,17 @@
>>>>>  
>>>>>  static inline int xntrace_max_begin(unsigned long v)
>>>>>  {
>>>>> -	TODO();
>>>>> -	return 0;
>>>>> +	return -ENOSYS;
>>>>>  }
>>>>>  
>>>>>  static inline int xntrace_max_end(unsigned long v)
>>>>>  {
>>>>> -	TODO();
>>>>> -	return 0;
>>>>> +	return -ENOSYS;
>>>>>  }
>>>>>  
>>>>>  static inline int xntrace_max_reset(void)
>>>>>  {
>>>>> -	TODO();
>>>>> -	return 0;
>>>>> +	return -ENOSYS;
>>>>>  }
>>>>>  
>>>>>  static inline int xntrace_user_start(void)
>>>>> @@ -96,14 +93,12 @@ static inline int xntrace_tick(unsigned long delay_ticks) /* ns */
>>>>>  
>>>>>  static inline int xntrace_panic_freeze(void)
>>>>>  {
>>>>> -	TODO();
>>>>> -	return 0;
>>>>> +	return -ENOSYS;
>>>>>  }
>>>>>  
>>>>>  static inline int xntrace_panic_dump(void)
>>>>>  {
>>>>> -	TODO();
>>>>> -	return 0;
>>>>> +	return -ENOSYS;
>>>>>  }
>>>>>  
>>>>
>>>> Looking at these two panic "abstractions": They are only called by
>>>> dovetail-specific handle_oob_trap_entry. There, we should rather use
>>>> kernel native tracing functions to stop and dump a trace.
>>>
>>> I found that in EVL implementation of  handle_oob_trap_entry ,especially 
>>> for note_trap implementation , it have not implemented stop and dump
>>> a trace and just try to printk warning (please refer to kernel/evl/thread.c of evl branch).
>>> handle_oob_trap_entry would try to demote current stage and leave in-band to handle
>>> following thing. If we do native thing here, is there duplicated thing happen?
>>>
>>
>> This is targeting a real error case: fault in kernel space when
>> debugging is on. But if we really oops, ftrace_dump_on_panic may just do
>> the job. So, yes, we could also simply drop this.
> 
> So there is no need to do further modification for xntrace_panic_freeze and xntrace_panic_dump based 
> on this patch? Would you apply it?
> 

If you drop those calls from dovetail, yes.

Jan

-- 
Siemens AG, T RDA IOT
Corporate Competence Center Embedded Linux