From bc3de17c3b959fcc6a5ba42f5d3b09402bf89687 Mon Sep 17 00:00:00 2001 From: "E. Scott Daniels" Date: Thu, 23 Jan 2020 16:20:12 -0500 Subject: [PATCH] Correct bug in SIwait causing excessive CPU use The select timer in SIwait was not being reset correctly and after a few loops the timer was set such that the thread was using 100% of a CPU. Also eliminated the build poll list from cache as this turned out to be slower than the original method. Signed-off-by: E. Scott Daniels Change-Id: Ic606f19aaf522ff7c7146a6fc62c76ec9f8de3a9 --- CHANGES | 3 ++ CMakeLists.txt | 2 +- src/rmr/si/src/si95/sibldpoll.c | 65 +++++++++++++++-------------------------- src/rmr/si/src/si95/siconst.h | 1 - src/rmr/si/src/si95/sinewses.c | 1 - src/rmr/si/src/si95/sistruct.h | 3 -- src/rmr/si/src/si95/siwait.c | 19 +++++++----- 7 files changed, 39 insertions(+), 55 deletions(-) diff --git a/CHANGES b/CHANGES index f5d970d..f3d5c77 100644 --- a/CHANGES +++ b/CHANGES @@ -2,6 +2,9 @@ API and build change and fix summaries. Doc correctsions and/or changes are not mentioned here; see the commit messages. +2020 January 23; verison 3.0.4 + Fix bug in SI95 causing excessive CPU usage on poll. + 2020 January 22; verison 3.0.3 Enable thread support for multiple receive threads. diff --git a/CMakeLists.txt b/CMakeLists.txt index 2714703..89f5cb9 100644 --- a/CMakeLists.txt +++ b/CMakeLists.txt @@ -38,7 +38,7 @@ cmake_minimum_required( VERSION 3.5 ) set( major_version "3" ) # should be automatically populated from git tag later, but until CI process sets a tag we use this set( minor_version "0" ) -set( patch_level "3" ) +set( patch_level "4" ) set( install_root "${CMAKE_INSTALL_PREFIX}" ) set( install_inc "include/rmr" ) diff --git a/src/rmr/si/src/si95/sibldpoll.c b/src/rmr/si/src/si95/sibldpoll.c index fda0921..6ad26ea 100644 --- a/src/rmr/si/src/si95/sibldpoll.c +++ b/src/rmr/si/src/si95/sibldpoll.c @@ -27,6 +27,7 @@ * be added to the write fdset. The fdcount variable will be set to * the highest sid + 1 and it can be passed to the select system * call when it is made. +* * Parms: gptr - Pointer to the general info structure * Returns: Nothing * Date: 26 March 1995 @@ -42,50 +43,32 @@ extern void SIbldpoll( struct ginfo_blk* gptr ) { struct tp_blk *nextb; // pointer into tp list -// FIX ME? we don't seem to see this flag set - //if( gptr->flags & GIF_SESS_CHANGE ) { // session changed, must rebuild the poll lists - gptr->fdcount = -1; // reset largest sid found + gptr->fdcount = -1; // reset largest sid found + + FD_ZERO( &gptr->readfds ); // reset the read and write sets + FD_ZERO( &gptr->writefds ); + FD_ZERO( &gptr->execpfds ); - FD_ZERO( &gptr->readfds ); // reset the read and write sets - FD_ZERO( &gptr->writefds ); - FD_ZERO( &gptr->execpfds ); - - for( tpptr = gptr->tplist; tpptr != NULL; tpptr = nextb ) { - nextb = tpptr->next; - if( tpptr->flags & TPF_DELETE ) { - SIterm( gptr, tpptr ); - } else { - if( tpptr->fd >= 0 ) { // if valid file descriptor - if( tpptr->fd >= gptr->fdcount ) { - gptr->fdcount = tpptr->fd + 1; // save largest fd (+1) for select - } - - FD_SET( tpptr->fd, &gptr->execpfds ); // set all fds for execpts - - if( !(tpptr->flags & TPF_DRAIN) ) { // if not draining - FD_SET( tpptr->fd, &gptr->readfds ); // set test for data flag - } - - if( tpptr->squeue != NULL ) { // stuff pending to send ? - FD_SET( tpptr->fd, &gptr->writefds ); // set flag to see if writable - } + for( tpptr = gptr->tplist; tpptr != NULL; tpptr = nextb ) { + nextb = tpptr->next; + if( tpptr->flags & TPF_DELETE ) { + SIterm( gptr, tpptr ); + } else { + if( tpptr->fd >= 0 ) { // if valid file descriptor + if( tpptr->fd >= gptr->fdcount ) { + gptr->fdcount = tpptr->fd + 1; // save largest fd (+1) for select } - } -/* - memcpy( &gptr->readfds_qs, &gptr->readfds, sizeof( fd_set ) ); // stash for use until change - memcpy( &gptr->writefds_qs, &gptr->writefds, sizeof( fd_set ) ); - memcpy( &gptr->execpfds_qs, &gptr->execpfds, sizeof( fd_set ) ); -*/ + FD_SET( tpptr->fd, &gptr->execpfds ); // set all fds for execpts + + if( !(tpptr->flags & TPF_DRAIN) ) { // if not draining + FD_SET( tpptr->fd, &gptr->readfds ); // set test for data flag + } - gptr->flags &= ~GIF_SESS_CHANGE; + if( tpptr->squeue != NULL ) { // stuff pending to send ? + FD_SET( tpptr->fd, &gptr->writefds ); // set flag to see if writable + } + } } -/* - } else { // sessions are the same we can just dup the quick sets we saved - memcpy( &gptr->readfds, &gptr->readfds_qs, sizeof( fd_set ) ); - memcpy( &gptr->writefds, &gptr->writefds_qs, sizeof( fd_set ) ); - memcpy( &gptr->execpfds, &gptr->execpfds_qs, sizeof( fd_set ) ); } -*/ - -} // SIbldpoll +} diff --git a/src/rmr/si/src/si95/siconst.h b/src/rmr/si/src/si95/siconst.h index 9bd32c8..e5ec5c0 100644 --- a/src/rmr/si/src/si95/siconst.h +++ b/src/rmr/si/src/si95/siconst.h @@ -44,7 +44,6 @@ // general info block flags #define GIF_SHUTDOWN 0x01 // shutdown in progress #define GIF_NODELAY 0x02 // set no delay flag on t_opens -#define GIF_SESS_CHANGE 0x04 // session list has changed; new poll list needed // transmission provider block flags #define TPF_LISTENFD 0x01 // set on tp blk that is fd for tcp listens diff --git a/src/rmr/si/src/si95/sinewses.c b/src/rmr/si/src/si95/sinewses.c index 0b00ba4..182fa6a 100644 --- a/src/rmr/si/src/si95/sinewses.c +++ b/src/rmr/si/src/si95/sinewses.c @@ -114,7 +114,6 @@ extern int SInewsession( struct ginfo_blk *gptr, struct tp_blk *tpptr ) { SIcbstat( gptr, status, SI_CB_CONN ); // handle status } - gptr->flags |= GIF_SESS_CHANGE; // sessions changed must rebuild the poll list SImap_fd( gptr, newtp->fd, newtp ); // add fd to the map return SI_OK; diff --git a/src/rmr/si/src/si95/sistruct.h b/src/rmr/si/src/si95/sistruct.h index c394afb..0713395 100644 --- a/src/rmr/si/src/si95/sistruct.h +++ b/src/rmr/si/src/si95/sistruct.h @@ -71,9 +71,6 @@ struct ginfo_blk { // general info block (context) fd_set readfds; // select/poll file des lists fd_set writefds; fd_set execpfds; - fd_set readfds_qs; // quick set read/write/except fd sets - fd_set writefds_qs; - fd_set execpfds_qs; char *rbuf; // read buffer struct callback_blk *cbtab; // pointer at the callback table int fdcount; // largest fd to select on in siwait diff --git a/src/rmr/si/src/si95/siwait.c b/src/rmr/si/src/si95/siwait.c index d45efa6..75de117 100644 --- a/src/rmr/si/src/si95/siwait.c +++ b/src/rmr/si/src/si95/siwait.c @@ -49,6 +49,12 @@ #include "sitransport.h" #include +/* + The select timeout is about 300 mu-sec. This is fast enough to add a new + outbound connection to the poll list before the other side responds, + but slow enough so as not to consume excess CPU when idle. +*/ +#define SI_SELECT_TIMEOUT 300000 extern int SIwait( struct ginfo_blk *gptr ) { int fd; // file descriptor for use in this routine @@ -68,23 +74,21 @@ extern int SIwait( struct ginfo_blk *gptr ) { gptr->sierr = SI_ERR_SHUTD; if( gptr->flags & GIF_SHUTDOWN ) { // cannot do if we should shutdown - fprintf( stderr, ">>> wait: shutdown on entry????\n" ); return SI_ERROR; // so just get out } gptr->sierr = SI_ERR_HANDLE; if( gptr->magicnum != MAGICNUM ) { // if not a valid ginfo block - fprintf( stderr, ">>> wait: bad magic on entry????\n" ); + fprintf( stderr, "[CRI] SI95: wait: bad global info struct magic number is wrong\n" ); return SI_ERROR; } - timeout.tv_sec = 0; - timeout.tv_usec = 500000; // pop every 500ms to ensure we pick up new outbound connections in list - - do { // main wait/process loop + do { // spin until a callback says to stop (likely never) + timeout.tv_sec = 0; // must be reset on every call! + timeout.tv_usec = SI_SELECT_TIMEOUT; - SIbldpoll( gptr ); // build the fdlist for poll + SIbldpoll( gptr ); // poll list is trashed on each pop; must rebuild pstat = select( gptr->fdcount, &gptr->readfds, &gptr->writefds, &gptr->execpfds, &timeout ); if( (pstat < 0 && errno != EINTR) ) { @@ -113,7 +117,6 @@ extern int SIwait( struct ginfo_blk *gptr ) { status = SInewsession( gptr, tpptr ); // accept connection } else { // data received on a regular port (we support just tcp now status = RECV( fd, gptr->rbuf, MAX_RBUF, 0 ); // read data - //fprintf( stderr, ">>>>> wait popped status =%d\n", status ); if( status > 0 && ! (tpptr->flags & TPF_DRAIN) ) { if( (cbptr = gptr->cbtab[SI_CB_CDATA].cbrtn) != NULL ) { status = (*cbptr)( gptr->cbtab[SI_CB_CDATA].cbdata, fd, gptr->rbuf, status ); -- 2.16.6