libs, bgpd: improve task cancellation by argument value #7951
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
polychaeta
reviewed
Jan 27, 2021
There was a problem hiding this comment.
Thanks for your contribution to FRR!
Click for style suggestions
To apply these suggestions:
curl -s https://gist.githubusercontent.com/polychaeta/00795bd92d1681e0128b005233ecbf10/raw/c2a3b72221affd811676a298cb71378bfecb39fb/cr_7951_1611776346.diff | git apply
diff --git a/lib/thread.c b/lib/thread.c
index bf7cf9def..346ab83af 100644
--- a/lib/thread.c
+++ b/lib/thread.c
@@ -1125,7 +1125,7 @@ static void cancel_arg_helper(struct thread_master *master,
return;
/* First process the ready lists. */
- frr_each_safe(thread_list, &master->event, t) {
+ frr_each_safe (thread_list, &master->event, t) {
if (t->arg != cr->eventobj)
continue;
thread_list_del(&master->event, t);
@@ -1134,7 +1134,7 @@ static void cancel_arg_helper(struct thread_master *master,
thread_add_unuse(master, t);
}
- frr_each_safe(thread_list, &master->ready, t) {
+ frr_each_safe (thread_list, &master->ready, t) {
if (t->arg != cr->eventobj)
continue;
thread_list_del(&master->ready, t);
@@ -1144,7 +1144,7 @@ static void cancel_arg_helper(struct thread_master *master,
}
/* Check the io tasks */
- for (i = 0; i < master->handler.pfdcount; ) {
+ for (i = 0; i < master->handler.pfdcount;) {
pfd = master->handler.pfds + i;
if (pfd->events & POLLIN)
If you are a new contributor to FRR, please see our contributing guidelines.
After making changes, you do not need to create a new PR. You should perform an amend or interactive rebase followed by a force push.
Continuous Integration Result: FAILEDContinuous Integration Result: FAILEDSee below for issues. This is a comment from an automated CI system. Get source / Pull Request: SuccessfulBuilding Stage: SuccessfulBasic Tests: FailedTopo tests part 0 on Ubuntu 18.04 arm8: Failed (click for details)Topology Test Results are at https://ci1.netdef.org/browse/FRR-FRRPULLREQ-TOPO0U18ARM8-16736/test Topology Tests failed for Topo tests part 0 on Ubuntu 18.04 arm8: see full log at https://ci1.netdef.org/browse/FRR-FRRPULLREQ-16736/artifact/TOPO0U18ARM8/ErrorLog/log_topotests.txt Topo tests part 4 on Ubuntu 18.04 arm8: Failed (click for details)Topo tests part 4 on Ubuntu 18.04 arm8: No useful log foundSuccessful on other platforms/tests
Warnings Generated during build:Debian 10 amd64 build: Successful with additional warningsDebian Package lintian failed for Debian 10 amd64 build: |
Outdated results 💚Basic BGPD CI results: SUCCESS, 0 tests failed
For details, please contact louberger |
|
CI:rerun |
donaldsharp
reviewed
Jan 28, 2021
lib/thread.c
Outdated
| @@ -1214,6 +1286,10 @@ void thread_cancel_event(struct thread_master *master, void *arg) | |||
| { | |||
| assert(master->owner == pthread_self()); | |||
|
|
|||
| /* Only worth anything if caller supplies an arg. */ | |||
| if (arg == NULL) | |||
There was a problem hiding this comment.
should we log something here? This seems like a developer has made a mistake of some sort?
There was a problem hiding this comment.
I could assert I guess? a log seems like something that no one's really going to see - dunno. I was half-thinking that it was ok for it to be benign to call with a NULL - it might just mean that some datastruct had a NULL, and the cleanup code didn't have to test that cell before calling the 'cancel' api?
donaldsharp
reviewed
Jan 28, 2021
lib/thread.c
Outdated
| @@ -1046,21 +1046,29 @@ struct thread *funcname_thread_add_event(struct thread_master *m, | |||
| * - POLLIN | |||
| * - POLLOUT | |||
| */ | |||
| static void thread_cancel_rw(struct thread_master *master, int fd, short state) | |||
| static void thread_cancel_rw(struct thread_master *master, int fd, short state, | |||
| int hint) | |||
There was a problem hiding this comment.
how about "idx_hint"? it's an index into an array, not an actual fd
Continuous Integration Result: FAILEDContinuous Integration Result: FAILEDSee below for issues. This is a comment from an automated CI system. Get source / Pull Request: SuccessfulBuilding Stage: SuccessfulBasic Tests: FailedTopo tests part 4 on Ubuntu 18.04 arm8: Failed (click for details)Topo tests part 4 on Ubuntu 18.04 arm8: No useful log foundTopo tests part 0 on Ubuntu 18.04 arm8: Failed (click for details)Topology Test Results are at https://ci1.netdef.org/browse/FRR-FRRPULLREQ-TOPO0U18ARM8-16778/test Topology Tests failed for Topo tests part 0 on Ubuntu 18.04 arm8: see full log at https://ci1.netdef.org/browse/FRR-FRRPULLREQ-16778/artifact/TOPO0U18ARM8/ErrorLog/log_topotests.txt Successful on other platforms/tests
Warnings Generated during build:Debian 10 amd64 build: Successful with additional warningsDebian Package lintian failed for Debian 10 amd64 build: |
mjstapp
force-pushed
the
fix_cancel_event
branch
from
f11a5fa to
94f8e49
Compare
polychaeta
reviewed
Jan 28, 2021
There was a problem hiding this comment.
Thanks for your contribution to FRR!
Click for style suggestions
To apply these suggestions:
curl -s https://gist.githubusercontent.com/polychaeta/eff1c4daae1ad69862cac0f0fe90333f/raw/06ed7328505c7ba733cef45a4d3786164bb187bc/cr_7951_1611856202.diff | git apply
diff --git a/lib/thread.c b/lib/thread.c
index aa0068cd9..57da2a4e2 100644
--- a/lib/thread.c
+++ b/lib/thread.c
@@ -1125,7 +1125,7 @@ static void cancel_arg_helper(struct thread_master *master,
return;
/* First process the ready lists. */
- frr_each_safe(thread_list, &master->event, t) {
+ frr_each_safe (thread_list, &master->event, t) {
if (t->arg != cr->eventobj)
continue;
thread_list_del(&master->event, t);
@@ -1134,7 +1134,7 @@ static void cancel_arg_helper(struct thread_master *master,
thread_add_unuse(master, t);
}
- frr_each_safe(thread_list, &master->ready, t) {
+ frr_each_safe (thread_list, &master->ready, t) {
if (t->arg != cr->eventobj)
continue;
thread_list_del(&master->ready, t);
@@ -1144,7 +1144,7 @@ static void cancel_arg_helper(struct thread_master *master,
}
/* Check the io tasks */
- for (i = 0; i < master->handler.pfdcount; ) {
+ for (i = 0; i < master->handler.pfdcount;) {
pfd = master->handler.pfds + i;
if (pfd->events & POLLIN)
If you are a new contributor to FRR, please see our contributing guidelines.
After making changes, you do not need to create a new PR. You should perform an amend or interactive rebase followed by a force push.
Continuous Integration Result: FAILEDContinuous Integration Result: FAILEDSee below for issues. This is a comment from an automated CI system. Get source / Pull Request: SuccessfulBuilding Stage: SuccessfulBasic Tests: FailedTopo tests part 4 on Ubuntu 18.04 arm8: Failed (click for details)Topo tests part 4 on Ubuntu 18.04 arm8: No useful log foundSuccessful on other platforms/tests
Warnings Generated during build:Debian 10 amd64 build: Successful with additional warningsDebian Package lintian failed for Debian 10 amd64 build: |
Outdated results 💚Basic BGPD CI results: SUCCESS, 0 tests failed
For details, please contact louberger |
|
Rebasing to pick up CI fixup |
mjstapp
force-pushed
the
fix_cancel_event
branch
from
94f8e49 to
16f61c0
Compare
polychaeta
reviewed
Jan 29, 2021
There was a problem hiding this comment.
Thanks for your contribution to FRR!
Click for style suggestions
To apply these suggestions:
curl -s https://gist.githubusercontent.com/polychaeta/b50dbd4273d6141007c44b1e079b522a/raw/001853916438d4c2a5d419dcab4897150da373c7/cr_7951_1611953614.diff | git apply
diff --git a/lib/thread.c b/lib/thread.c
index 699ff0189..57da2a4e2 100644
--- a/lib/thread.c
+++ b/lib/thread.c
@@ -1125,7 +1125,7 @@ static void cancel_arg_helper(struct thread_master *master,
return;
/* First process the ready lists. */
- frr_each_safe(thread_list, &master->event, t) {
+ frr_each_safe (thread_list, &master->event, t) {
if (t->arg != cr->eventobj)
continue;
thread_list_del(&master->event, t);
@@ -1134,7 +1134,7 @@ static void cancel_arg_helper(struct thread_master *master,
thread_add_unuse(master, t);
}
- frr_each_safe(thread_list, &master->ready, t) {
+ frr_each_safe (thread_list, &master->ready, t) {
if (t->arg != cr->eventobj)
continue;
thread_list_del(&master->ready, t);
If you are a new contributor to FRR, please see our contributing guidelines.
After making changes, you do not need to create a new PR. You should perform an amend or interactive rebase followed by a force push.
Outdated results 🛑Basic BGPD CI results: FAILURE
For details, please contact louberger |
Continuous Integration Result: FAILEDContinuous Integration Result: FAILEDSee below for issues. This is a comment from an automated CI system. Get source / Pull Request: SuccessfulBuilding Stage: SuccessfulBasic Tests: FailedTopo tests part 4 on Ubuntu 18.04 arm8: Failed (click for details)Topo tests part 4 on Ubuntu 18.04 arm8: No useful log foundSuccessful on other platforms/tests
Warnings Generated during build:Debian 10 amd64 build: Successful with additional warningsDebian Package lintian failed for Debian 10 amd64 build: |
Outdated results 💚Basic BGPD CI results: SUCCESS, 0 tests failed
For details, please contact louberger |
|
CI:rerun |
Continuous Integration Result: FAILEDContinuous Integration Result: FAILEDSee below for issues. This is a comment from an automated CI system. Get source / Pull Request: SuccessfulBuilding Stage: SuccessfulBasic Tests: FailedTopo tests part 4 on Ubuntu 18.04 arm8: Failed (click for details)Topo tests part 4 on Ubuntu 18.04 arm8: No useful log foundSuccessful on other platforms/tests
Warnings Generated during build:Debian 10 amd64 build: Successful with additional warningsDebian Package lintian failed for Debian 10 amd64 build: |
|
CI:rerun |
Continuous Integration Result: FAILEDContinuous Integration Result: FAILEDSee below for issues. This is a comment from an automated CI system. Get source / Pull Request: SuccessfulBuilding Stage: SuccessfulBasic Tests: FailedTopo tests part 4 on Ubuntu 18.04 arm8: Failed (click for details)Topo tests part 4 on Ubuntu 18.04 arm8: No useful log foundSuccessful on other platforms/tests
Warnings Generated during build:Debian 10 amd64 build: Successful with additional warningsDebian Package lintian failed for Debian 10 amd64 build: |
|
CI:rerun |
Continuous Integration Result: FAILEDContinuous Integration Result: FAILEDSee below for issues. This is a comment from an automated CI system. Get source / Pull Request: SuccessfulBuilding Stage: SuccessfulBasic Tests: FailedTopo tests part 4 on Ubuntu 18.04 arm8: Failed (click for details)Topo tests part 4 on Ubuntu 18.04 arm8: No useful log foundSuccessful on other platforms/tests
Warnings Generated during build:Debian 10 amd64 build: Successful with additional warningsDebian Package lintian failed for Debian 10 amd64 build: |
mjstapp
force-pushed
the
fix_cancel_event
branch
from
16f61c0 to
bb3c6c9
Compare
polychaeta
reviewed
Feb 3, 2021
There was a problem hiding this comment.
Thanks for your contribution to FRR!
Click for style suggestions
To apply these suggestions:
curl -s https://gist.githubusercontent.com/polychaeta/cc0d9788f530f95e49eb1be083e9f407/raw/05be35dc1ce83bd88cf589d50c4658356effe426/cr_7951_1612373346.diff | git apply
diff --git a/bgpd/bgp_fsm.h b/bgpd/bgp_fsm.h
index bf4966c83..948bd12a8 100644
--- a/bgpd/bgp_fsm.h
+++ b/bgpd/bgp_fsm.h
@@ -31,7 +31,7 @@
#define BGP_TIMER_OFF(T) \
do { \
- THREAD_OFF((T)); \
+ THREAD_OFF((T)); \
} while (0)
#define BGP_EVENT_ADD(P, E) \
@@ -47,17 +47,18 @@
thread_cancel_event_ready(bm->master, (P)); \
} while (0)
-#define BGP_UPDATE_GROUP_TIMER_ON(T, F) \
- do { \
- if (BGP_SUPPRESS_FIB_ENABLED(peer->bgp) && \
- PEER_ROUTE_ADV_DELAY(peer)) \
- thread_add_timer_msec(bm->master, (F), peer, \
- (BGP_DEFAULT_UPDATE_ADVERTISEMENT_TIME * 1000),\
- (T)); \
- else \
- thread_add_timer_msec(bm->master, (F), peer, \
- 0, (T)); \
- } while (0) \
+#define BGP_UPDATE_GROUP_TIMER_ON(T, F) \
+ do { \
+ if (BGP_SUPPRESS_FIB_ENABLED(peer->bgp) \
+ && PEER_ROUTE_ADV_DELAY(peer)) \
+ thread_add_timer_msec( \
+ bm->master, (F), peer, \
+ (BGP_DEFAULT_UPDATE_ADVERTISEMENT_TIME \
+ * 1000), \
+ (T)); \
+ else \
+ thread_add_timer_msec(bm->master, (F), peer, 0, (T)); \
+ } while (0)
#define BGP_MSEC_JITTER 10
diff --git a/lib/thread.c b/lib/thread.c
index b74d0c4a6..b38ac1bd0 100644
--- a/lib/thread.c
+++ b/lib/thread.c
@@ -53,7 +53,7 @@ struct cancel_req {
};
/* Flags for task cancellation */
-#define THREAD_CANCEL_FLAG_READY 0x01
+#define THREAD_CANCEL_FLAG_READY 0x01
static int thread_timer_cmp(const struct thread *a, const struct thread *b)
{
@@ -1136,7 +1136,7 @@ static void cancel_arg_helper(struct thread_master *master,
return;
/* First process the ready lists. */
- frr_each_safe(thread_list, &master->event, t) {
+ frr_each_safe (thread_list, &master->event, t) {
if (t->arg != cr->eventobj)
continue;
thread_list_del(&master->event, t);
@@ -1145,7 +1145,7 @@ static void cancel_arg_helper(struct thread_master *master,
thread_add_unuse(master, t);
}
- frr_each_safe(thread_list, &master->ready, t) {
+ frr_each_safe (thread_list, &master->ready, t) {
if (t->arg != cr->eventobj)
continue;
thread_list_del(&master->ready, t);
@@ -1308,7 +1308,7 @@ static void cancel_event_helper(struct thread_master *m, void *arg, int flags)
cr->flags = flags;
- frr_with_mutex(&m->mtx) {
+ frr_with_mutex (&m->mtx) {
cr->eventobj = arg;
listnode_add(m->cancel_req, cr);
do_thread_cancel(m);
If you are a new contributor to FRR, please see our contributing guidelines.
After making changes, you do not need to create a new PR. You should perform an amend or interactive rebase followed by a force push.
mjstapp
changed the title
|
Revised to deal with bgp, who relies on the original behavior (and expects timers to left in place, for example). |
Outdated results 💚Basic BGPD CI results: SUCCESS, 0 tests failed
For details, please contact louberger |
Continuous Integration Result: FAILEDContinuous Integration Result: FAILEDSee below for issues. This is a comment from an automated CI system. Get source / Pull Request: SuccessfulBuilding Stage: SuccessfulBasic Tests: FailedTopo tests part 1 on Ubuntu 16.04 amd64: Failed (click for details)Topology Test Results are at https://ci1.netdef.org/browse/FRR-FRRPULLREQ-TP1U1604AMD64-16928/test Topology Tests failed for Topo tests part 1 on Ubuntu 16.04 amd64: see full log at https://ci1.netdef.org/browse/FRR-FRRPULLREQ-16928/artifact/TP1U1604AMD64/ErrorLog/log_topotests.txt Successful on other platforms/tests
Warnings Generated during build:Debian 10 amd64 build: Successful with additional warningsDebian Package lintian failed for Debian 10 amd64 build: |
|
CI:rerun |
Continuous Integration Result: SUCCESSFULContinuous Integration Result: SUCCESSFULCongratulations, this patch passed basic tests Tested-by: NetDEF / OpenSourceRouting.org CI System CI System Testrun URL: https://ci1.netdef.org/browse/FRR-FRRPULLREQ-16935/ This is a comment from an automated CI system. Warnings Generated during build:Debian 10 amd64 build: Successful with additional warningsDebian Package lintian failed for Debian 10 amd64 build: |
added 2 commits
February 9, 2021 12:10
No reason for the thread/task cancellation struct to be public: move it out of the header file. Also add a flags field. Signed-off-by: Mark Stapp <mjs@voltanet.io>
Extend the thread_cancel_event api so that it's more complete: look in all the lists of events, including io and timers, for matching tasks. Add a limited version of the api that only examines tasks in the event and ready queues. BGP appears to require the old behavior, so change its macro to use the more limited cancel api. Signed-off-by: Mark Stapp <mjs@voltanet.io>
mjstapp
force-pushed
the
fix_cancel_event
branch
from
bb3c6c9 to
a9318a3
Compare
polychaeta
reviewed
Feb 9, 2021
There was a problem hiding this comment.
Thanks for your contribution to FRR!
Click for style suggestions
To apply these suggestions:
curl -s https://gist.githubusercontent.com/polychaeta/5d484a06930bed5d10a3f629d0bf7d11/raw/05be35dc1ce83bd88cf589d50c4658356effe426/cr_7951_1612893730.diff | git apply
diff --git a/bgpd/bgp_fsm.h b/bgpd/bgp_fsm.h
index bf4966c83..948bd12a8 100644
--- a/bgpd/bgp_fsm.h
+++ b/bgpd/bgp_fsm.h
@@ -31,7 +31,7 @@
#define BGP_TIMER_OFF(T) \
do { \
- THREAD_OFF((T)); \
+ THREAD_OFF((T)); \
} while (0)
#define BGP_EVENT_ADD(P, E) \
@@ -47,17 +47,18 @@
thread_cancel_event_ready(bm->master, (P)); \
} while (0)
-#define BGP_UPDATE_GROUP_TIMER_ON(T, F) \
- do { \
- if (BGP_SUPPRESS_FIB_ENABLED(peer->bgp) && \
- PEER_ROUTE_ADV_DELAY(peer)) \
- thread_add_timer_msec(bm->master, (F), peer, \
- (BGP_DEFAULT_UPDATE_ADVERTISEMENT_TIME * 1000),\
- (T)); \
- else \
- thread_add_timer_msec(bm->master, (F), peer, \
- 0, (T)); \
- } while (0) \
+#define BGP_UPDATE_GROUP_TIMER_ON(T, F) \
+ do { \
+ if (BGP_SUPPRESS_FIB_ENABLED(peer->bgp) \
+ && PEER_ROUTE_ADV_DELAY(peer)) \
+ thread_add_timer_msec( \
+ bm->master, (F), peer, \
+ (BGP_DEFAULT_UPDATE_ADVERTISEMENT_TIME \
+ * 1000), \
+ (T)); \
+ else \
+ thread_add_timer_msec(bm->master, (F), peer, 0, (T)); \
+ } while (0)
#define BGP_MSEC_JITTER 10
diff --git a/lib/thread.c b/lib/thread.c
index b74d0c4a6..b38ac1bd0 100644
--- a/lib/thread.c
+++ b/lib/thread.c
@@ -53,7 +53,7 @@ struct cancel_req {
};
/* Flags for task cancellation */
-#define THREAD_CANCEL_FLAG_READY 0x01
+#define THREAD_CANCEL_FLAG_READY 0x01
static int thread_timer_cmp(const struct thread *a, const struct thread *b)
{
@@ -1136,7 +1136,7 @@ static void cancel_arg_helper(struct thread_master *master,
return;
/* First process the ready lists. */
- frr_each_safe(thread_list, &master->event, t) {
+ frr_each_safe (thread_list, &master->event, t) {
if (t->arg != cr->eventobj)
continue;
thread_list_del(&master->event, t);
@@ -1145,7 +1145,7 @@ static void cancel_arg_helper(struct thread_master *master,
thread_add_unuse(master, t);
}
- frr_each_safe(thread_list, &master->ready, t) {
+ frr_each_safe (thread_list, &master->ready, t) {
if (t->arg != cr->eventobj)
continue;
thread_list_del(&master->ready, t);
@@ -1308,7 +1308,7 @@ static void cancel_event_helper(struct thread_master *m, void *arg, int flags)
cr->flags = flags;
- frr_with_mutex(&m->mtx) {
+ frr_with_mutex (&m->mtx) {
cr->eventobj = arg;
listnode_add(m->cancel_req, cr);
do_thread_cancel(m);
If you are a new contributor to FRR, please see our contributing guidelines.
After making changes, you do not need to create a new PR. You should perform an amend or interactive rebase followed by a force push.
|
rebased to newer master |
💚 Basic BGPD CI results: SUCCESS, 0 tests failedResults table
For details, please contact louberger |
Continuous Integration Result: FAILEDContinuous Integration Result: FAILEDSee below for issues. This is a comment from an automated CI system. Get source / Pull Request: SuccessfulBuilding Stage: SuccessfulBasic Tests: FailedTopotests Ubuntu 16.04 amd64 part 3: Failed (click for details)Topology Test Results are at https://ci1.netdef.org/browse/FRR-FRRPULLREQ-TP3U1604AMD64-17026/test Topology Tests failed for Topotests Ubuntu 16.04 amd64 part 3: see full log at https://ci1.netdef.org/browse/FRR-FRRPULLREQ-17026/artifact/TP3U1604AMD64/ErrorLog/log_topotests.txt Successful on other platforms/tests
Warnings Generated during build:Debian 10 amd64 build: Successful with additional warningsDebian Package lintian failed for Debian 10 amd64 build: |
|
CI:rerun |
Continuous Integration Result: SUCCESSFULCongratulations, this patch passed basic tests Tested-by: NetDEF / OpenSourceRouting.org CI System CI System Testrun URL: https://ci1.netdef.org/browse/FRR-FRRPULLREQ-17031/ This is a comment from an automated CI system. Warnings Generated during build:Debian 10 amd64 build: Successful with additional warningsDebian Package lintian failed for Debian 10 amd64 build: |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Extend the thread_cancel_event api so that it's more complete: look in all the lists of tasks, including io and timers, for matching tasks to cancel. Add a dedicated api that does the more limited original behavior, examining only the ready and events queues. BGP appears to require this more-limited version, so use it via the bgp macro.