external/error_report: Add Infinity wait error reporting feature to T… #2329

dr-venkman · 2018-10-11T10:33:06Z

Add Infinity wait error reporting feature, where callstack of threads in a seemingly endless wait are reported to a server. This can also be used to inspect threads that are possibly deadlocked.

Following are the summarized details of the feature add-on:

Definition of an appropriate Kconfig for the infinity wait feature. Also, turning on active cpu load monitoring when this feature is requested, which is necessary.
Implementation of the infinity wait policy under external/error_report
Replacing all scheduler-based APIs at external/error_report with apporiate ioctls, and implementing a os/drivers/error_report to place the scheduler API calls instead.
Implementing a scenario for an infinitely waiting thread pair in the sample error_report_demo application
Moving the error_report_init API call to os_bringup function from its previous location.

seinfra · 2018-10-11T10:44:57Z

Target : [9127a4bbcd96fe3f775c36dc8698367a40cc24b8] -

seinfra · 2018-10-11T10:45:23Z

Target : [9127a4bbcd96fe3f775c36dc8698367a40cc24b8] -

seinfra · 2018-10-11T10:50:10Z

Target : [332af3c61c068f3cf2ca9b6b69d3aeec2ba4e447] - Code Rule Check (C++) OK.

seinfra · 2018-10-11T10:50:55Z

Target : [332af3c61c068f3cf2ca9b6b69d3aeec2ba4e447] - Code Rule Check Result:
apps/examples/err_report_demo/err_report_main.c:281: ERROR: [BRC_M_FTN] open brace '{' following function declarations go on the next line
apps/examples/err_report_demo/err_report_main.c:287: ERROR: [BRC_M_FTN] open brace '{' following function declarations go on the next line
apps/examples/err_report_demo/err_report_main.c:293: ERROR: [BRC_M_FTN] open brace '{' following function declarations go on the next line
os/drivers/error_report/error_report_drv.c:73: ERROR: [SPC_M_KWD] space required before the open parenthesis '('
os/drivers/error_report/error_report_drv.c:82: ERROR: [SPC_M_KWD] space required before the open parenthesis '('
os/drivers/error_report/error_report_drv.c:85: ERROR: [BRC_M_FTN] open brace '{' following function declarations go on the next line
os/drivers/error_report/error_report_drv.c:122: ERROR: [BRC_M_FTN] open brace '{' following function declarations go on the next line
os/drivers/error_report/error_report_drv.c:144: ERROR: [BRC_M_SMT] else should follow close brace '}'
os/drivers/error_report/error_report_drv.c:174: ERROR: [BRC_M_SMT] else should follow close brace '}'
os/drivers/error_report/error_report_drv.c:178: ERROR: [BRC_M_SMT] else should follow close brace '}'
os/drivers/error_report/error_report_drv.c:193: ERROR: [BRC_M_SMT] else should follow close brace '}'

seinfra · 2018-10-11T11:04:14Z

Target : [02c140ca1275e20cb97dcfacc371e6cc6d6096c1] - Code Rule Check Result:
os/drivers/error_report/error_report_drv.c:146: ERROR: [BRC_M_SMT] else should follow close brace '}'
os/drivers/error_report/error_report_drv.c:176: ERROR: [BRC_M_SMT] else should follow close brace '}'
os/drivers/error_report/error_report_drv.c:180: ERROR: [BRC_M_SMT] else should follow close brace '}'
os/drivers/error_report/error_report_drv.c:195: ERROR: [BRC_M_SMT] else should follow close brace '}'

seinfra · 2018-10-11T11:09:03Z

Target : [02c140ca1275e20cb97dcfacc371e6cc6d6096c1] - Code Rule Check (C++) OK.

sunghan-chang · 2018-10-11T11:47:20Z

apps/examples/err_report_demo/err_report_main.c

+	wifi_manager_deinit();
+	error_report_deinit();
+	pthread_cancel(thread1);
+	pthread_cancel(thread2);


You should call pthread_join or pthread_detach to prevent memory leak.

Thanks for pointing this out. I will implement as you suggest.

sunghan-chang · 2018-10-11T11:48:14Z

apps/examples/err_report_demo/err_report_main.c

@@ -285,6 +349,10 @@ static void error_report_test(const char *endpoint)
 	/* Verify multiple errors across WiFi Manager */
 	error_report_multiple(endpoint);

+	/* Verify Infinite Wait */
+#if CONFIG_ERROR_REPORT_INFINITE_WAIT


Addressed below. Thank you.

sunghan-chang · 2018-10-11T11:49:31Z

apps/examples/err_report_demo/err_report_main.c

+	ERR_REPORT_TEST_WAIT;
+	sem_init(&g_sem1, 0, 0);
+	sem_init(&g_sem2, 0, 0);
+	if ((r = pthread_create(&thread1, NULL, (pthread_startroutine_t) prv_thread1, NULL)) != 0) {


Let's remove a space after type cast.

Thank you. Will modify as suggested.

sunghan-chang · 2018-10-11T11:50:59Z

external/error_report/Kconfig

+config ERROR_REPORT_INFINITE_WAIT
+	bool "Report error for infinitely waiting threads"
+	default n
+	select SCHED_CPULOAD


Why do you make a dependancy with SCHED_CPULOAD?

This is done because the feature needs CPULOAD monitoring for it to work.

sunghan-chang · 2018-10-11T11:52:21Z

external/error_report/error_report.c

@@ -43,40 +46,147 @@ typedef struct {
 	uint16_t front;
 	int8_t q_pending;
 	pthread_mutex_t err_mutex;
+#if CONFIG_ERROR_REPORT_INFINITE_WAIT


#ifdef
Please check all of this.

Thanks for pointing out. Will carry out the changes.

sunghan-chang · 2018-10-11T12:28:35Z

os/drivers/error_report/error_report_drv.c

+#include <stdint.h>
+#include <errno.h>
+#include <debug.h>
+#include <apps/builtin.h>


why this in here?

You are right, this isn't needed. Thanks for pointing out.

sunghan-chang · 2018-10-11T12:29:41Z

os/drivers/error_report/error_report_drv.c

+	if (taskname == NULL) {
+		return -1;
+	}
+	for (i = 0; i < sizeof(g_exceptional_waits)/sizeof(g_exceptional_waits[0]); i++) {


Thanks, spaces added for readability.

sunghan-chang · 2018-10-11T12:36:28Z

os/drivers/error_report/error_report_drv.c

+			waitdata->ncalls++;
+		}
+		waitdata->task_state = tcb->task_state;
+		waitdata->entry = prv_fetch_taskaddr(waitdata->pid);


sched_gettcb is called twice in this case. Line 252 and here.

Ok, I factored the prv_fetch_taskaddr to include a macro RETURN_ADDR_FROM_TCB for fetching task addr given a tcb pointer. So, the above statement can be replaced by a macro call to RETURN_ADDR_FROM_TCB.

sunghan-chang · 2018-10-11T12:39:31Z

os/drivers/error_report/error_report_drv.c

+static void prv_tcb_handler(FAR struct tcb_s *tcb, void *args)
+{
+	int pos = 0;
+	if (tcb->pid < 5) {


Why 5? for idle, hpwork, lpwork, and so on? Can you guarantee those are always enabled?

Removed the condition check, as these tasks are now considered as exceptional cases, and will have a larger time window for reporting purposes. Thanks again.

sunghan-chang · 2018-10-11T12:41:11Z

os/drivers/error_report/error_report_drv.c

+						if ((tcb->flags & TCB_FLAG_TTYPE_MASK) == TCB_FLAG_TTYPE_PTHREAD) {
+							task_addr = (unsigned long)e.pthread;
+						} else {
+							task_addr = (unsigned long)e.main;


Why don't you use prv_fetch_taskaddr which you made?

Maybe you already got the tcb structure so that it is little different between here and prv_fetch_taskaddr.
Then, you had better refactor codes to make a function for duplicated operations.

Refactored using the RETURN_ADDR_FROM_TCB macro call.

sunghan-chang · 2018-10-11T12:45:51Z

os/drivers/error_report/error_report_drv.c

+						} else {
+							task_addr = (unsigned long)e.main;
+						}					
+						saved_pc = tcb->xcp.regs[13];


Is it enough to debug only with regs[13]?

Thanks for the question. The tcb structure provides many fields to debug, of which regs[13] points to the stack. Assuming that developers would prefer something similar to a gdb> backtrace, we figured this would be a great place to start with. Please feel free to share any additional fields in tcb structure that can simplify debugging.

sunghan-chang · 2018-10-11T12:46:44Z

@dr-venkman Can you find the root cause with provided information by error report?

thapav · 2018-10-11T13:16:20Z

os/drivers/error_report/error_report_drv.c

+		} else if (g_thd_waitq[pos].pid) {
+			/* Collision resolution, find the slot using linear search */
+			int i;
+			for (i = pos+1; i != pos; i = (i+1)%CONFIG_MAX_TASKS) {


Please insert spaces between operator and operands.

Thanks for pointing out. Will modify as suggested.

thapav · 2018-10-11T13:16:38Z

os/drivers/error_report/error_report_drv.c

+		else {
+			/* Collision resolution, find the next empty slot */
+			int i;
+			for (i = pos+1; i != pos; i = (i+1)%CONFIG_MAX_TASKS) {


Please insert spaces between operator and operands.

Thanks for pointing out. Will modify as suggested.

thapav · 2018-10-11T13:17:38Z

os/drivers/error_report/error_report_drv.c

+{
+	int ret = -EINVAL;
+
+


extra empty line can be removed

Removed empty lines for readability. Thank you.

thapav · 2018-10-11T13:19:00Z

os/drivers/error_report/error_report_drv.c

+ *
+ * Description:
+ *   Register task management path, ERROR_REPORT_DRVPATH
+ *


Description for parameter could be added here

Added detailed description for the function, including the parameter.

dr-venkman · 2018-10-12T04:41:10Z

@dr-venkman Can you find the root cause with provided information by error report?

That is a broader area of work, as root cause analysis might involve more pieces of information, and possibly fault diagnosis algorithms as well. For now, this commit provides the callstack of threads that are inferrred to be waiting infinitely (a large elapsed time interval, in practice) long in time. I hope that answers sufficiently. Please let me know if you need further details.

dr-venkman

Addressed most, if not all of the comments. Thanks a lot for your inputs.

seinfra · 2018-10-12T05:21:54Z

Target : [5e4b199422c6ff46071e28d137e4144cced26c4f] - Code Rule Check Result:
os/drivers/error_report/error_report_drv.c:94: ERROR: [SPC_M_KWD] space required before the open parenthesis '('
os/drivers/error_report/error_report_drv.c:148: ERROR: [BRC_M_SMT] that open brace { should be on the previous line
os/drivers/error_report/error_report_drv.c:148: ERROR: [BRC_M_SMT] else should follow close brace '}'
os/drivers/error_report/error_report_drv.c:179: ERROR: [BRC_M_SMT] that open brace { should be on the previous line
os/drivers/error_report/error_report_drv.c:179: ERROR: [BRC_M_SMT] else should follow close brace '}'
os/drivers/error_report/error_report_drv.c:184: ERROR: [BRC_M_SMT] that open brace { should be on the previous line
os/drivers/error_report/error_report_drv.c:184: ERROR: [BRC_M_SMT] else should follow close brace '}'
os/drivers/error_report/error_report_drv.c:200: ERROR: [BRC_M_SMT] that open brace { should be on the previous line
os/drivers/error_report/error_report_drv.c:200: ERROR: [BRC_M_SMT] else should follow close brace '}'

seinfra · 2018-10-12T05:27:02Z

Target : [5e4b199422c6ff46071e28d137e4144cced26c4f] - Code Rule Check (C++) OK.

seinfra · 2018-10-12T05:34:19Z

Target : [3a1891a81abbe6845f40f94301b427c7359ffa3f] - Code Rule Check OK.

seinfra · 2018-10-12T05:39:22Z

Target : [3a1891a81abbe6845f40f94301b427c7359ffa3f] - Code Rule Check (C++) OK.

seinfra · 2018-10-12T06:38:26Z

Target : [9e23482628431960553b8d26a4a2fa565e5af6af] - Code Rule Check OK.

seinfra · 2018-10-12T06:38:53Z

Target : [9e23482628431960553b8d26a4a2fa565e5af6af] - Code Rule Check (C++) OK.

seinfra · 2018-10-12T08:18:59Z

Target : [ae8ecb84561dd9ccfe2f6fc6bb14750be02dede2] - Code Rule Check OK.

seinfra · 2018-10-12T08:23:45Z

Target : [ae8ecb84561dd9ccfe2f6fc6bb14750be02dede2] - Code Rule Check (C++) OK.

…izenRT Add Infinity wait error reporting feature, where callstack of threads in a seemingly endless wait are reported to a server. This can also be used to inspect threads that are possibly deadlocked. Following are the summarized details of the feature add-on: 1. Definition of an appropriate Kconfig for the infinity wait feature. Also, turning on active cpu load monitoring when this feature is requested, which is necessary. 2. Implementation of the infinity wait policy under `external/error_report` 3. Replacing all scheduler-based APIs at `external/error_report` with apporiate `ioctls`, and implementing a `os/drivers/error_report` to place the scheduler API calls instead. 4. Implementing a scenario for an infinitely waiting thread pair in the sample `error_report_demo` application 5. Moving the `error_report_init` API call to `os_bringup` function from its previous location.

seinfra · 2018-10-15T02:00:20Z

Target : [bb3c4ed] - Code Rule Check (C++) OK.

seinfra · 2018-10-15T02:00:50Z

Target : [bb3c4ed] - Code Rule Check OK.

juitem · 2018-10-16T11:22:27Z

os/drivers/error_report/error_report_drv.c

+		uint32_t *ptr;
+		ptr = (uint32_t *)tcb->xcp.regs[13];
+		while ((void *)ptr < tcb->adj_stack_ptr && (waitdata->ncalls < CONFIG_ERROR_REPORT_BACKTRACE_MAX_DEPTH)) {
+			if (*ptr >= 0x04000000 && *ptr <= 0x04800000) {


it's cypress's DDR address right?
do not use numbers directly.

juitem · 2018-10-16T14:37:13Z

I think this method has several restrictions.

This thread should be top most priority.
Kernel / platform / network lockups cannot be reported. Only Application level lock ups can be detected. Which can be obtained with 'ps' command in TASH

Thread is newly created and checking CPU loads which seems too much overhead compared with adding implentation on scheduler.
I think we can discuss convenient methods

seinfra · 2019-04-24T12:01:35Z

Target : [bb3c4ed] - Code Rule Check OK.

seinfra · 2019-04-24T12:13:20Z

Target : [bb3c4ed] - Code Rule Check (C++) OK.

seinfra · 2019-04-24T12:19:52Z

Target : [bb3c4ed] - Code Rule Check (C++) OK.

seinfra · 2019-11-12T08:24:06Z

Target : [bb3c4ed] - Code Rule Check OK.

seinfra · 2019-11-12T08:28:05Z

Target : [bb3c4ed] - Code Rule Check (C++) OK.

tizen-build · 2020-04-10T03:10:43Z

Target : [bb3c4ed] - Code Rule Check OK.

tizen-build · 2023-03-21T02:17:21Z

Target : [bb3c4ed] - Code Rule Check OK.

dr-venkman requested a review from sunghan-chang October 11, 2018 10:33

dr-venkman force-pushed the add_infinite_thread_wait_errreport branch 2 times, most recently from 9127a4b to 332af3c Compare October 11, 2018 10:41

dr-venkman force-pushed the add_infinite_thread_wait_errreport branch from 332af3c to 02c140c Compare October 11, 2018 10:56

sunghan-chang reviewed Oct 11, 2018

View reviewed changes

thapav reviewed Oct 11, 2018

View reviewed changes

dr-venkman requested a review from pillip8282 October 12, 2018 01:23

dr-venkman force-pushed the add_infinite_thread_wait_errreport branch from 02c140c to 5e4b199 Compare October 12, 2018 05:14

dr-venkman commented Oct 12, 2018

View reviewed changes

dr-venkman force-pushed the add_infinite_thread_wait_errreport branch from 5e4b199 to 3a1891a Compare October 12, 2018 05:28

dr-venkman force-pushed the add_infinite_thread_wait_errreport branch 2 times, most recently from 26bf49d to 9e23482 Compare October 12, 2018 06:32

dr-venkman force-pushed the add_infinite_thread_wait_errreport branch from 9e23482 to ae8ecb8 Compare October 12, 2018 08:12

dr-venkman force-pushed the add_infinite_thread_wait_errreport branch from ae8ecb8 to bb3c4ed Compare October 15, 2018 01:46

juitem reviewed Oct 16, 2018

View reviewed changes

external/error_report: Add Infinity wait error reporting feature to T… #2329

Are you sure you want to change the base?

external/error_report: Add Infinity wait error reporting feature to T… #2329

Conversation

dr-venkman commented Oct 11, 2018

seinfra commented Oct 11, 2018

seinfra commented Oct 11, 2018

seinfra commented Oct 11, 2018

seinfra commented Oct 11, 2018

seinfra commented Oct 11, 2018

seinfra commented Oct 11, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sunghan-chang commented Oct 11, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dr-venkman commented Oct 12, 2018 • edited Loading

dr-venkman left a comment

Choose a reason for hiding this comment

seinfra commented Oct 12, 2018

seinfra commented Oct 12, 2018

seinfra commented Oct 12, 2018

seinfra commented Oct 12, 2018

seinfra commented Oct 12, 2018

seinfra commented Oct 12, 2018

seinfra commented Oct 12, 2018

seinfra commented Oct 12, 2018

seinfra commented Oct 15, 2018

seinfra commented Oct 15, 2018

Choose a reason for hiding this comment

juitem commented Oct 16, 2018

seinfra commented Apr 24, 2019

seinfra commented Apr 24, 2019

seinfra commented Apr 24, 2019

seinfra commented Nov 12, 2019

seinfra commented Nov 12, 2019

tizen-build commented Apr 10, 2020

tizen-build commented Mar 21, 2023

dr-venkman commented Oct 12, 2018 •

edited

Loading