100% CPU usage on Linux / need proper Sleep

[mrwonko]

there's also the issue of linux using 100% cpu see ioq3/qcommon/common.c NET_Sleep call we'll need to adjust our code to match that client-only, not dedi linux, i assume, but when we remove win32 specific backend it may affect windows the idea is to sleep inside of select(), i believe currently there is no sleep on linux client

~@Razish

[ensiform]

This ties in with unified sdl and the rest of system and such so I don't see why a separate issue is necessary.

[ensiform]

It also ties into changes to UDP downloads too.

Plus there's the bit about avoiding busy wait in newer ioq3 as well.

[dionrhys]

Networking is outside of the scope of SDL, but due to popular demand a simple cross-platform sockets wrapper called "SDL_net" is available at the SDL libraries page. http://wiki.libsdl.org/FAQDevelopment#Does_SDL_support_networking.3F

Will unified SDL mean adding SDL_net as an additional library dependency?

[ensiform]

No. Just it ties in to some code with how packets are handled due to NET sleep and NET event in ioq3.

[ensiform]

Because on windows it will no longer rely on win messages etc.

[ensiform]

It just means we are throwing out the win files for sys sdl and con_ files.

The Unix net and Win net were already merged to qcommon/net_ip.

And dedicated bins won't link sdl at all fwiw.

[mrwonko]

This ties in with unified sdl and the rest of system and such so I don't see why a separate issue is necessary.

Yeah, could've probably done a comment on that issue. I just wanted to note this down.

But how does this tie into SDL? We can't use SDL's sleep if this is needed in Dedicated...

[ensiform]

Its not using SDL sleep but it REQUIRES the different functions in NET_Sleep/Net_Event in net_ip when you switch to SDL because it no longer can use peek windows messages functions to prevent 100% cpu.

The sys/com event system etc.

[Razish]

Preliminary diff (thanks ioq3 + Ensiform) for Linux people having issues here, but it hangs on Windows, so it can't be merged into master yet. Feel free to contribute a fix!

diff --git a/codemp/qcommon/common.cpp b/codemp/qcommon/common.cpp
index 61f7c98..7c74293 100644
--- a/codemp/qcommon/common.cpp
+++ b/codemp/qcommon/common.cpp
@@ -18,7 +18,6 @@ cvar_t    *com_developer;
 cvar_t *com_dedicated;
 cvar_t *com_timescale;
 cvar_t *com_fixedtime;
-cvar_t *com_dropsim;       // 0.0 to 1.0, simulated packet drops
 cvar_t *com_journal;
 cvar_t *com_maxfps;
 cvar_t *com_timedemo;
@@ -42,6 +41,7 @@ cvar_t    *com_cameraMode;
 cvar_t *com_unfocused;
 cvar_t *com_minimized;
 cvar_t  *com_homepath;
+cvar_t *com_busyWait;

 cvar_t *com_affinity;

@@ -897,6 +897,7 @@ int Com_EventLoop( void ) {
            }
            Cbuf_AddText( "\n" );
            break;
+#if 0
        case SE_PACKET:
            // this cvar allows simulation of connections that
            // drop a lot of packets.  Note that loopback connections
@@ -927,6 +928,7 @@ int Com_EventLoop( void ) {
                CL_PacketEvent( evFrom, &buf );
            }
            break;
+#endif
        }

        // free any block data
@@ -1226,7 +1228,6 @@ void Com_Init( char *commandLine ) {
        com_fixedtime = Cvar_Get ("fixedtime", "0", CVAR_CHEAT);
        com_showtrace = Cvar_Get ("com_showtrace", "0", CVAR_CHEAT);

-       com_dropsim = Cvar_Get ("com_dropsim", "0", CVAR_CHEAT);
        com_viewlog = Cvar_Get( "viewlog", "0", 0 );
        com_speeds = Cvar_Get ("com_speeds", "0", 0);
        com_timedemo = Cvar_Get ("timedemo", "0", 0);
@@ -1247,6 +1248,8 @@ void Com_Init( char *commandLine ) {
        com_G2Report = Cvar_Get("com_G2Report", "0", 0);
 #endif

+       com_busyWait = Cvar_Get( "com_busyWait", "0", CVAR_ARCHIVE );
+
        com_affinity = Cvar_Get( "com_affinity", "1", CVAR_ARCHIVE );

        com_bootlogo = Cvar_Get( "com_bootlogo", "1", CVAR_ARCHIVE);
@@ -1440,6 +1443,26 @@ int Com_ModifyMsec( int msec ) {
    return msec;
 }

+/*
+=================
+Com_TimeVal
+=================
+*/
+
+int Com_TimeVal( int minMsec )
+{
+   int timeVal;
+
+   timeVal = Sys_Milliseconds() - com_frameTime;
+
+   if ( timeVal >= minMsec )
+       timeVal = 0;
+   else
+       timeVal = minMsec - timeVal;
+
+   return timeVal;
+}
+
 #ifdef G2_PERFORMANCE_ANALYSIS
 #include "qcommon/timing.h"
 void G2Time_ResetTimers(void);
@@ -1461,22 +1484,14 @@ void Com_Frame( void ) {
        G2PerformanceTimer_PreciseFrame.Start();
 #endif
        int     msec, minMsec;
-       static int  lastTime = 0;
-
-       int     timeBeforeFirstEvents;
-       int           timeBeforeServer;
-       int           timeBeforeEvents;
-       int           timeBeforeClient;
-       int           timeAfter;
-
+       int     timeVal;// , timeValSV;
+       static int  lastTime = 0, bias = 0;

-       // bk001204 - init to zero.
-       //  also:  might be clobbered by `longjmp' or `vfork'
-       timeBeforeFirstEvents =0;
-       timeBeforeServer =0;
-       timeBeforeEvents =0;
-       timeBeforeClient = 0;
-       timeAfter = 0;
+       int     timeBeforeFirstEvents = 0;
+       int           timeBeforeServer = 0;
+       int           timeBeforeEvents = 0;
+       int           timeBeforeClient = 0;
+       int           timeAfter = 0;

        // write config file if anything changed
        Com_WriteConfiguration();
@@ -1496,6 +1511,67 @@ void Com_Frame( void ) {
            timeBeforeFirstEvents = Sys_Milliseconds ();
        }

+       // Figure out how much time we have
+       if ( !com_timedemo->integer )
+       {
+           if ( com_dedicated->integer )
+               minMsec = SV_FrameMsec();
+           else
+           {
+               //if ( com_minimized->integer && com_maxfpsMinimized->integer > 0 )
+               //  minMsec = 1000 / com_maxfpsMinimized->integer;
+               //else if ( com_unfocused->integer && com_maxfpsUnfocused->integer > 0 )
+               //  minMsec = 1000 / com_maxfpsUnfocused->integer;
+               /*else*/ if ( com_maxfps->integer > 0 )
+                   minMsec = 1000 / com_maxfps->integer;
+               else
+                   minMsec = 1;
+
+               timeVal = com_frameTime - lastTime;
+               bias += timeVal - minMsec;
+
+               if ( bias > minMsec )
+                   bias = minMsec;
+
+               // Adjust minMsec if previous frame took too long to render so
+               // that framerate is stable at the requested value.
+               minMsec -= bias;
+           }
+       }
+       else
+           minMsec = 1;
+
+       do
+       {
+           if ( com_sv_running->integer )
+           {
+               //timeValSV = SV_SendQueuedPackets();
+
+               timeVal = Com_TimeVal( minMsec );
+
+               //if ( timeValSV < timeVal )
+               //  timeVal = timeValSV;
+           }
+           else
+               timeVal = Com_TimeVal( minMsec );
+
+           if ( com_busyWait->integer || timeVal < 1 )
+               NET_Sleep( 0 );
+           else
+               NET_Sleep( timeVal - 1 );
+       } while ( Com_TimeVal( minMsec ) );
+
+       lastTime = com_frameTime;
+       com_frameTime = Com_EventLoop();
+
+       msec = com_frameTime - lastTime;
+
+       Cbuf_Execute();
+
+       // mess with msec if needed
+       msec = Com_ModifyMsec( msec );
+
+#if 0
        // we may want to spin here if things are going too fast
        if ( !com_dedicated->integer && com_maxfps->integer > 0 && !com_timedemo->integer ) {
            minMsec = 1000 / com_maxfps->integer;
@@ -1516,6 +1592,7 @@ void Com_Frame( void ) {
        // mess with msec if needed
        com_frameMsec = msec;
        msec = Com_ModifyMsec( msec );
+#endif

        //
        // server side
diff --git a/codemp/qcommon/net_ip.cpp b/codemp/qcommon/net_ip.cpp
index 77b68a5..1a7fb83 100644
--- a/codemp/qcommon/net_ip.cpp
+++ b/codemp/qcommon/net_ip.cpp
@@ -71,6 +71,8 @@ static cvar_t *net_socksPassword;
 static cvar_t  *net_ip;
 static cvar_t  *net_port;

+static cvar_t  *net_dropsim;
+
 static struct sockaddr socksRelayAddr;

 static SOCKET  ip_socket = INVALID_SOCKET;
@@ -878,6 +880,8 @@ static qboolean NET_GetCvars( void ) {
    modified += net_socksPassword->modified;
    net_socksPassword->modified = qfalse;

+   net_dropsim = Cvar_Get( "net_dropsim", "", CVAR_TEMP );
+
    return modified ? qtrue : qfalse;
 }

@@ -982,6 +986,90 @@ void NET_Shutdown( void ) {

 /*
 ====================
+NET_Event
+
+Called from NET_Sleep which uses select() to determine which sockets have seen action.
+====================
+*/
+
+void NET_Event( fd_set *fdr )
+{
+   byte bufData[MAX_MSGLEN + 1];
+   netadr_t from;
+   msg_t netmsg;
+
+   while ( 1 )
+   {
+       MSG_Init( &netmsg, bufData, sizeof(bufData) );
+
+       if ( Sys_GetPacket( &from, &netmsg ) )
+       {
+           if ( net_dropsim->value > 0.0f && net_dropsim->value <= 100.0f )
+           {
+               // com_dropsim->value percent of incoming packets get dropped.
+               if ( rand() < (int)(((double)RAND_MAX) / 100.0 * (double)net_dropsim->value) )
+                   continue;          // drop this packet
+           }
+
+           if ( com_sv_running->integer )
+               Com_RunAndTimeServerPacket( &from, &netmsg );
+           else
+               CL_PacketEvent( from, &netmsg );
+       }
+       else
+           break;
+   }
+}
+
+/*
+====================
+NET_Sleep
+
+Sleeps msec or until something happens on the network
+====================
+*/
+void NET_Sleep( int msec )
+{
+   struct timeval timeout;
+   fd_set fdr;
+   int retval;
+   SOCKET highestfd = INVALID_SOCKET;
+
+   if ( msec < 0 )
+       msec = 0;
+
+   FD_ZERO( &fdr );
+
+   if ( ip_socket != INVALID_SOCKET )
+   {
+       FD_SET( ip_socket, &fdr );
+
+       highestfd = ip_socket;
+   }
+
+#ifdef _WIN32
+   if ( highestfd == INVALID_SOCKET )
+   {
+       // windows ain't happy when select is called without valid FDs
+       SleepEx( msec, 0 );
+       return;
+   }
+#endif
+
+   timeout.tv_sec = msec / 1000;
+   timeout.tv_usec = (msec % 1000) * 1000;
+
+   retval = select( highestfd + 1, &fdr, NULL, NULL, &timeout );
+
+   if ( retval == SOCKET_ERROR )
+       Com_Printf( "Warning: select() syscall failed: %s\n", NET_ErrorString() );
+   else if ( retval > 0 )
+       NET_Event( &fdr );
+}
+
+#if 0
+/*
+====================
 NET_Sleep

 sleeps msec or until net socket is ready
@@ -1008,6 +1096,7 @@ void NET_Sleep( int msec ) {
    select(ip_socket+1, &fdset, NULL, NULL, &timeout);
 #endif
 }
+#endif

 /*
 ====================
diff --git a/codemp/qcommon/qcommon.h b/codemp/qcommon/qcommon.h
index 71d214b..290a5c6 100644
--- a/codemp/qcommon/qcommon.h
+++ b/codemp/qcommon/qcommon.h
@@ -732,6 +732,7 @@ int         Com_Filter(char *filter, char *name, int casesensitive);
 int            Com_FilterPath(char *filter, char *name, int casesensitive);
 int            Com_RealTime(qtime_t *qtime);
 qboolean   Com_SafeMode( void );
+void       Com_RunAndTimeServerPacket( netadr_t *evFrom, msg_t *buf );

 void       Com_StartupVariable( const char *match );
 // checks for and removes command line "+set var arg" constructs
@@ -952,6 +953,7 @@ void SV_Init( void );
 void SV_Shutdown( char *finalmsg );
 void SV_Frame( int msec );
 void SV_PacketEvent( netadr_t from, msg_t *msg );
+int SV_FrameMsec( void );
 qboolean SV_GameCommand( void );

@@ -1065,6 +1067,7 @@ bool Sys_PathCmp( const char *path1, const char *path2 );
 char **Sys_ListFiles( const char *directory, const char *extension, char *filter, int *numfiles, qboolean wantsubs );
 void   Sys_FreeFileList( char **fileList );
 //rwwRMG - changed to fileList to not conflict with list type
+void   Sys_Sleep( int msec );

 qboolean Sys_LowPhysicalMemory();
 unsigned int Sys_ProcessorCount();
diff --git a/codemp/server/sv_main.cpp b/codemp/server/sv_main.cpp
index 7586f44..9e1549e 100644
--- a/codemp/server/sv_main.cpp
+++ b/codemp/server/sv_main.cpp
@@ -995,6 +995,29 @@ void SV_CheckCvars( void ) {

 /*
 ==================
+SV_FrameMsec
+Return time in millseconds until processing of the next server frame.
+==================
+*/
+int SV_FrameMsec()
+{
+   if ( sv_fps )
+   {
+       int frameMsec;
+
+       frameMsec = 1000.0f / sv_fps->value;
+
+       if ( frameMsec < sv.timeResidual )
+           return 0;
+       else
+           return frameMsec - sv.timeResidual;
+   }
+   else
+       return 1;
+}
+
+/*
+==================
 SV_Frame

 Player movement occurs as a result of packet events, which
@@ -1013,6 +1036,11 @@ void SV_Frame( int msec ) {
    }

    if ( !com_sv_running->integer ) {
+       // Running as a server, but no map loaded
+#ifdef DEDICATED
+       // Block until something interesting happens
+       Sys_Sleep( -1 );
+#endif
        return;
    }

@@ -1037,12 +1065,14 @@ void SV_Frame( int msec ) {

    if (!com_dedicated->integer) SV_BotFrame( sv.time + sv.timeResidual );

+#if 0
    if ( com_dedicated->integer && sv.timeResidual < frameMsec && (!com_timescale || com_timescale->value >= 1) ) {
        // NET_Sleep will give the OS time slices until either get a packet
        // or time enough for a server frame has gone by
        NET_Sleep(frameMsec - sv.timeResidual);
        return;
    }
+#endif

    // if time is about to hit the 32nd bit, kick all clients
    // and clear sv.time, rather
diff --git a/codemp/sys/sys_unix.cpp b/codemp/sys/sys_unix.cpp
index b20f2eb..5a32d51 100644
--- a/codemp/sys/sys_unix.cpp
+++ b/codemp/sys/sys_unix.cpp
@@ -192,8 +192,8 @@ byte        sys_packetReceived[MAX_MSGLEN];
 sysEvent_t Sys_GetEvent( void ) {
    sysEvent_t  ev;
    char        *s;
-   msg_t       netmsg;
-   netadr_t    adr;
+   //msg_t     netmsg;
+   //netadr_t  adr;

    // return if we have data
    if ( eventHead > eventTail ) {
@@ -213,6 +213,7 @@ sysEvent_t Sys_GetEvent( void ) {
        Sys_QueEvent( 0, SE_CONSOLE, 0, 0, len, b );
    }

+#if 0
    // check for network packets
    MSG_Init( &netmsg, sys_packetReceived, sizeof( sys_packetReceived ) );
    if ( Sys_GetPacket ( &adr, &netmsg ) ) {
@@ -226,6 +227,7 @@ sysEvent_t Sys_GetEvent( void ) {
        memcpy( buf+1, netmsg.data, netmsg.cursize );
        Sys_QueEvent( 0, SE_PACKET, 0, 0, len, buf );
    }
+#endif

    // return if we have data
    if ( eventHead > eventTail ) {
diff --git a/codemp/win32/win_main.cpp b/codemp/win32/win_main.cpp
index d8b1c00..587486b 100644
--- a/codemp/win32/win_main.cpp
+++ b/codemp/win32/win_main.cpp
@@ -355,6 +355,25 @@ void   Sys_FreeFileList( char **psList ) {
 //========================================================

 /*
+==============
+Sys_Sleep
+
+Block execution for msec or until input is received.
+==============
+*/
+void Sys_Sleep( int msec )
+{
+   if ( msec == 0 )
+       return;
+
+   // Client Sys_Sleep doesn't support waiting on stdin
+   if ( msec < 0 )
+       return;
+
+   Sleep( msec );
+}
+
+/*
 ================
 Sys_GetClipboardData

@@ -687,8 +706,8 @@ sysEvent_t Sys_GetEvent( void ) {
     MSG            msg;
    sysEvent_t  ev;
    char        *s;
-   msg_t       netmsg;
-   netadr_t    adr;
+   //msg_t     netmsg;
+   //netadr_t  adr;

    // return if we have data
    if ( eventHead > eventTail ) {
@@ -721,6 +740,7 @@ sysEvent_t Sys_GetEvent( void ) {
        Sys_QueEvent( 0, SE_CONSOLE, 0, 0, len, b );
    }

+#if 0
    // check for network packets
    MSG_Init( &netmsg, sys_packetReceived, sizeof( sys_packetReceived ) );
    if ( Sys_GetPacket ( &adr, &netmsg ) ) {
@@ -735,6 +755,7 @@ sysEvent_t Sys_GetEvent( void ) {
        memcpy( buf+1, &netmsg.data[netmsg.readcount], netmsg.cursize - netmsg.readcount );
        Sys_QueEvent( 0, SE_PACKET, 0, 0, len, buf );
    }
+#endif

    // return if we have data
    if ( eventHead > eventTail ) {
diff --git a/codemp/win32/win_main_ded.cpp b/codemp/win32/win_main_ded.cpp
index 5131fc2..13d84ed 100644
--- a/codemp/win32/win_main_ded.cpp
+++ b/codemp/win32/win_main_ded.cpp
@@ -497,6 +497,24 @@ void   Sys_FreeFileList( char **psList ) {
 //========================================================

 /*
+==============
+Sys_Sleep
+
+Block execution for msec or until input is received.
+==============
+*/
+void Sys_Sleep( int msec )
+{
+   if ( msec == 0 )
+       return;
+
+   if ( msec < 0 )
+       WaitForSingleObject( GetStdHandle( STD_INPUT_HANDLE ), INFINITE );
+   else
+       WaitForSingleObject( GetStdHandle( STD_INPUT_HANDLE ), msec );
+}
+
+/*
 ================
 Sys_GetClipboardData

@@ -829,8 +847,8 @@ sysEvent_t Sys_GetEvent( void ) {
     MSG            msg;
    sysEvent_t  ev;
    char        *s;
-   msg_t       netmsg;
-   netadr_t    adr;
+   //msg_t     netmsg;
+   //netadr_t  adr;

    // return if we have data
    if ( eventHead > eventTail ) {
@@ -863,6 +881,7 @@ sysEvent_t Sys_GetEvent( void ) {
        Sys_QueEvent( 0, SE_CONSOLE, 0, 0, len, b );
    }

+#if 0
    // check for network packets
    MSG_Init( &netmsg, sys_packetReceived, sizeof( sys_packetReceived ) );
    if ( Sys_GetPacket ( &adr, &netmsg ) ) {
@@ -877,6 +896,7 @@ sysEvent_t Sys_GetEvent( void ) {
        memcpy( buf+1, &netmsg.data[netmsg.readcount], netmsg.cursize - netmsg.readcount );
        Sys_QueEvent( 0, SE_PACKET, 0, 0, len, buf );
    }
+#endif

    // return if we have data
    if ( eventHead > eventTail ) {

[ensiform]

And won't work in SP.

[smcv]

I did essentially the same backport as @Razish before I found this bug and saw that you'd done it already, but my version also works in SP by using Sys_Sleep() there. It seems to work OK on Linux; no idea about Windows or Mac.

It's at https://github.com/smcv/OpenJK/tree/sleepy if you want it.

[ensiform]

com_maxfps stops working with either implementation on windows in MP.

[smcv]

Thanks, that's useful to know. I can see why you wouldn't want to merge either version while it regresses Windows.

[ensiform]

Not even sure why it happens, the only difference between us and ioq3 is that we are not yet using SDL/SDL2 on Windows but I'm not sure why exactly that would make a difference yet.

[smcv]

I'll try to reproduce it on Windows (or at least Wine) at some point, and see if I can work it out from there.

Does the sleep patch (either Razish's version or mine) work OK on Windows with "+set com_busyWait 1" on the command-line? If a per-platform default (or defaulting to 1) can make things better on Linux while not making Windows actually regress from where it is now, that'd be a start...

[ensiform]

That's the default with the patch as far as I know so I'm not sure that would help.

[smcv]

No, the default is to not busy-wait (i.e. the new way, using select/sleep, but possibly breaking Windows) in both @Razish's patch and mine:

• com_busyWait = Cvar_Get( "com_busyWait", "0", CVAR_ARCHIVE );

[ensiform]

Oh right, got them mixed up. Haven't honestly tried in ages.

[smcv]

I've rebased https://github.com/smcv/OpenJK/tree/sleepy on the unified SDL stuff. It works on Linux, but I don't have a Windows environment - could someone try it on Windows please?

Expected results: normally, CPU usage should be high-ish (on my laptop it's ~ 70% of a CPU core) but no longer 100%. If you set some stupidly low framerate like set com_maxfps 5, the frame rate should drop to 5 fps, and the CPU usage should also drop.

If it doesn't work on Windows, please describe what "doesn't work" means, then try with set com_busywait 1 which should revert to the equivalent of the old code (so, occupying 100% of a CPU core).

[ensiform]

FYI your updated patch seems to bring com_minimized and com_unfocused back into dedicated server.

Still haven't tested things yet. Probably won't until after vacation.

[xycaleth]

I'm going through issues to re-evaluate their priority. Is this change necessarily required for milestone 1? And it certainly doesn't seem high priority to me - the game runs fine without it and I see this more as an optimization rather than a need.

It seems like a pretty big change and I don't want to risk destabilizing the code too much as we're trying to wrap up the first release.

[ensiform]

Well I would think server hosts wouldn't like servers taking up so much CPU all the time when the original isn't.

[xycaleth]

Oh right, so jampDed doesn't do busy waits either?

[xycaleth]

jampDed does do busy waits, but adds a Sleep(5) to avoid 100% CPU utilization. The same currently happens in OpenJK. For now I think this is fine so I'll remove this issue from milestone 1.

[smcv]

FYI your updated patch seems to bring com_minimized and com_unfocused back into dedicated server.

Yes, ioquake3 has them in its dedicated server too. They are just never set to a non-trivial value in the dedicated server. The memory cost of a couple of cvars is low, and it seems better to have

    if (com_dedicated->integer) {
        /* server stuff here */
    } else {
        /* more elaborate logic involving com_minimized, com_unfocused, com_maxfps* */
    }

rather than

#ifdef DEDICATED
    /* server stuff here */
#else
    if (com_dedicated->integer) {
        /* duplicate copy of server stuff here */
    } else {
        /* more elaborate logic involving com_minimized, com_unfocused, com_maxfps* */
    }
#endif

or worse,

#ifndef DEDICATED
    if (com_dedicated->integer)
#endif
    {
        /* server stuff here */
    }
#ifndef DEDICATED
    else {
        /* more elaborate logic involving com_minimized, com_unfocused, com_maxfps* */
    }
#endif

I know OpenJK doesn't currently support using the client executable as a dedicated server (or at least I think I remember seeing that somewhere?), but if the logic for doing that is more or less intact, then that reduces the delta between ioquake3 and OpenJK, which seems like a win for future maintainability. (I assume you want to absorb pretty much all of the ioquake3 enhancements eventually.)

[ensiform]

Sure but it is still irrelevant for the DEDICATED preprocessor define (the separate executable) to have them as cvars which is what I was getting at.