-
Notifications
You must be signed in to change notification settings - Fork 1.3k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
FRR pthread improvements #1672
FRR pthread improvements #1672
Conversation
Some work on FRR's pthread wrapper. * Provide a built-in way to synchronize thread startup * Make utility functions take frr_pthread * instead of its integer ID * Pass frr_pthread * as pthread start function argument * Correct some comment styling * Rename some variables to match naming conventions in the file * Change parameter ordering in stop function prototype to follow the convention in the other functions * Default new frr_pthreads to using a vanilla event loop For the last point, the original goal when designing the implementation of pthreads into FRR was to be able to use the thread.c event based system inside pthreads. This code essentially encapuslates all the thread.c functionality into an easy to use pthread out of the box. Creating a new frr_pthread with a null attributes field will cause the created frr_pthread to run a thread.c event loop. The upshot of this is that it is now possible to safely run existing functions in a pthread in roughly 3 lines of code. It also serves as an example / starting point for others. Signed-off-by: Quentin Young <qlyoung@cumulusnetworks.com>
Use the new threading facilities provided in lib/ to streamline the threads used in bgpd. In particular, all of the lifecycle code has been removed from the I/O thread and replaced with the default loop. Did not do the same to the keepalives thread as it is much smaller (doesn't need the event system). Also cleaned up some comments to match the style guide. Signed-off-by: Quentin Young <qlyoung@cumulusnetworks.com>
df9b438
to
a715eab
Compare
🛑 Basic BGPD CI results: FAILUREResults table
For details, please contact louberger |
Continuous Integration Result: FAILEDSee below for issues. This is a comment from an EXPERIMENTAL automated CI system. Get source and apply patch from patchwork: SuccessfulBuilding Stage: FailedOmniOS amd64 build: Successful NetBSD6 amd64 build: FailedDejaGNU Unittests (make check) failed for NetBSD6 amd64 build: (see full PyTest log at https://ci1.netdef.org/browse/FRR-FRRPULLREQ-2399/artifact/CI007BUILD/ErrorLog/log_pytests.txt)
NetBSD6 amd64 build: config.status output from configure script can be found at https://ci1.netdef.org/browse/FRR-FRRPULLREQ-2399/artifact/CI007BUILD/config.status/config.status CentOS7 amd64 build: FailedDejaGNU Unittests (make check) failed for CentOS7 amd64 build Debian8 amd64 build: FailedDejaGNU Unittests (make check) failed for Debian8 amd64 build FreeBSD10 amd64 build: FailedDejaGNU Unittests (make check) failed for FreeBSD10 amd64 build Ubuntu1404 amd64 build: FailedDejaGNU Unittests (make check) failed for Ubuntu1404 amd64 build OpenBSD60 amd64 build: FailedDejaGNU Unittests (make check) failed for OpenBSD60 amd64 build NetBSD7 amd64 build: FailedDejaGNU Unittests (make check) failed for NetBSD7 amd64 build Ubuntu1604 amd64 build: FailedDejaGNU Unittests (make check) failed for Ubuntu1604 amd64 build Fedora24 amd64 build: FailedDejaGNU Unittests (make check) failed for Fedora24 amd64 build FreeBSD11 amd64 build: FailedDejaGNU Unittests (make check) failed for FreeBSD11 amd64 build CentOS6 amd64 build: FailedDejaGNU Unittests (make check) failed for CentOS6 amd64 build FreeBSD9 amd64 build: FailedDejaGNU Unittests (make check) failed for FreeBSD9 amd64 build Debian9 amd64 build: FailedDebian9 amd64 build: config.status output from configure script can be found at https://ci1.netdef.org/browse/FRR-FRRPULLREQ-2399/artifact/CI021BUILD/config.status/config.status Ubuntu1204 amd64 build: FailedDejaGNU Unittests (make check) failed for Ubuntu1204 amd64 build |
🛑 Basic BGPD CI results: FAILUREResults table
For details, please contact louberger |
Continuous Integration Result: FAILEDSee below for issues. This is a comment from an EXPERIMENTAL automated CI system. Get source and apply patch from patchwork: SuccessfulBuilding Stage: FailedOmniOS amd64 build: Successful NetBSD6 amd64 build: FailedDejaGNU Unittests (make check) failed for NetBSD6 amd64 build: (see full PyTest log at https://ci1.netdef.org/browse/FRR-FRRPULLREQ-2400/artifact/CI007BUILD/ErrorLog/log_pytests.txt)
NetBSD6 amd64 build: config.status output from configure script can be found at https://ci1.netdef.org/browse/FRR-FRRPULLREQ-2400/artifact/CI007BUILD/config.status/config.status CentOS7 amd64 build: FailedDejaGNU Unittests (make check) failed for CentOS7 amd64 build Debian8 amd64 build: FailedDejaGNU Unittests (make check) failed for Debian8 amd64 build FreeBSD10 amd64 build: FailedDejaGNU Unittests (make check) failed for FreeBSD10 amd64 build Ubuntu1404 amd64 build: FailedDejaGNU Unittests (make check) failed for Ubuntu1404 amd64 build OpenBSD60 amd64 build: FailedDejaGNU Unittests (make check) failed for OpenBSD60 amd64 build NetBSD7 amd64 build: FailedDejaGNU Unittests (make check) failed for NetBSD7 amd64 build Ubuntu1604 amd64 build: FailedDejaGNU Unittests (make check) failed for Ubuntu1604 amd64 build Fedora24 amd64 build: FailedDejaGNU Unittests (make check) failed for Fedora24 amd64 build FreeBSD11 amd64 build: FailedDejaGNU Unittests (make check) failed for FreeBSD11 amd64 build CentOS6 amd64 build: FailedDejaGNU Unittests (make check) failed for CentOS6 amd64 build FreeBSD9 amd64 build: FailedDejaGNU Unittests (make check) failed for FreeBSD9 amd64 build Debian9 amd64 build: FailedDebian9 amd64 build: config.status output from configure script can be found at https://ci1.netdef.org/browse/FRR-FRRPULLREQ-2400/artifact/CI021BUILD/config.status/config.status Ubuntu1204 amd64 build: FailedDejaGNU Unittests (make check) failed for Ubuntu1204 amd64 build |
Signed-off-by: Quentin Young <qlyoung@cumulusnetworks.com>
Whoopsie, |
Continuous Integration Result: FAILEDSee below for issues. This is a comment from an EXPERIMENTAL automated CI system. Get source and apply patch from patchwork: SuccessfulBuilding Stage: SuccessfulBasic Tests: FailedDebian 9 deb pkg check: Successful Topology tests on Ubuntu 16.04 amd64: FailedTopology Test Results are at https://ci1.netdef.org/browse/FRR-FRRPULLREQ-TOPOU1604-2401/test Topology Tests failed for Topology tests on Ubuntu 16.04 amd64:
see full log at https://ci1.netdef.org/browse/FRR-FRRPULLREQ-2401/artifact/TOPOU1604/ErrorLog/log_topotests.txt Topology Tests memory analysis: https://ci1.netdef.org/browse/FRR-FRRPULLREQ-2401/artifact/TOPOU1604/MemoryLeaks/CLANG Static Analyzer Summary
No Changes in Static Analysis warnings compared to base21 Static Analyzer issues remaining.See details at |
seeing the following on startup: |
@qlyoung sorry about the close, hit wrong button! |
If a peer already has keepalives turned on when asking to turn them on, return immediately. Same thing for turning them off. Signed-off-by: Quentin Young <qlyoung@cumulusnetworks.com>
💚 Basic BGPD CI results: SUCCESS, 0 tests failedResults table
For details, please contact louberger |
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-2403/ This is a comment from an EXPERIMENTAL automated CI system. CLANG Static Analyzer Summary
No Changes in Static Analysis warnings compared to base21 Static Analyzer issues remaining.See details at |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
These changes look good to me, but it would be good to get a second set of eyes on them before merging.
Some work on FRR's pthread wrapper.
frr_pthread *
instead of its integer IDfrr_pthread *
as pthread start function argumentconvention in the other functions
frr_pthread
s to using a vanilla event loopFor the last point, the original goal when designing the implementation of
pthreads into FRR was to be able to use the
thread.c
event based system insidepthreads. This code essentially encapuslates all the thread.c functionality
into an easy to use pthread out of the box. Creating a new
frr_pthread
with anull attributes field will cause the created
frr_pthread
to run athread.c
event loop. The upshot of this is that it is now possible to safely run
existing functions in a pthread in roughly 3 lines of code. It also serves as
an example / starting point for others.
In line with this change I also updated bgpd to make use of the new threading
facilities. In particular, all of the lifecycle code has been removed from the
I/O thread and replaced with the default loop. Did not do the same to the
keepalives thread as it is much smaller (doesn't need the event system).