Correct bug in SIwait causing excessive CPU use 25/2325/2
authorE. Scott Daniels <daniels@research.att.com>
Thu, 23 Jan 2020 21:20:12 +0000 (16:20 -0500)
committerE. Scott Daniels <daniels@research.att.com>
Thu, 23 Jan 2020 21:36:12 +0000 (16:36 -0500)
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 <daniels@research.att.com>
Change-Id: Ic606f19aaf522ff7c7146a6fc62c76ec9f8de3a9

CHANGES
CMakeLists.txt
src/rmr/si/src/si95/sibldpoll.c
src/rmr/si/src/si95/siconst.h
src/rmr/si/src/si95/sinewses.c
src/rmr/si/src/si95/sistruct.h
src/rmr/si/src/si95/siwait.c

diff --git a/CHANGES b/CHANGES
index f5d970d..f3d5c77 100644 (file)
--- 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.
 
index 2714703..89f5cb9 100644 (file)
@@ -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" )
index fda0921..6ad26ea 100644 (file)
@@ -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 
+}
index 9bd32c8..e5ec5c0 100644 (file)
@@ -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 
index 0b00ba4..182fa6a 100644 (file)
@@ -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;
index c394afb..0713395 100644 (file)
@@ -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 
index d45efa6..75de117 100644 (file)
 #include "sitransport.h"
 #include       <sys/wait.h>
 
+/*
+       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 );