From 639e68a621b7ae8c4a296ca7e45b47075268fded Mon Sep 17 00:00:00 2001 From: Tim Newsome Date: Thu, 15 Sep 2022 11:41:33 -0700 Subject: [PATCH 1/6] Make poll backoff time based. Requested in https://review.openocd.org/c/openocd/+/6964. I'm making the change here so I have a chance to test it properly before pushing it upstream. A nice effect of this change is that we avoid some unnecessary polling early on when gdb is connecting and we would poll once every time we send gdb a qXfer packet. Change-Id: I4bdb9f05839e8c1e01ff6dde49d6589595418095 Signed-off-by: Tim Newsome --- src/target/target.c | 61 +++++++++++++++++++++------------------------ src/target/target.h | 7 ++++-- 2 files changed, 34 insertions(+), 34 deletions(-) diff --git a/src/target/target.c b/src/target/target.c index 7d3861ab5..3a5651309 100644 --- a/src/target/target.c +++ b/src/target/target.c @@ -3046,44 +3046,41 @@ static int handle_target(void *priv) is_jtag_poll_safe() && target; target = target->next) { - if (!target->tap->enabled) + /* This function only gets called every polling_interval, so + * allow some slack in the time comparison. Otherwise, if we + * schedule for now+polling_interval, the next poll won't + * actually happen until a polling_interval later. */ + if (!target->tap->enabled || + power_dropout || + srst_asserted || + timeval_ms() + polling_interval / 2 < target->backoff.next_attempt) continue; - if (target->backoff.times > target->backoff.count) { - /* do not poll this time as we failed previously */ - target->backoff.count++; - continue; + /* polling may fail silently until the target has been examined */ + retval = target_poll(target); + if (retval == ERROR_OK) { + target->backoff.interval = polling_interval; + } else { + /* Increase interval between polling up to 5000ms */ + target->backoff.interval = MAX(polling_interval, + MIN(target->backoff.interval * 2 + 1, 5000)); + /* Tell GDB to halt the debugger. This allows the user to run + * monitor commands to handle the situation. */ + target_call_event_callbacks(target, TARGET_EVENT_GDB_HALT); } - target->backoff.count = 0; + target->backoff.next_attempt = timeval_ms() + target->backoff.interval; + LOG_TARGET_DEBUG(target, "target_poll() -> %d, next attempt in %dms", + retval, target->backoff.interval); - /* only poll target if we've got power and srst isn't asserted */ - if (!power_dropout && !srst_asserted) { - /* polling may fail silently until the target has been examined */ - retval = target_poll(target); + if (retval != ERROR_OK && examine_attempted) { + target_reset_examined(target); + retval = target_examine_one(target); if (retval != ERROR_OK) { - /* 100ms polling interval. Increase interval between polling up to 5000ms */ - if (target->backoff.times * polling_interval < 5000) - target->backoff.times = MIN(target->backoff.times * 2 + 1, - 5000 / polling_interval); - - /* Tell GDB to halt the debugger. This allows the user to - * run monitor commands to handle the situation. - */ - target_call_event_callbacks(target, TARGET_EVENT_GDB_HALT); + LOG_TARGET_DEBUG(target, "Examination failed, GDB will be halted. " + "Polling again in %dms", + target->backoff.interval); + return retval; } - if (target->backoff.times > 0 && examine_attempted) { - LOG_DEBUG("[%s] Polling failed, trying to reexamine", target_name(target)); - target_reset_examined(target); - retval = target_examine_one(target); - if (retval != ERROR_OK) { - LOG_DEBUG("[%s] Examination failed, GDB will be halted. Polling again in %dms", - target_name(target), target->backoff.times * polling_interval); - return retval; - } - } - - /* Since we succeeded, we reset backoff count */ - target->backoff.times = 0; } } diff --git a/src/target/target.h b/src/target/target.h index 4b494d8d5..726a14d3f 100644 --- a/src/target/target.h +++ b/src/target/target.h @@ -117,8 +117,8 @@ struct gdb_service { /* target back off timer */ struct backoff_timer { - int times; - int count; + int64_t next_attempt; + unsigned int interval; }; /* split target registers into multiple class */ @@ -199,6 +199,9 @@ struct target { struct rtos *rtos; /* Instance of Real Time Operating System support */ bool rtos_auto_detect; /* A flag that indicates that the RTOS has been specified as "auto" * and must be detected when symbols are offered */ + /* Track when next to poll(). If polling is failing, we don't want to + * poll too quickly because we'll just overwhelm the user with error + * messages. */ struct backoff_timer backoff; int smp; /* add some target attributes for smp support */ struct list_head *smp_targets; /* list all targets in this smp group/cluster From 4004db5d3a7e767aa4a6b7fddf92ac3370670c79 Mon Sep 17 00:00:00 2001 From: Tim Newsome Date: Fri, 16 Sep 2022 16:51:57 -0700 Subject: [PATCH 2/6] Make polling_interval unsigned. Hopefully fixes win32 build. Change-Id: I13d6d475f03bada96b2eb943f2c16df05413d34f Signed-off-by: Tim Newsome --- src/target/target.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/target/target.c b/src/target/target.c index 3a5651309..35c5be8f6 100644 --- a/src/target/target.c +++ b/src/target/target.c @@ -159,7 +159,7 @@ static struct target_timer_callback *target_timer_callbacks; static int64_t target_timer_next_event_value; static LIST_HEAD(target_reset_callback_list); static LIST_HEAD(target_trace_callback_list); -static const int polling_interval = TARGET_DEFAULT_POLLING_INTERVAL; +static const unsigned int polling_interval = TARGET_DEFAULT_POLLING_INTERVAL; static LIST_HEAD(empty_smp_targets); static const struct jim_nvp nvp_assert[] = { From 89746e111b11ea6cb5cc2cb76edfc4e31ff88038 Mon Sep 17 00:00:00 2001 From: Tim Newsome Date: Mon, 19 Sep 2022 09:43:51 -0700 Subject: [PATCH 3/6] Fix comment indent. Co-authored-by: Jan Matyas <50193733+JanMatCodasip@users.noreply.github.com> Signed-off-by: Tim Newsome --- src/target/target.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/target/target.c b/src/target/target.c index 35c5be8f6..e663fff63 100644 --- a/src/target/target.c +++ b/src/target/target.c @@ -3065,7 +3065,7 @@ static int handle_target(void *priv) target->backoff.interval = MAX(polling_interval, MIN(target->backoff.interval * 2 + 1, 5000)); /* Tell GDB to halt the debugger. This allows the user to run - * monitor commands to handle the situation. */ + * monitor commands to handle the situation. */ target_call_event_callbacks(target, TARGET_EVENT_GDB_HALT); } target->backoff.next_attempt = timeval_ms() + target->backoff.interval; From fa1abc63d22a9b6511a73025bd7e80d5b7d411af Mon Sep 17 00:00:00 2001 From: Tim Newsome Date: Mon, 19 Sep 2022 09:44:10 -0700 Subject: [PATCH 4/6] Add explanatory comment. Co-authored-by: Jan Matyas <50193733+JanMatCodasip@users.noreply.github.com> Signed-off-by: Tim Newsome --- src/target/target.c | 1 + 1 file changed, 1 insertion(+) diff --git a/src/target/target.c b/src/target/target.c index e663fff63..5e2fe8cda 100644 --- a/src/target/target.c +++ b/src/target/target.c @@ -3059,6 +3059,7 @@ static int handle_target(void *priv) /* polling may fail silently until the target has been examined */ retval = target_poll(target); if (retval == ERROR_OK) { + /* Polling succeeded, reset the back-off interval */ target->backoff.interval = polling_interval; } else { /* Increase interval between polling up to 5000ms */ From b7738370b75543870c4cbb15061b8e7b38887e9e Mon Sep 17 00:00:00 2001 From: Tim Newsome Date: Mon, 19 Sep 2022 09:48:09 -0700 Subject: [PATCH 5/6] Make large if() more readable. Change-Id: Ie43400387ab9f290e744ebaa09786612237e6c7e Signed-off-by: Tim Newsome --- src/target/target.c | 6 ++---- 1 file changed, 2 insertions(+), 4 deletions(-) diff --git a/src/target/target.c b/src/target/target.c index 5e2fe8cda..5867149a7 100644 --- a/src/target/target.c +++ b/src/target/target.c @@ -3050,10 +3050,8 @@ static int handle_target(void *priv) * allow some slack in the time comparison. Otherwise, if we * schedule for now+polling_interval, the next poll won't * actually happen until a polling_interval later. */ - if (!target->tap->enabled || - power_dropout || - srst_asserted || - timeval_ms() + polling_interval / 2 < target->backoff.next_attempt) + const bool poll_needed = timeval_ms() + polling_interval / 2 >= target->backoff.next_attempt; + if (!target->tap->enabled || power_dropout || srst_asserted || !poll_needed) continue; /* polling may fail silently until the target has been examined */ From 3c5be531dfe40d67199ffbb57c28b7bd96651923 Mon Sep 17 00:00:00 2001 From: Tim Newsome Date: Thu, 22 Sep 2022 09:58:24 -0700 Subject: [PATCH 6/6] Don't use const on temporary variable. It's not part of OpenOCD style: https://review.openocd.org/c/openocd/+/6319/5..10/src/target/aarch64.c#b1500 Change-Id: Ifb612a942507ca5ed8cac3e3ec59e0e14b0298ed Signed-off-by: Tim Newsome --- src/target/target.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/target/target.c b/src/target/target.c index 5867149a7..927329a64 100644 --- a/src/target/target.c +++ b/src/target/target.c @@ -3050,7 +3050,7 @@ static int handle_target(void *priv) * allow some slack in the time comparison. Otherwise, if we * schedule for now+polling_interval, the next poll won't * actually happen until a polling_interval later. */ - const bool poll_needed = timeval_ms() + polling_interval / 2 >= target->backoff.next_attempt; + bool poll_needed = timeval_ms() + polling_interval / 2 >= target->backoff.next_attempt; if (!target->tap->enabled || power_dropout || srst_asserted || !poll_needed) continue;