From baff9c0363b3e0b012c55744836195040f53f3c0 Mon Sep 17 00:00:00 2001 From: Marc Di Luzio Date: Fri, 10 May 2019 20:37:51 +0100 Subject: [PATCH 01/28] Allow for long options using getopt_long --- daemon/main.c | 70 ++++++++++++++++++++++++++++----------------- data/gamemoded.8.in | 27 ++++++++--------- 2 files changed, 58 insertions(+), 39 deletions(-) diff --git a/daemon/main.c b/daemon/main.c index bd212af..7781bed 100644 --- a/daemon/main.c +++ b/daemon/main.c @@ -56,6 +56,7 @@ POSSIBILITY OF SUCH DAMAGE. #include "gamemode_client.h" #include "logging.h" +#include #include #include #include @@ -64,12 +65,13 @@ POSSIBILITY OF SUCH DAMAGE. #define USAGE_TEXT \ "Usage: %s [-d] [-l] [-r] [-t] [-h] [-v]\n\n" \ - " -d daemonize self after launch\n" \ - " -l log to syslog\n" \ - " -r request gamemode and pause\n" \ - " -t run tests\n" \ - " -h print this help\n" \ - " -v print version\n" \ + " -r, --request Request gamemode and pause\n" \ + " -s, --status Query the status of gamemode\n" \ + " -d, --daemonize Daemonize self after launch\n" \ + " -l, --log-to-syslog Log to syslog\n" \ + " -r, --test Run tests\n" \ + " -h, --help Print this help\n" \ + " -v, --version Print version\n" \ "\n" \ "See man page for more information.\n" @@ -102,8 +104,17 @@ int main(int argc, char *argv[]) bool daemon = false; bool use_syslog = false; int opt = 0; - int status; - while ((opt = getopt(argc, argv, "dlsrtvh")) != -1) { + + /* Options struct for getopt_long */ + static struct option long_options[] = { + { "daemonize", no_argument, 0, 'd' }, { "log-to-syslog", no_argument, 0, 'l' }, + { "request", no_argument, 0, 'r' }, { "test", no_argument, 0, 't' }, + { "status", no_argument, 0, 's' }, { "help", no_argument, 0, 'h' }, + { "version", no_argument, 0, 'v' } + }; + static const char *short_options = "dls::r::tvh"; + + while ((opt = getopt_long(argc, argv, short_options, long_options, 0)) != -1) { switch (opt) { case 'd': daemon = true; @@ -111,31 +122,41 @@ int main(int argc, char *argv[]) case 'l': use_syslog = true; break; - case 's': { - if ((status = gamemode_query_status()) < 0) { + + case 's': + + switch (gamemode_query_status()) { + case 0: + LOG_MSG("gamemode is inactive\n"); + break; + case 1: + LOG_MSG("gamemode is active\n"); + break; + case -1: LOG_ERROR("gamemode status request failed: %s\n", gamemode_error_string()); exit(EXIT_FAILURE); - } else if (status > 0) { - LOG_MSG("gamemode is active\n"); - } else { - LOG_MSG("gamemode is inactive\n"); + default: + LOG_ERROR("gamemode_query_status returned unexpected value 2\n"); + exit(EXIT_FAILURE); } exit(EXIT_SUCCESS); - break; - } + case 'r': + if (gamemode_request_start() < 0) { LOG_ERROR("gamemode request failed: %s\n", gamemode_error_string()); exit(EXIT_FAILURE); } - if ((status = gamemode_query_status()) == 2) { + switch (gamemode_query_status()) { + case 2: LOG_MSG("gamemode request succeeded and is active\n"); - } else if (status == 1) { + break; + case 1: LOG_ERROR("gamemode request succeeded and is active but registration failed\n"); exit(EXIT_FAILURE); - } else { + case 0: LOG_ERROR("gamemode request succeeded but is not active\n"); exit(EXIT_FAILURE); } @@ -153,23 +174,20 @@ int main(int argc, char *argv[]) } exit(EXIT_SUCCESS); - break; - case 't': - status = game_mode_run_client_tests(); + + case 't': { + int status = game_mode_run_client_tests(); exit(status); - break; + } case 'v': LOG_MSG(VERSION_TEXT); exit(EXIT_SUCCESS); - break; case 'h': LOG_MSG(USAGE_TEXT, argv[0]); exit(EXIT_SUCCESS); - break; default: fprintf(stderr, USAGE_TEXT, argv[0]); exit(EXIT_FAILURE); - break; } } diff --git a/data/gamemoded.8.in b/data/gamemoded.8.in index 247a381..3c5b010 100644 --- a/data/gamemoded.8.in +++ b/data/gamemoded.8.in @@ -4,7 +4,7 @@ .SH NAME gamemoded \- optimises system performance on demand .SH SYNOPSIS -\fBgamemoded\fR [\fB\-d\fR] [\fB\-l\fR] [\fB\-h\fR] [\fB\-v\fR] +\fBgamemoded\fR [OPTIONS...] .SH DESCRIPTION \fBGameMode\fR is a daemon/lib combo for Linux that allows games to request a set of optimisations be temporarily applied to the host OS. @@ -12,26 +12,27 @@ The design has a clear cut abstraction between the host daemon and library (\fBg \fBGameMode\fR was designed primarily as a stop-gap solution to problems with the Intel and AMD CPU powersave or ondemand governors, but is intended to be expanded beyond just CPU governor states, as there are a wealth of automation tasks one might want to apply. .SH OPTIONS + .TP 8 -.B \-d +.B \-r, \-\-request +Request gamemode and pause +.TP 8 +.B \-s, \-\-status +Query the status of gamemode +.TP 8 +.B \-d, \-\-daemonize Run the daemon as a separate process (daemonize it) .TP 8 -.B \-l +.B \-l, \-\-log-to-syslog Log to syslog -.TP 8 -.B \-r -Request gamemode and wait for any signal -.TP 8 -.B \-s -Query the current status of gamemode -.TP 8 -.B \-h +.TP 8 +.B \-h, \-\-help Print help text .TP 8 -.B \-t +.B \-t, \-\-test Run diagnostic tests on the current installation .TP 8 -.B \-v +.B \-v, \-\-version Print the version .SH USAGE From 717777e6c23852f5b3b2aa438c24a05b91cbbe22 Mon Sep 17 00:00:00 2001 From: Marc Di Luzio Date: Fri, 10 May 2019 20:40:31 +0100 Subject: [PATCH 02/28] Implement PID support for --request and --status --- daemon/main.c | 142 ++++++++++++++++++++++++++++++-------------- data/gamemoded.8.in | 10 ++-- 2 files changed, 105 insertions(+), 47 deletions(-) diff --git a/daemon/main.c b/daemon/main.c index 7781bed..ad611bf 100644 --- a/daemon/main.c +++ b/daemon/main.c @@ -65,8 +65,10 @@ POSSIBILITY OF SUCH DAMAGE. #define USAGE_TEXT \ "Usage: %s [-d] [-l] [-r] [-t] [-h] [-v]\n\n" \ - " -r, --request Request gamemode and pause\n" \ - " -s, --status Query the status of gamemode\n" \ + " -r[PID], --request=[PID] Toggle gamemode for process\n" \ + " When no PID given, requests gamemode and pauses\n" \ + " -s[PID], --status=[PID] Query the status of gamemode for process\n" \ + " When no PID given, queries the status globally\n" \ " -d, --daemonize Daemonize self after launch\n" \ " -l, --log-to-syslog Log to syslog\n" \ " -r, --test Run tests\n" \ @@ -107,9 +109,9 @@ int main(int argc, char *argv[]) /* Options struct for getopt_long */ static struct option long_options[] = { - { "daemonize", no_argument, 0, 'd' }, { "log-to-syslog", no_argument, 0, 'l' }, - { "request", no_argument, 0, 'r' }, { "test", no_argument, 0, 't' }, - { "status", no_argument, 0, 's' }, { "help", no_argument, 0, 'h' }, + { "daemonize", no_argument, 0, 'd' }, { "log-to-syslog", no_argument, 0, 'l' }, + { "request", optional_argument, 0, 'r' }, { "test", no_argument, 0, 't' }, + { "status", optional_argument, 0, 's' }, { "help", no_argument, 0, 'h' }, { "version", no_argument, 0, 'v' } }; static const char *short_options = "dls::r::tvh"; @@ -124,53 +126,107 @@ int main(int argc, char *argv[]) break; case 's': - - switch (gamemode_query_status()) { - case 0: - LOG_MSG("gamemode is inactive\n"); - break; - case 1: - LOG_MSG("gamemode is active\n"); - break; - case -1: - LOG_ERROR("gamemode status request failed: %s\n", gamemode_error_string()); - exit(EXIT_FAILURE); - default: - LOG_ERROR("gamemode_query_status returned unexpected value 2\n"); - exit(EXIT_FAILURE); + if (optarg != NULL) { + pid_t pid = atoi(optarg); + switch (gamemode_query_status_for(pid)) { + case 0: /* inactive */ + LOG_MSG("gamemode is inactive\n"); + break; + case 1: /* active not not registered */ + LOG_MSG("gamemode is active but [%d] not registered\n", pid); + break; + case 2: /* active for client */ + LOG_MSG("gamemode is active and [%d] registered\n", pid); + break; + case -1: + LOG_ERROR("gamemode_query_status_for(%d) failed: %s\n", + pid, + gamemode_error_string()); + exit(EXIT_FAILURE); + default: + LOG_ERROR("gamemode_query_status returned unexpected value 2\n"); + exit(EXIT_FAILURE); + } + } else { + switch (gamemode_query_status()) { + case 0: + LOG_MSG("gamemode is inactive\n"); + break; + case 1: + LOG_MSG("gamemode is active\n"); + break; + case -1: + LOG_ERROR("gamemode status request failed: %s\n", gamemode_error_string()); + exit(EXIT_FAILURE); + default: + LOG_ERROR("gamemode_query_status returned unexpected value 2\n"); + exit(EXIT_FAILURE); + } } exit(EXIT_SUCCESS); case 'r': - if (gamemode_request_start() < 0) { - LOG_ERROR("gamemode request failed: %s\n", gamemode_error_string()); - exit(EXIT_FAILURE); - } + if (optarg != NULL) { + pid_t pid = atoi(optarg); + switch (gamemode_query_status_for(pid)) { + case 0: /* inactive */ + case 1: /* active not not registered */ + LOG_MSG("gamemode not active for client, requesting start for %d...\n", pid); + if (gamemode_request_start_for(pid) < 0) { + LOG_ERROR("gamemode_request_start_for(%d) failed: %s\n", + pid, + gamemode_error_string()); + exit(EXIT_FAILURE); + } + LOG_MSG("request succeeded\n"); + break; + case 2: /* active for client */ + LOG_MSG("gamemode active for client, requesting end for %d...\n", pid); + if (gamemode_request_end_for(pid) < 0) { + LOG_ERROR("gamemode_request_end_for(%d) failed: %s\n", + pid, + gamemode_error_string()); + exit(EXIT_FAILURE); + } + LOG_MSG("request succeeded\n"); + break; + case -1: + LOG_ERROR("gamemode_query_status_for(%d) failed: %s\n", + pid, + gamemode_error_string()); + exit(EXIT_FAILURE); + } + } else { + if (gamemode_request_start() < 0) { + LOG_ERROR("gamemode request failed: %s\n", gamemode_error_string()); + exit(EXIT_FAILURE); + } - switch (gamemode_query_status()) { - case 2: - LOG_MSG("gamemode request succeeded and is active\n"); - break; - case 1: - LOG_ERROR("gamemode request succeeded and is active but registration failed\n"); - exit(EXIT_FAILURE); - case 0: - LOG_ERROR("gamemode request succeeded but is not active\n"); - exit(EXIT_FAILURE); - } + switch (gamemode_query_status()) { + case 2: + LOG_MSG("gamemode request succeeded and is active\n"); + break; + case 1: + LOG_ERROR("gamemode request succeeded and is active but registration failed\n"); + exit(EXIT_FAILURE); + case 0: + LOG_ERROR("gamemode request succeeded but is not active\n"); + exit(EXIT_FAILURE); + } - // Simply pause and wait a SIGINT - if (signal(SIGINT, sigint_handler_noexit) == SIG_ERR) { - FATAL_ERRORNO("Could not catch SIGINT"); - } - pause(); + // Simply pause and wait a SIGINT + if (signal(SIGINT, sigint_handler_noexit) == SIG_ERR) { + FATAL_ERRORNO("Could not catch SIGINT"); + } + pause(); - // Explicitly clean up - if (gamemode_request_end() < 0) { - LOG_ERROR("gamemode request failed: %s\n", gamemode_error_string()); - exit(EXIT_FAILURE); + // Explicitly clean up + if (gamemode_request_end() < 0) { + LOG_ERROR("gamemode request failed: %s\n", gamemode_error_string()); + exit(EXIT_FAILURE); + } } exit(EXIT_SUCCESS); diff --git a/data/gamemoded.8.in b/data/gamemoded.8.in index 3c5b010..424a650 100644 --- a/data/gamemoded.8.in +++ b/data/gamemoded.8.in @@ -14,11 +14,13 @@ The design has a clear cut abstraction between the host daemon and library (\fBg .SH OPTIONS .TP 8 -.B \-r, \-\-request -Request gamemode and pause +.B \-r[PID], \-\-request=[PID] +Toggle gamemode for process. +When no PID given, requests gamemode and pauses .TP 8 -.B \-s, \-\-status -Query the status of gamemode +.B \-s[PID], \-\-status=[PID] +Query the status of gamemode for process +When no PID given, queries the status globally .TP 8 .B \-d, \-\-daemonize Run the daemon as a separate process (daemonize it) From a482b72d37d6afd1da7c49d33e3a12d0302d0c0e Mon Sep 17 00:00:00 2001 From: Marc Di Luzio Date: Fri, 10 May 2019 20:40:58 +0100 Subject: [PATCH 03/28] Fix comments in gamemode_client.h --- lib/gamemode_client.h | 8 +++++--- 1 file changed, 5 insertions(+), 3 deletions(-) diff --git a/lib/gamemode_client.h b/lib/gamemode_client.h index dfc67e7..9c6f46a 100644 --- a/lib/gamemode_client.h +++ b/lib/gamemode_client.h @@ -58,9 +58,11 @@ POSSIBILITY OF SUCH DAMAGE. * -1 if the request failed * -2 if the request was rejected * - * int gamemode_query_status_for(pid_t pid) - Query the current status of gamemode for another - * process 0 if gamemode is inactive 1 if gamemode is active 2 if gamemode is active and this client - * is registered -1 if the query failed + * int gamemode_query_status_for(pid_t pid) - Query status of gamemode for another process + * 0 if gamemode is inactive + * 1 if gamemode is active + * 2 if gamemode is active and this client is registered + * -1 if the query failed * * const char* gamemode_error_string() - Get an error string * returns a string describing any of the above errors From 7e5216c4a09030b0d350e65e04891e4a734b4959 Mon Sep 17 00:00:00 2001 From: Marc Di Luzio Date: Sat, 11 May 2019 09:09:55 +0100 Subject: [PATCH 04/28] Some comments and cleanup --- daemon/main.c | 33 +++++++++++++++++++++------------ 1 file changed, 21 insertions(+), 12 deletions(-) diff --git a/daemon/main.c b/daemon/main.c index ad611bf..d75c04b 100644 --- a/daemon/main.c +++ b/daemon/main.c @@ -148,18 +148,19 @@ int main(int argc, char *argv[]) exit(EXIT_FAILURE); } } else { - switch (gamemode_query_status()) { - case 0: + int ret = 0; + switch ((ret = gamemode_query_status())) { + case 0: /* inactive */ LOG_MSG("gamemode is inactive\n"); break; - case 1: + case 1: /* active */ LOG_MSG("gamemode is active\n"); break; - case -1: + case -1: /* error */ LOG_ERROR("gamemode status request failed: %s\n", gamemode_error_string()); exit(EXIT_FAILURE); - default: - LOG_ERROR("gamemode_query_status returned unexpected value 2\n"); + default: /* unexpected value eg. 2 */ + LOG_ERROR("gamemode_query_status returned unexpected value %d\n", ret); exit(EXIT_FAILURE); } } @@ -170,6 +171,8 @@ int main(int argc, char *argv[]) if (optarg != NULL) { pid_t pid = atoi(optarg); + + /* toggle gamemode for the process */ switch (gamemode_query_status_for(pid)) { case 0: /* inactive */ case 1: /* active not not registered */ @@ -192,37 +195,43 @@ int main(int argc, char *argv[]) } LOG_MSG("request succeeded\n"); break; - case -1: + case -1: /* error */ LOG_ERROR("gamemode_query_status_for(%d) failed: %s\n", pid, gamemode_error_string()); exit(EXIT_FAILURE); } + } else { + /* Request gamemode for this process */ if (gamemode_request_start() < 0) { LOG_ERROR("gamemode request failed: %s\n", gamemode_error_string()); exit(EXIT_FAILURE); } + /* Request and report on the status */ switch (gamemode_query_status()) { - case 2: + case 2: /* active for this client */ LOG_MSG("gamemode request succeeded and is active\n"); break; - case 1: + case 1: /* active */ LOG_ERROR("gamemode request succeeded and is active but registration failed\n"); exit(EXIT_FAILURE); - case 0: + case 0: /* innactive */ LOG_ERROR("gamemode request succeeded but is not active\n"); exit(EXIT_FAILURE); + case -1: /* error */ + LOG_ERROR("gamemode_query_status failed: %s\n", gamemode_error_string()); + exit(EXIT_FAILURE); } - // Simply pause and wait a SIGINT + /* Simply pause and wait a SIGINT */ if (signal(SIGINT, sigint_handler_noexit) == SIG_ERR) { FATAL_ERRORNO("Could not catch SIGINT"); } pause(); - // Explicitly clean up + /* Explicitly clean up */ if (gamemode_request_end() < 0) { LOG_ERROR("gamemode request failed: %s\n", gamemode_error_string()); exit(EXIT_FAILURE); From 09d63ae4f558169b8a6559a926b2f6cbe4afddb3 Mon Sep 17 00:00:00 2001 From: Marc Di Luzio Date: Sat, 11 May 2019 10:33:29 +0100 Subject: [PATCH 05/28] Set up for resetting niceness value Add tests to check this feature Apply config validation in config for simplicity Note: if anything messes with the niceness (ie. it starts non-zero, or it's not the expected value during setup, we'll bail out) --- daemon/daemon_config.c | 5 ++++ daemon/gamemode-sched.c | 62 +++++++++++++++++++++++++++++++---------- daemon/gamemode-tests.c | 58 ++++++++++++++++++++++++++++++++++++++ daemon/gamemode.c | 7 ++++- daemon/gamemode.h | 3 +- 5 files changed, 118 insertions(+), 17 deletions(-) diff --git a/daemon/daemon_config.c b/daemon/daemon_config.c index 23e0e76..cb2a7eb 100644 --- a/daemon/daemon_config.c +++ b/daemon/daemon_config.c @@ -548,6 +548,11 @@ long config_get_renice_value(GameModeConfig *self) { long value = 0; memcpy_locked_config(self, &value, &self->values.renice, sizeof(long)); + /* Validate the renice value */ + if ((value < 1 || value > 20) && value != 0) { + LOG_ONCE(ERROR, "Configured renice value '%ld' is invalid, will not renice.\n", value); + value = 0; + } return value; } diff --git a/daemon/gamemode-sched.c b/daemon/gamemode-sched.c index 698ecd1..e4f2019 100644 --- a/daemon/gamemode-sched.c +++ b/daemon/gamemode-sched.c @@ -54,13 +54,28 @@ POSSIBILITY OF SUCH DAMAGE. * This tries to change the scheduler of the client to soft realtime mode * available in some kernels as SCHED_ISO. It also tries to adjust the nice * level. If some of each fail, ignore this and log a warning. - * - * We don't need to store the current values because when the client exits, - * everything will be good: Scheduling is only applied to the client and - * its children. */ -void game_mode_apply_renice(const GameModeContext *self, const pid_t client) + +#define RENICE_INVALID -128 /* Special value to store invalid value */ +int game_mode_get_renice(const pid_t client) { + /* Clear errno as -1 is a regitimate return */ + errno = 0; + int priority = getpriority(PRIO_PROCESS, (id_t)client); + if (priority == -1 && errno) { + LOG_ERROR("getprority(PRIO_PROCESS, %d) failed : %s\n", client, strerror(errno)); + return RENICE_INVALID; + } + return -priority; +} + +/* If expected is 0 then we try to apply our renice, otherwise, we try to remove it */ +void game_mode_apply_renice(const GameModeContext *self, const pid_t client, int expected) +{ + if (expected == RENICE_INVALID) + /* Silently bail if fed an invalid value */ + return; + GameModeConfig *config = game_mode_config_from_context(self); /* @@ -69,18 +84,35 @@ void game_mode_apply_renice(const GameModeContext *self, const pid_t client) long int renice = config_get_renice_value(config); if (renice == 0) { return; - } else if ((renice < 1) || (renice > 20)) { - LOG_ONCE(ERROR, "Configured renice value '%ld' is invalid, will not renice.\n", renice); - return; - } else { - renice = -renice; } - /* - * don't adjust priority if it was already adjusted - */ - if (getpriority(PRIO_PROCESS, (id_t)client) != 0) { - LOG_ERROR("Refused to renice client [%d]: already reniced\n", client); + /* Invert the renice value */ + renice = -renice; + + /* When expected is non-zero, we should try and remove the renice only if it doesn't match the + * expected value */ + if (expected != 0) { + expected = (int)renice; + renice = 0; + } + + /* Clear errno as -1 is a regitimate return */ + errno = 0; + int prio = getpriority(PRIO_PROCESS, (id_t)client); + if (prio == -1 && errno) { + /* process has likely ended, only log an error if we were actually trying to set a non-zero + * value */ + if (errno == ESRCH && renice != 0) + LOG_ERROR("getpriority returned ESRCH for process %d\n", client); + } else if (prio != expected) { + /* + * Don't adjust priority if it does not match the expected value + * ie. Another process has changed it, or it began non-standard + */ + LOG_ERROR("Refused to renice client [%d]: prio was (%d) but we expected (%d)\n", + client, + prio, + expected); } else if (setpriority(PRIO_PROCESS, (id_t)client, (int)renice)) { LOG_HINTED(ERROR, "Failed to renice client [%d], ignoring error condition: %s\n", diff --git a/daemon/gamemode-tests.c b/daemon/gamemode-tests.c index 71f2a9d..f34a613 100644 --- a/daemon/gamemode-tests.c +++ b/daemon/gamemode-tests.c @@ -536,6 +536,49 @@ int run_gpu_optimisation_tests(struct GameModeConfig *config) return gpustatus; } +int run_renice_tests(struct GameModeConfig *config) +{ + /* read configuration "renice" (1..20) */ + long int renice = config_get_renice_value(config); + if (renice == 0) { + return 1; /* not configured */ + } + + /* Verify renice starts at 0 */ + int val = game_mode_get_renice(getpid()); + if (val != 0) { + LOG_ERROR("Initial renice value is non-zero: %d\n", val); + return -1; + } + + int ret = 0; + + /* Ask for gamemode for ourselves */ + gamemode_request_start(); + + /* Check renice is now requested value */ + val = game_mode_get_renice(getpid()); + if (val != renice) { + LOG_ERROR( + "renice value not set correctly after gamemode_request_start\nExpected: %ld, Was: %d\n", + renice, + val); + ret = -1; + } + + /* End gamemode for ourselves */ + gamemode_request_end(); + + /* Check renice is returned to correct value */ + val = game_mode_get_renice(getpid()); + if (val != 0) { + LOG_ERROR("renice value non-zero after gamemode_request_end\nExpected: 0, Was: %d\n", val); + ret = -1; + } + + return ret; +} + /** * game_mode_run_feature_tests runs a set of tests for each current feature (based on the current * config) returns 0 for success, -1 for failure @@ -598,6 +641,21 @@ static int game_mode_run_feature_tests(struct GameModeConfig *config) /* TODO: Unknown if this is testable, org.freedesktop.ScreenSaver has no query method */ /* Was the process reniced? */ + { + LOG_MSG("::: Verifying renice\n"); + int renicestatus = run_renice_tests(config); + + if (renicestatus == 1) + LOG_MSG("::: Passed (no renice configured)\n"); + else if (renicestatus == 0) + LOG_MSG("::: Passed\n"); + else { + LOG_MSG("::: Failed!\n"); + // Renice should be expected to work, if set + status = 1; + } + } + /* Was the scheduling applied? */ /* Were io priorities changed? */ /* Note: These don't get cleared up on un-register, so will have already been applied */ diff --git a/daemon/gamemode.c b/daemon/gamemode.c index e8fab8b..ff19799 100644 --- a/daemon/gamemode.c +++ b/daemon/gamemode.c @@ -71,6 +71,8 @@ struct GameModeContext { struct GameModeGPUInfo *stored_gpu; /**initial_renice = game_mode_get_renice(client); + game_mode_apply_renice(self, client, 0 /* expect zero value to start with */); + /* Apply scheduler policies */ - game_mode_apply_renice(self, client); game_mode_apply_scheduling(self, client); /* Apply io priorities */ diff --git a/daemon/gamemode.h b/daemon/gamemode.h index 7a7703c..61003fa 100644 --- a/daemon/gamemode.h +++ b/daemon/gamemode.h @@ -135,7 +135,8 @@ int game_mode_close_proc(const procfd_t procfd); * Provides internal API functions specific to adjusting process * scheduling. */ -void game_mode_apply_renice(const GameModeContext *self, const pid_t client); +int game_mode_get_renice(const pid_t client); +void game_mode_apply_renice(const GameModeContext *self, const pid_t client, int expected); void game_mode_apply_scheduling(const GameModeContext *self, const pid_t client); /** gamemode-wine.c From 6d141496588e32882fbf44cb165c02e337bcbdfd Mon Sep 17 00:00:00 2001 From: Marc Di Luzio Date: Sat, 11 May 2019 10:41:43 +0100 Subject: [PATCH 06/28] Reset renice value on unregister Part of fixing up https://github.com/FeralInteractive/gamemode/issues/141 --- daemon/gamemode.c | 3 +++ 1 file changed, 3 insertions(+) diff --git a/daemon/gamemode.c b/daemon/gamemode.c index ff19799..8823c89 100644 --- a/daemon/gamemode.c +++ b/daemon/gamemode.c @@ -511,6 +511,9 @@ int game_mode_context_unregister(GameModeContext *self, pid_t client, pid_t requ game_mode_client_count_changed(); + /* Restore the renice value for the process, expecting it to be our config value */ + game_mode_apply_renice(self, client, (int)config_get_renice_value(self->config)); + return 0; } From 2249a713556cca28579ce703ab58549279ae9cb4 Mon Sep 17 00:00:00 2001 From: Marc Di Luzio Date: Sat, 11 May 2019 11:53:52 +0100 Subject: [PATCH 07/28] Get ready for re-setting ioprio value on un-register Implements tests for feature Fixes CLAMP macro --- daemon/daemon_config.c | 19 ++++++++ daemon/daemon_config.h | 1 + daemon/gamemode-ioprio.c | 93 ++++++++++++++++++++++++---------------- daemon/gamemode-tests.c | 63 +++++++++++++++++++++++++++ daemon/gamemode.c | 8 ++-- daemon/gamemode.h | 3 +- daemon/helpers.h | 2 +- 7 files changed, 148 insertions(+), 41 deletions(-) diff --git a/daemon/daemon_config.c b/daemon/daemon_config.c index cb2a7eb..a03147b 100644 --- a/daemon/daemon_config.c +++ b/daemon/daemon_config.c @@ -31,6 +31,7 @@ POSSIBILITY OF SUCH DAMAGE. #define _GNU_SOURCE #include "daemon_config.h" +#include "helpers.h" #include "logging.h" /* Ben Hoyt's inih library */ @@ -564,12 +565,30 @@ long config_get_ioprio_value(GameModeConfig *self) long value = 0; char ioprio_value[CONFIG_VALUE_MAX] = { 0 }; memcpy_locked_config(self, ioprio_value, &self->values.ioprio, sizeof(self->values.ioprio)); + + /* account for special string values */ if (0 == strncmp(ioprio_value, "off", sizeof(self->values.ioprio))) value = IOPRIO_DONT_SET; else if (0 == strncmp(ioprio_value, "default", sizeof(self->values.ioprio))) value = IOPRIO_RESET_DEFAULT; else value = atoi(ioprio_value); + + /* Validate values */ + if (IOPRIO_RESET_DEFAULT == value) { + LOG_ONCE(MSG, "IO priority will be reset to default behavior (based on CPU priority).\n"); + value = 0; + } else { + /* maybe clamp the value */ + long invalid_ioprio = value; + value = CLAMP(0, 7, value); + if (value != invalid_ioprio) + LOG_ONCE(ERROR, + "IO priority value %ld invalid, clamping to %ld\n", + invalid_ioprio, + value); + } + return value; } diff --git a/daemon/daemon_config.h b/daemon/daemon_config.h index 7418d07..790d26c 100644 --- a/daemon/daemon_config.h +++ b/daemon/daemon_config.h @@ -44,6 +44,7 @@ POSSIBILITY OF SUCH DAMAGE. */ #define IOPRIO_RESET_DEFAULT -1 #define IOPRIO_DONT_SET -2 +#define IOPRIO_DEFAULT 4 /* * Opaque config context type diff --git a/daemon/gamemode-ioprio.c b/daemon/gamemode-ioprio.c index f287421..544be9f 100644 --- a/daemon/gamemode-ioprio.c +++ b/daemon/gamemode-ioprio.c @@ -86,6 +86,25 @@ static inline int ioprio_set(int which, int who, int ioprio) return (int)syscall(SYS_ioprio_set, which, who, ioprio); } +static inline int ioprio_get(int which, int who) +{ + return (int)syscall(SYS_ioprio_get, which, who); +} + +/** + * Get the i/o priorities + */ +int game_mode_get_ioprio(const pid_t client) +{ + int ret = ioprio_get(IOPRIO_WHO_PROCESS, client); + if (ret == -1) { + LOG_ERROR("Failed to get ioprio value for [%d] with error %s\n", client, strerror(errno)); + ret = IOPRIO_DONT_SET; + } + /* We support only IOPRIO_CLASS_BE as IOPRIO_CLASS_RT required CAP_SYS_ADMIN */ + return IOPRIO_PRIO_DATA(ret); +} + /** * Apply io priorities * @@ -93,49 +112,51 @@ static inline int ioprio_set(int which, int who, int ioprio) * and can possibly reduce lags or latency when a game has to load assets * on demand. */ -void game_mode_apply_ioprio(const GameModeContext *self, const pid_t client) +void game_mode_apply_ioprio(const GameModeContext *self, const pid_t client, int expected) { + if (expected == IOPRIO_DONT_SET) + /* Silently bail if fed a don't set (invalid) */ + return; + GameModeConfig *config = game_mode_config_from_context(self); - LOG_MSG("Setting scheduling policies...\n"); - - /* - * read configuration "ioprio" (0..7) - */ + /* read configuration "ioprio" (0..7) */ int ioprio = (int)config_get_ioprio_value(config); - if (IOPRIO_RESET_DEFAULT == ioprio) { - LOG_MSG("IO priority will be reset to default behavior (based on CPU priority).\n"); - ioprio = 0; - } else if (IOPRIO_DONT_SET == ioprio) { - return; - } else { - /* maybe clamp the value */ - int invalid_ioprio = ioprio; - ioprio = CLAMP(0, 7, ioprio); - if (ioprio != invalid_ioprio) - LOG_ONCE(ERROR, - "IO priority value %d invalid, clamping to %d\n", - invalid_ioprio, - ioprio); - /* We support only IOPRIO_CLASS_BE as IOPRIO_CLASS_RT required CAP_SYS_ADMIN */ - ioprio = IOPRIO_PRIO_VALUE(IOPRIO_CLASS_BE, ioprio); + /* Special value to simply not set the value */ + if (ioprio == IOPRIO_DONT_SET) + return; + + LOG_MSG("Setting ioprio value...\n"); + + /* If fed the default, we'll try and reset the value back */ + if (expected != IOPRIO_DEFAULT) { + expected = (int)ioprio; + ioprio = IOPRIO_DEFAULT; } - /* - * Actually apply the io priority - */ - int c = IOPRIO_PRIO_CLASS(ioprio), p = IOPRIO_PRIO_DATA(ioprio); - if (ioprio_set(IOPRIO_WHO_PROCESS, client, ioprio) == 0) { - if (0 == ioprio) - LOG_MSG("Resetting client [%d] IO priority.\n", client); - else - LOG_MSG("Setting client [%d] IO priority to (%d,%d).\n", client, c, p); - } else { - LOG_ERROR("Setting client [%d] IO priority to (%d,%d) failed with error %d, ignoring\n", + int current = game_mode_get_ioprio(client); + if (current == IOPRIO_DONT_SET) { + /* Couldn't get the ioprio value, let's bail */ + return; + } else if (current != expected) { + /* Don't try and adjust the ioprio value if the value we got doesn't match default */ + LOG_ERROR("Refused to set ioprio on client [%d]: prio was (%d) but we expected (%d)\n", client, - c, - p, - errno); + current, + expected); + } else { + /* + * For now we only support IOPRIO_CLASS_BE + * IOPRIO_CLASS_RT requires CAP_SYS_ADMIN but should be possible with a polkit process + */ + int p = ioprio; + ioprio = IOPRIO_PRIO_VALUE(IOPRIO_CLASS_BE, ioprio); + if (ioprio_set(IOPRIO_WHO_PROCESS, client, ioprio) != 0) { + LOG_ERROR("Setting client [%d] IO priority to (%d) failed with error %d, ignoring\n", + client, + p, + errno); + } } } diff --git a/daemon/gamemode-tests.c b/daemon/gamemode-tests.c index f34a613..2d4c54a 100644 --- a/daemon/gamemode-tests.c +++ b/daemon/gamemode-tests.c @@ -579,6 +579,53 @@ int run_renice_tests(struct GameModeConfig *config) return ret; } +int run_ioprio_tests(struct GameModeConfig *config) +{ + /* read configuration "ioprio" */ + long int ioprio = config_get_ioprio_value(config); + if (ioprio == IOPRIO_DONT_SET) { + return 1; /* not configured */ + } + + /* Verify ioprio starts at 0 */ + int val = game_mode_get_ioprio(getpid()); + if (val != IOPRIO_DEFAULT) { + LOG_ERROR("Initial ioprio value is non-default\nExpected: %d, Was: %d\n", + IOPRIO_DEFAULT, + val); + return -1; + } + + int ret = 0; + + /* Ask for gamemode for ourselves */ + gamemode_request_start(); + + /* Check renice is now requested value */ + val = game_mode_get_ioprio(getpid()); + if (val != ioprio) { + LOG_ERROR( + "ioprio value not set correctly after gamemode_request_start\nExpected: %ld, Was: %d\n", + ioprio, + val); + ret = -1; + } + + /* End gamemode for ourselves */ + gamemode_request_end(); + + /* Check ioprio is returned to correct value */ + val = game_mode_get_ioprio(getpid()); + if (val != IOPRIO_DEFAULT) { + LOG_ERROR("ioprio value non-default after gamemode_request_end\nExpected: %d, Was: %d\n", + IOPRIO_DEFAULT, + val); + ret = -1; + } + + return ret; +} + /** * game_mode_run_feature_tests runs a set of tests for each current feature (based on the current * config) returns 0 for success, -1 for failure @@ -656,6 +703,22 @@ static int game_mode_run_feature_tests(struct GameModeConfig *config) } } + /* Was the process ioprio set? */ + { + LOG_MSG("::: Verifying ioprio\n"); + int iopriostatus = run_ioprio_tests(config); + + if (iopriostatus == 1) + LOG_MSG("::: Passed (no ioprio configured)\n"); + else if (iopriostatus == 0) + LOG_MSG("::: Passed\n"); + else { + LOG_MSG("::: Failed!\n"); + // Ioprio should be expected to work, if set + status = 1; + } + } + /* Was the scheduling applied? */ /* Were io priorities changed? */ /* Note: These don't get cleared up on un-register, so will have already been applied */ diff --git a/daemon/gamemode.c b/daemon/gamemode.c index 8823c89..d9c1708 100644 --- a/daemon/gamemode.c +++ b/daemon/gamemode.c @@ -72,6 +72,7 @@ struct GameModeContext { struct GameModeGPUInfo *target_gpu; /**initial_renice = game_mode_get_renice(client); game_mode_apply_renice(self, client, 0 /* expect zero value to start with */); + /* Store current ioprio value and apply */ + self->initial_ioprio = game_mode_get_ioprio(client); + game_mode_apply_ioprio(self, client, IOPRIO_DEFAULT); + /* Apply scheduler policies */ game_mode_apply_scheduling(self, client); - /* Apply io priorities */ - game_mode_apply_ioprio(self, client); - game_mode_client_count_changed(); return 0; diff --git a/daemon/gamemode.h b/daemon/gamemode.h index 61003fa..d5d6eb6 100644 --- a/daemon/gamemode.h +++ b/daemon/gamemode.h @@ -122,7 +122,8 @@ char *game_mode_lookup_user_home(void); * Provides internal API functions specific to adjusting process * IO priorities. */ -void game_mode_apply_ioprio(const GameModeContext *self, const pid_t client); +int game_mode_get_ioprio(const pid_t client); +void game_mode_apply_ioprio(const GameModeContext *self, const pid_t client, int expected); /** gamemode-proc.c * Provides internal API functions specific to working with process diff --git a/daemon/helpers.h b/daemon/helpers.h index 8595092..f8dfc1a 100644 --- a/daemon/helpers.h +++ b/daemon/helpers.h @@ -39,7 +39,7 @@ POSSIBILITY OF SUCH DAMAGE. /** * Value clamping helper, works like MIN/MAX but constraints a value within the range. */ -#define CLAMP(lbound, ubound, value) MIN(MIN(lbound, ubound), MAX(MAX(lbound, ubound), value)) +#define CLAMP(l, u, value) MAX(MIN(l, u), MIN(MAX(l, u), value)) /** * Little helper to safely print into a buffer, returns a pointer into the buffer From 598969a538a77d84151cdbea9dc422802dee1dcd Mon Sep 17 00:00:00 2001 From: Marc Di Luzio Date: Sat, 11 May 2019 11:57:45 +0100 Subject: [PATCH 08/28] Implement restoring the old ioprio value when unregistered Another part of the fix for https://github.com/FeralInteractive/gamemode/issues/141 --- daemon/gamemode.c | 3 +++ 1 file changed, 3 insertions(+) diff --git a/daemon/gamemode.c b/daemon/gamemode.c index d9c1708..db947cb 100644 --- a/daemon/gamemode.c +++ b/daemon/gamemode.c @@ -513,6 +513,9 @@ int game_mode_context_unregister(GameModeContext *self, pid_t client, pid_t requ game_mode_client_count_changed(); + /* Restore the ioprio value for the process, expecting it to be the config value */ + game_mode_apply_ioprio(self, client, (int)config_get_ioprio_value(self->config)); + /* Restore the renice value for the process, expecting it to be our config value */ game_mode_apply_renice(self, client, (int)config_get_renice_value(self->config)); From 7f196cdd1a8f38b46426090b3d60411290e5ec83 Mon Sep 17 00:00:00 2001 From: Marc Di Luzio Date: Sat, 11 May 2019 13:06:43 +0100 Subject: [PATCH 09/28] Fix formatting for travis --- daemon/main.c | 13 +++++++------ 1 file changed, 7 insertions(+), 6 deletions(-) diff --git a/daemon/main.c b/daemon/main.c index d75c04b..525fce3 100644 --- a/daemon/main.c +++ b/daemon/main.c @@ -108,12 +108,13 @@ int main(int argc, char *argv[]) int opt = 0; /* Options struct for getopt_long */ - static struct option long_options[] = { - { "daemonize", no_argument, 0, 'd' }, { "log-to-syslog", no_argument, 0, 'l' }, - { "request", optional_argument, 0, 'r' }, { "test", no_argument, 0, 't' }, - { "status", optional_argument, 0, 's' }, { "help", no_argument, 0, 'h' }, - { "version", no_argument, 0, 'v' } - }; + static struct option long_options[] = { { "daemonize", no_argument, 0, 'd' }, + { "log-to-syslog", no_argument, 0, 'l' }, + { "request", optional_argument, 0, 'r' }, + { "test", no_argument, 0, 't' }, + { "status", optional_argument, 0, 's' }, + { "help", no_argument, 0, 'h' }, + { "version", no_argument, 0, 'v' } }; static const char *short_options = "dls::r::tvh"; while ((opt = getopt_long(argc, argv, short_options, long_options, 0)) != -1) { From 5ffe832faf2e50b3380ed4dbe6e5b091b4008ef7 Mon Sep 17 00:00:00 2001 From: Marc Di Luzio Date: Sat, 11 May 2019 13:24:13 +0100 Subject: [PATCH 10/28] Don't store the initial values 1. We don't use them anyway (though that could be a feature request) 2. They weren't stored per-client, so would be incorrect anyway --- daemon/gamemode.c | 5 ----- 1 file changed, 5 deletions(-) diff --git a/daemon/gamemode.c b/daemon/gamemode.c index db947cb..6b977f8 100644 --- a/daemon/gamemode.c +++ b/daemon/gamemode.c @@ -71,9 +71,6 @@ struct GameModeContext { struct GameModeGPUInfo *stored_gpu; /**initial_renice = game_mode_get_renice(client); game_mode_apply_renice(self, client, 0 /* expect zero value to start with */); /* Store current ioprio value and apply */ - self->initial_ioprio = game_mode_get_ioprio(client); game_mode_apply_ioprio(self, client, IOPRIO_DEFAULT); /* Apply scheduler policies */ From 934b4976036567c61b4fbbc87b857e33ddf9ea08 Mon Sep 17 00:00:00 2001 From: Marc Di Luzio Date: Sun, 12 May 2019 12:47:53 +0100 Subject: [PATCH 11/28] Implement multithreaded test framework (and use for ioprio) ioprio tests will now fail due to https://github.com/FeralInteractive/gamemode/issues/140 --- daemon/gamemode-tests.c | 161 ++++++++++++++++++++++++++++++++++++++++ 1 file changed, 161 insertions(+) diff --git a/daemon/gamemode-tests.c b/daemon/gamemode-tests.c index 2d4c54a..3702931 100644 --- a/daemon/gamemode-tests.c +++ b/daemon/gamemode-tests.c @@ -36,6 +36,8 @@ POSSIBILITY OF SUCH DAMAGE. #include "logging.h" #include +#include +#include #include #include #include @@ -536,6 +538,145 @@ int run_gpu_optimisation_tests(struct GameModeConfig *config) return gpustatus; } +/** + * Multithreaded process simulation + * + * Some of the optimisations that gamemode implements needs to be tested against a full process + * tree, otherwise we may only be applying them to only the main thread + */ +typedef struct { + pthread_barrier_t *barrier; + pid_t this; +} ThreadInfo; + +static void *fake_thread_wait(void *arg) +{ + ThreadInfo *info = (ThreadInfo *)arg; + + /* Store the thread ID */ + info->this = (pid_t)syscall(SYS_gettid); + + /** + * Wait twice + * First to sync that all threads have started + * Second to sync all threads exiting + */ + /* TODO: Error handle */ + pthread_barrier_wait(info->barrier); + pthread_barrier_wait(info->barrier); + + return NULL; +} + +/** + * Function to test a thread for specific attributes + * Arguments are: + * expected value + * the thread id + */ +typedef int (*testfunc)(int, pid_t); + +/* Runs a process tree in a child and tests each thread */ +static pid_t run_process_tree(int innactive, int active, testfunc func) +{ + /* Create a fake game-like multithreaded fork */ + pid_t child = fork(); + if (child == 0) { + /* Some stetup */ + const unsigned int numthreads = 3; + pthread_barrier_t barrier; + pthread_barrier_init(&barrier, NULL, numthreads + 1); + + /* First, request gamemode for this child process before it created the threads */ + gamemode_request_start(); + + /* Spawn a few child threads */ + pthread_t threads[numthreads]; + ThreadInfo info[numthreads]; + for (unsigned int i = 0; i < numthreads; i++) { + info[i].barrier = &barrier; + int err = pthread_create(&threads[i], NULL, fake_thread_wait, &info[i]); + if (err != 0) { + LOG_ERROR("Failed to spawn thread! Error: %d\n", err); + exit(EXIT_FAILURE); + } + } + + /* Wait for threads to be created */ + pthread_barrier_wait(&barrier); + + /* Test each spawned thread */ + int ret = 0; + for (unsigned int i = 0; i < numthreads; i++) + ret ^= func(active, info[i].this); + + if (ret != 0) { + LOG_ERROR("Initial ioprio values for new threads were incorrect!\n"); + gamemode_request_end(); + exit(ret); + } + + /* Request gamemode end */ + gamemode_request_end(); + + /* Test each spawned thread */ + for (unsigned int i = 0; i < numthreads; i++) + ret ^= func(innactive, info[i].this); + if (ret != 0) { + LOG_ERROR("Ioprio values for threads were not reset after gamemode_request_end!\n"); + exit(ret); + } + + /* Request gamemode again - this time after threads were created */ + gamemode_request_start(); + + /* Test each spawned thread */ + for (unsigned int i = 0; i < numthreads; i++) + ret ^= func(active, info[i].this); + if (ret != 0) { + LOG_ERROR("ioprio values for threads were not set correctly!\n"); + exit(ret); + } + + /* Request gamemode end */ + gamemode_request_end(); + + /* Test each spawned thread */ + for (unsigned int i = 0; i < numthreads; i++) + ret ^= func(innactive, info[i].this); + if (ret != 0) { + LOG_ERROR("Ioprio values for threads were not reset after gamemode_request_end!\n"); + exit(ret); + } + + /* Tell the threads to continue */ + pthread_barrier_wait(&barrier); + + /* Wait for threads to join */ + /* TODO: Error check */ + for (unsigned int i = 0; i < numthreads; i++) + ret &= pthread_join(threads[i], NULL); + + /* We're done, so return the error code generated */ + exit(ret); + } + + /* Wait for the child */ + int wstatus = 0; + waitpid(child, &wstatus, 0); + + int status = 0; + if (WIFEXITED(wstatus)) + status = WEXITSTATUS(wstatus); + else { + /* TODO: Gather some error information */ + LOG_ERROR("Multithreaded child exited abnormally!\n"); + status = -1; + } + + return status; +} + int run_renice_tests(struct GameModeConfig *config) { /* read configuration "renice" (1..20) */ @@ -579,6 +720,19 @@ int run_renice_tests(struct GameModeConfig *config) return ret; } +int test_ioprio(int expected, pid_t this) +{ + int val = game_mode_get_ioprio(this); + if (val != expected) { + LOG_ERROR("ioprio value was incorrect for thread %d!\nExpected: %d, Was: %d\n", + this, + expected, + val); + return -1; + } + return 0; +} + int run_ioprio_tests(struct GameModeConfig *config) { /* read configuration "ioprio" */ @@ -623,6 +777,13 @@ int run_ioprio_tests(struct GameModeConfig *config) ret = -1; } + /* Check multiprocess nice works as well */ + val = run_process_tree(IOPRIO_DEFAULT, (int)ioprio, test_ioprio); + if (val != 0) { + LOG_ERROR("Multithreaded ioprio tests failed!\n"); + ret = -1; + } + return ret; } From 6869470f9bd7c6558aca0895d30aa2889936e982 Mon Sep 17 00:00:00 2001 From: Marc Di Luzio Date: Sun, 12 May 2019 12:48:45 +0100 Subject: [PATCH 12/28] Fix feature status codes - failures here should be considered a failure --- daemon/gamemode-tests.c | 10 +++++----- 1 file changed, 5 insertions(+), 5 deletions(-) diff --git a/daemon/gamemode-tests.c b/daemon/gamemode-tests.c index 3702931..42d16ec 100644 --- a/daemon/gamemode-tests.c +++ b/daemon/gamemode-tests.c @@ -809,7 +809,7 @@ static int game_mode_run_feature_tests(struct GameModeConfig *config) else { LOG_MSG("::: Failed!\n"); // Consider the CPU governor feature required - status = 1; + status = -1; } } @@ -825,7 +825,7 @@ static int game_mode_run_feature_tests(struct GameModeConfig *config) else { LOG_MSG("::: Failed!\n"); // Any custom scripts should be expected to work - status = 1; + status = -1; } } @@ -841,7 +841,7 @@ static int game_mode_run_feature_tests(struct GameModeConfig *config) else { LOG_MSG("::: Failed!\n"); // Any custom scripts should be expected to work - status = 1; + status = -1; } } @@ -860,7 +860,7 @@ static int game_mode_run_feature_tests(struct GameModeConfig *config) else { LOG_MSG("::: Failed!\n"); // Renice should be expected to work, if set - status = 1; + status = -1; } } @@ -876,7 +876,7 @@ static int game_mode_run_feature_tests(struct GameModeConfig *config) else { LOG_MSG("::: Failed!\n"); // Ioprio should be expected to work, if set - status = 1; + status = -1; } } From 6639b17500aa430e438688a6e820e9619c7e8dcd Mon Sep 17 00:00:00 2001 From: Marc Di Luzio Date: Sun, 12 May 2019 12:59:59 +0100 Subject: [PATCH 13/28] Make the multithreaded ioprio tests not cause a full failure --- daemon/gamemode-tests.c | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/daemon/gamemode-tests.c b/daemon/gamemode-tests.c index 42d16ec..883f030 100644 --- a/daemon/gamemode-tests.c +++ b/daemon/gamemode-tests.c @@ -874,9 +874,9 @@ static int game_mode_run_feature_tests(struct GameModeConfig *config) else if (iopriostatus == 0) LOG_MSG("::: Passed\n"); else { - LOG_MSG("::: Failed!\n"); - // Ioprio should be expected to work, if set - status = -1; + LOG_MSG( + "::: Failed! (known: https://github.com/FeralInteractive/gamemode/issues/140)\n"); + status = 1; } } From bf2b057915e2119ee6fdebf80a484806c66204e8 Mon Sep 17 00:00:00 2001 From: Marc Di Luzio Date: Sun, 12 May 2019 19:53:16 +0100 Subject: [PATCH 14/28] First pass at fix for ioprio not applying to process tree properly See https://github.com/FeralInteractive/gamemode/issues/140 --- daemon/gamemode-ioprio.c | 67 +++++++++++++++++++++++++++------------- 1 file changed, 46 insertions(+), 21 deletions(-) diff --git a/daemon/gamemode-ioprio.c b/daemon/gamemode-ioprio.c index 544be9f..fd85904 100644 --- a/daemon/gamemode-ioprio.c +++ b/daemon/gamemode-ioprio.c @@ -36,7 +36,9 @@ POSSIBILITY OF SUCH DAMAGE. #include "helpers.h" #include "logging.h" +#include #include +#include #include #include @@ -135,28 +137,51 @@ void game_mode_apply_ioprio(const GameModeContext *self, const pid_t client, int ioprio = IOPRIO_DEFAULT; } - int current = game_mode_get_ioprio(client); - if (current == IOPRIO_DONT_SET) { - /* Couldn't get the ioprio value, let's bail */ + /* Open the tasks dir for the client */ + char tasks[128]; + snprintf(tasks, sizeof(tasks), "/proc/%d/task", client); + DIR *client_task_dir = opendir(tasks); + if (client_task_dir == NULL) { + LOG_ERROR("Could not inspect tasks for client %d! Skipping ioprio optimisation.\n", client); return; - } else if (current != expected) { - /* Don't try and adjust the ioprio value if the value we got doesn't match default */ - LOG_ERROR("Refused to set ioprio on client [%d]: prio was (%d) but we expected (%d)\n", - client, - current, - expected); - } else { - /* - * For now we only support IOPRIO_CLASS_BE - * IOPRIO_CLASS_RT requires CAP_SYS_ADMIN but should be possible with a polkit process - */ - int p = ioprio; - ioprio = IOPRIO_PRIO_VALUE(IOPRIO_CLASS_BE, ioprio); - if (ioprio_set(IOPRIO_WHO_PROCESS, client, ioprio) != 0) { - LOG_ERROR("Setting client [%d] IO priority to (%d) failed with error %d, ignoring\n", - client, - p, - errno); + } + + /* Iterate for all tasks of client process */ + struct dirent *tid_entry; + while ((tid_entry = readdir(client_task_dir)) != NULL) { + /* Skip . and .. */ + if (tid_entry->d_name[0] == '.') + continue; + + /* task name is the name of the file */ + int tid = atoi(tid_entry->d_name); + + int current = game_mode_get_ioprio(tid); + if (current == IOPRIO_DONT_SET) { + /* Couldn't get the ioprio value + * This could simply mean that the thread exited before fetching the ioprio + * So we should continue + */ + } else if (current != expected) { + /* Don't try and adjust the ioprio value if the value we got doesn't match default */ + LOG_ERROR("Refused to set ioprio on client [%d]: prio was (%d) but we expected (%d)\n", + tid, + current, + expected); + } else { + /* + * For now we only support IOPRIO_CLASS_BE + * IOPRIO_CLASS_RT requires CAP_SYS_ADMIN but should be possible with a polkit process + */ + int p = ioprio; + ioprio = IOPRIO_PRIO_VALUE(IOPRIO_CLASS_BE, ioprio); + if (ioprio_set(IOPRIO_WHO_PROCESS, tid, ioprio) != 0) { + LOG_ERROR( + "Setting client [%d] IO priority to (%d) failed with error %d, ignoring\n", + tid, + p, + errno); + } } } } From 38e48a2d8ebc2cdbebb7c9f5f6dd6e34321b9c35 Mon Sep 17 00:00:00 2001 From: Marc Di Luzio Date: Sun, 12 May 2019 19:56:20 +0100 Subject: [PATCH 15/28] Update logging to reflect thread, client relationship --- daemon/gamemode-ioprio.c | 10 +++++++--- 1 file changed, 7 insertions(+), 3 deletions(-) diff --git a/daemon/gamemode-ioprio.c b/daemon/gamemode-ioprio.c index fd85904..620e604 100644 --- a/daemon/gamemode-ioprio.c +++ b/daemon/gamemode-ioprio.c @@ -142,7 +142,8 @@ void game_mode_apply_ioprio(const GameModeContext *self, const pid_t client, int snprintf(tasks, sizeof(tasks), "/proc/%d/task", client); DIR *client_task_dir = opendir(tasks); if (client_task_dir == NULL) { - LOG_ERROR("Could not inspect tasks for client %d! Skipping ioprio optimisation.\n", client); + LOG_ERROR("Could not inspect tasks for client [%d]! Skipping ioprio optimisation.\n", + client); return; } @@ -164,7 +165,8 @@ void game_mode_apply_ioprio(const GameModeContext *self, const pid_t client, int */ } else if (current != expected) { /* Don't try and adjust the ioprio value if the value we got doesn't match default */ - LOG_ERROR("Refused to set ioprio on client [%d]: prio was (%d) but we expected (%d)\n", + LOG_ERROR("Skipping ioprio on client [%d,%d]: ioprio was (%d) but we expected (%d)\n", + client, tid, current, expected); @@ -176,8 +178,10 @@ void game_mode_apply_ioprio(const GameModeContext *self, const pid_t client, int int p = ioprio; ioprio = IOPRIO_PRIO_VALUE(IOPRIO_CLASS_BE, ioprio); if (ioprio_set(IOPRIO_WHO_PROCESS, tid, ioprio) != 0) { + /* This could simply mean the thread is gone now, as above */ LOG_ERROR( - "Setting client [%d] IO priority to (%d) failed with error %d, ignoring\n", + "Setting client [%d,%d] IO priority to (%d) failed with error %d, ignoring.\n", + client, tid, p, errno); From 87cfd9c5a60a04bd0854f0a7311fd13554a0ba3a Mon Sep 17 00:00:00 2001 From: Marc Di Luzio Date: Sun, 12 May 2019 20:19:19 +0100 Subject: [PATCH 16/28] Remove error log about known issue now that it is fixed --- daemon/gamemode-tests.c | 5 ++--- 1 file changed, 2 insertions(+), 3 deletions(-) diff --git a/daemon/gamemode-tests.c b/daemon/gamemode-tests.c index 883f030..61e63aa 100644 --- a/daemon/gamemode-tests.c +++ b/daemon/gamemode-tests.c @@ -874,9 +874,8 @@ static int game_mode_run_feature_tests(struct GameModeConfig *config) else if (iopriostatus == 0) LOG_MSG("::: Passed\n"); else { - LOG_MSG( - "::: Failed! (known: https://github.com/FeralInteractive/gamemode/issues/140)\n"); - status = 1; + LOG_MSG("::: Failed!\n"); + status = -1; } } From 3ac49385dccaf6b7f04e220da922c7f2b2fc0914 Mon Sep 17 00:00:00 2001 From: Marc Di Luzio Date: Sun, 12 May 2019 20:20:06 +0100 Subject: [PATCH 17/28] Add a test for renicing multithreaded processes --- daemon/gamemode-tests.c | 24 ++++++++++++++++++++++-- 1 file changed, 22 insertions(+), 2 deletions(-) diff --git a/daemon/gamemode-tests.c b/daemon/gamemode-tests.c index 61e63aa..31becb6 100644 --- a/daemon/gamemode-tests.c +++ b/daemon/gamemode-tests.c @@ -677,6 +677,19 @@ static pid_t run_process_tree(int innactive, int active, testfunc func) return status; } +int test_renice(int expected, pid_t this) +{ + int val = game_mode_get_renice(this); + if (val != expected) { + LOG_ERROR("nice value was incorrect for thread %d!\nExpected: %d, Was: %d\n", + this, + expected, + val); + return -1; + } + return 0; +} + int run_renice_tests(struct GameModeConfig *config) { /* read configuration "renice" (1..20) */ @@ -717,6 +730,13 @@ int run_renice_tests(struct GameModeConfig *config) ret = -1; } + /* Check multiprocess nice works as well */ + val = run_process_tree(0, (int)renice, test_renice); + if (val != 0) { + LOG_ERROR("Multithreaded renice tests failed!\n"); + ret = -1; + } + return ret; } @@ -858,9 +878,9 @@ static int game_mode_run_feature_tests(struct GameModeConfig *config) else if (renicestatus == 0) LOG_MSG("::: Passed\n"); else { - LOG_MSG("::: Failed!\n"); + LOG_MSG("::: Failed! (non-fatal, known issue with multithreaded programs)\n"); // Renice should be expected to work, if set - status = -1; + status = 1; } } From f152ea93381efd75007190030916efc2513ae41d Mon Sep 17 00:00:00 2001 From: Marc Di Luzio Date: Sun, 12 May 2019 20:27:00 +0100 Subject: [PATCH 18/28] Initial implementation of applying renice to all process threads --- daemon/gamemode-sched.c | 73 +++++++++++++++++++++++++++-------------- 1 file changed, 49 insertions(+), 24 deletions(-) diff --git a/daemon/gamemode-sched.c b/daemon/gamemode-sched.c index e4f2019..9969615 100644 --- a/daemon/gamemode-sched.c +++ b/daemon/gamemode-sched.c @@ -35,8 +35,10 @@ POSSIBILITY OF SUCH DAMAGE. #include "gamemode.h" #include "logging.h" +#include #include #include +#include #include #include #include @@ -96,30 +98,53 @@ void game_mode_apply_renice(const GameModeContext *self, const pid_t client, int renice = 0; } - /* Clear errno as -1 is a regitimate return */ - errno = 0; - int prio = getpriority(PRIO_PROCESS, (id_t)client); - if (prio == -1 && errno) { - /* process has likely ended, only log an error if we were actually trying to set a non-zero - * value */ - if (errno == ESRCH && renice != 0) - LOG_ERROR("getpriority returned ESRCH for process %d\n", client); - } else if (prio != expected) { - /* - * Don't adjust priority if it does not match the expected value - * ie. Another process has changed it, or it began non-standard - */ - LOG_ERROR("Refused to renice client [%d]: prio was (%d) but we expected (%d)\n", - client, - prio, - expected); - } else if (setpriority(PRIO_PROCESS, (id_t)client, (int)renice)) { - LOG_HINTED(ERROR, - "Failed to renice client [%d], ignoring error condition: %s\n", - " -- Your user may not have permission to do this. Please read the docs\n" - " -- to learn how to adjust the pam limits.\n", - client, - strerror(errno)); + /* Open the tasks dir for the client */ + char tasks[128]; + snprintf(tasks, sizeof(tasks), "/proc/%d/task", client); + DIR *client_task_dir = opendir(tasks); + if (client_task_dir == NULL) { + LOG_ERROR("Could not inspect tasks for client [%d]! Skipping ioprio optimisation.\n", + client); + return; + } + + /* Iterate for all tasks of client process */ + struct dirent *tid_entry; + while ((tid_entry = readdir(client_task_dir)) != NULL) { + /* Skip . and .. */ + if (tid_entry->d_name[0] == '.') + continue; + + /* task name is the name of the file */ + int tid = atoi(tid_entry->d_name); + + /* Clear errno as -1 is a regitimate return */ + errno = 0; + int prio = getpriority(PRIO_PROCESS, (id_t)tid); + if (prio == -1 && errno) { + /* process has likely ended, only log an error if we were actually trying to set a + * non-zero value */ + if (errno == ESRCH && renice != 0) + LOG_ERROR("getpriority returned ESRCH for process %d\n", tid); + } else if (prio != expected) { + /* + * Don't adjust priority if it does not match the expected value + * ie. Another process has changed it, or it began non-standard + */ + LOG_ERROR("Refused to renice client [%d,%d]: prio was (%d) but we expected (%d)\n", + client, + tid, + prio, + expected); + } else if (setpriority(PRIO_PROCESS, (id_t)client, (int)renice)) { + LOG_HINTED(ERROR, + "Failed to renice client [%d,%d], ignoring error condition: %s\n", + " -- Your user may not have permission to do this. Please read the docs\n" + " -- to learn how to adjust the pam limits.\n", + client, + tid, + strerror(errno)); + } } } From 16c932f5d079ab2e0191f651b5d10da65c95ee40 Mon Sep 17 00:00:00 2001 From: Marc Di Luzio Date: Mon, 13 May 2019 18:12:12 +0100 Subject: [PATCH 19/28] Adjust logs for generic process test function --- daemon/gamemode-tests.c | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-) diff --git a/daemon/gamemode-tests.c b/daemon/gamemode-tests.c index 31becb6..ef5fd07 100644 --- a/daemon/gamemode-tests.c +++ b/daemon/gamemode-tests.c @@ -611,7 +611,7 @@ static pid_t run_process_tree(int innactive, int active, testfunc func) ret ^= func(active, info[i].this); if (ret != 0) { - LOG_ERROR("Initial ioprio values for new threads were incorrect!\n"); + LOG_ERROR("Initial values for new threads were incorrect!\n"); gamemode_request_end(); exit(ret); } @@ -623,7 +623,7 @@ static pid_t run_process_tree(int innactive, int active, testfunc func) for (unsigned int i = 0; i < numthreads; i++) ret ^= func(innactive, info[i].this); if (ret != 0) { - LOG_ERROR("Ioprio values for threads were not reset after gamemode_request_end!\n"); + LOG_ERROR("values for threads were not reset after gamemode_request_end!\n"); exit(ret); } @@ -634,7 +634,7 @@ static pid_t run_process_tree(int innactive, int active, testfunc func) for (unsigned int i = 0; i < numthreads; i++) ret ^= func(active, info[i].this); if (ret != 0) { - LOG_ERROR("ioprio values for threads were not set correctly!\n"); + LOG_ERROR("values for threads were not set correctly!\n"); exit(ret); } @@ -645,7 +645,7 @@ static pid_t run_process_tree(int innactive, int active, testfunc func) for (unsigned int i = 0; i < numthreads; i++) ret ^= func(innactive, info[i].this); if (ret != 0) { - LOG_ERROR("Ioprio values for threads were not reset after gamemode_request_end!\n"); + LOG_ERROR("values for threads were not reset after gamemode_request_end!\n"); exit(ret); } From 2d19b61a385ff9e2218aacb1109f95962dc9397e Mon Sep 17 00:00:00 2001 From: Marc Di Luzio Date: Mon, 13 May 2019 18:43:11 +0100 Subject: [PATCH 20/28] Always log when getpriority fails - we will likely want to know --- daemon/gamemode-sched.c | 10 ++++++---- 1 file changed, 6 insertions(+), 4 deletions(-) diff --git a/daemon/gamemode-sched.c b/daemon/gamemode-sched.c index 9969615..156c9f3 100644 --- a/daemon/gamemode-sched.c +++ b/daemon/gamemode-sched.c @@ -121,11 +121,13 @@ void game_mode_apply_renice(const GameModeContext *self, const pid_t client, int /* Clear errno as -1 is a regitimate return */ errno = 0; int prio = getpriority(PRIO_PROCESS, (id_t)tid); + if (prio == -1 && errno) { - /* process has likely ended, only log an error if we were actually trying to set a - * non-zero value */ - if (errno == ESRCH && renice != 0) - LOG_ERROR("getpriority returned ESRCH for process %d\n", tid); + /* Process may well have ended */ + LOG_ERROR("getpriority failed for client [%d,%d] with error: %s\n", + client, + tid, + strerror(errno)); } else if (prio != expected) { /* * Don't adjust priority if it does not match the expected value From f00a89bc567219083c07a5bfdc8ccbf3c4aa8971 Mon Sep 17 00:00:00 2001 From: Marc Di Luzio Date: Mon, 13 May 2019 18:43:28 +0100 Subject: [PATCH 21/28] Actually apply setpriority to the thread not the client --- daemon/gamemode-sched.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/daemon/gamemode-sched.c b/daemon/gamemode-sched.c index 156c9f3..0414b96 100644 --- a/daemon/gamemode-sched.c +++ b/daemon/gamemode-sched.c @@ -138,7 +138,7 @@ void game_mode_apply_renice(const GameModeContext *self, const pid_t client, int tid, prio, expected); - } else if (setpriority(PRIO_PROCESS, (id_t)client, (int)renice)) { + } else if (setpriority(PRIO_PROCESS, (id_t)tid, (int)renice)) { LOG_HINTED(ERROR, "Failed to renice client [%d,%d], ignoring error condition: %s\n", " -- Your user may not have permission to do this. Please read the docs\n" From 4091a0c532bc048c28349e54cdb7aed19bc5c37d Mon Sep 17 00:00:00 2001 From: Marc Di Luzio Date: Mon, 13 May 2019 18:43:49 +0100 Subject: [PATCH 22/28] Now that renice works - set it to a hard fail again Also move some comments around --- daemon/gamemode-tests.c | 13 +++++-------- 1 file changed, 5 insertions(+), 8 deletions(-) diff --git a/daemon/gamemode-tests.c b/daemon/gamemode-tests.c index ef5fd07..6cad733 100644 --- a/daemon/gamemode-tests.c +++ b/daemon/gamemode-tests.c @@ -865,9 +865,6 @@ static int game_mode_run_feature_tests(struct GameModeConfig *config) } } - /* Does the screensaver get inhibited? */ - /* TODO: Unknown if this is testable, org.freedesktop.ScreenSaver has no query method */ - /* Was the process reniced? */ { LOG_MSG("::: Verifying renice\n"); @@ -878,9 +875,9 @@ static int game_mode_run_feature_tests(struct GameModeConfig *config) else if (renicestatus == 0) LOG_MSG("::: Passed\n"); else { - LOG_MSG("::: Failed! (non-fatal, known issue with multithreaded programs)\n"); + LOG_MSG("::: Failed!\n"); // Renice should be expected to work, if set - status = 1; + status = -1; } } @@ -899,10 +896,10 @@ static int game_mode_run_feature_tests(struct GameModeConfig *config) } } - /* Was the scheduling applied? */ - /* Were io priorities changed? */ - /* Note: These don't get cleared up on un-register, so will have already been applied */ /* TODO */ + /* Was the scheduling applied and removed? Does it get applied to a full process tree? */ + /* Does the screensaver get inhibited? Unknown if this is testable, org.freedesktop.ScreenSaver + * has no query method */ if (status != -1) LOG_MSG(":: Passed%s\n\n", status > 0 ? " (with optional failures)" : ""); From 139b90e0b7d70f1e55e20c011dcb20d196400c28 Mon Sep 17 00:00:00 2001 From: Marc Di Luzio Date: Mon, 13 May 2019 18:51:34 +0100 Subject: [PATCH 23/28] Refactor the process test function Make it simply take a functor to the per tid get method --- daemon/gamemode-tests.c | 67 +++++++++++------------------------------ 1 file changed, 17 insertions(+), 50 deletions(-) diff --git a/daemon/gamemode-tests.c b/daemon/gamemode-tests.c index 6cad733..8e0ed4f 100644 --- a/daemon/gamemode-tests.c +++ b/daemon/gamemode-tests.c @@ -568,21 +568,14 @@ static void *fake_thread_wait(void *arg) return NULL; } -/** - * Function to test a thread for specific attributes - * Arguments are: - * expected value - * the thread id - */ -typedef int (*testfunc)(int, pid_t); - /* Runs a process tree in a child and tests each thread */ -static pid_t run_process_tree(int innactive, int active, testfunc func) +static pid_t run_tests_on_process_tree(int innactive, int active, int (*func)(pid_t)) { /* Create a fake game-like multithreaded fork */ pid_t child = fork(); if (child == 0) { /* Some stetup */ + bool fail = false; const unsigned int numthreads = 3; pthread_barrier_t barrier; pthread_barrier_init(&barrier, NULL, numthreads + 1); @@ -606,14 +599,13 @@ static pid_t run_process_tree(int innactive, int active, testfunc func) pthread_barrier_wait(&barrier); /* Test each spawned thread */ - int ret = 0; for (unsigned int i = 0; i < numthreads; i++) - ret ^= func(active, info[i].this); + fail |= (active != func(info[i].this)); - if (ret != 0) { + if (fail) { LOG_ERROR("Initial values for new threads were incorrect!\n"); gamemode_request_end(); - exit(ret); + exit(-1); } /* Request gamemode end */ @@ -621,10 +613,10 @@ static pid_t run_process_tree(int innactive, int active, testfunc func) /* Test each spawned thread */ for (unsigned int i = 0; i < numthreads; i++) - ret ^= func(innactive, info[i].this); - if (ret != 0) { + fail |= (innactive != func(info[i].this)); + if (fail) { LOG_ERROR("values for threads were not reset after gamemode_request_end!\n"); - exit(ret); + exit(-1); } /* Request gamemode again - this time after threads were created */ @@ -632,10 +624,10 @@ static pid_t run_process_tree(int innactive, int active, testfunc func) /* Test each spawned thread */ for (unsigned int i = 0; i < numthreads; i++) - ret ^= func(active, info[i].this); - if (ret != 0) { + fail |= (active != func(info[i].this)); + if (fail) { LOG_ERROR("values for threads were not set correctly!\n"); - exit(ret); + exit(-1); } /* Request gamemode end */ @@ -643,10 +635,10 @@ static pid_t run_process_tree(int innactive, int active, testfunc func) /* Test each spawned thread */ for (unsigned int i = 0; i < numthreads; i++) - ret ^= func(innactive, info[i].this); - if (ret != 0) { + fail |= (innactive != func(info[i].this)); + if (fail) { LOG_ERROR("values for threads were not reset after gamemode_request_end!\n"); - exit(ret); + exit(-1); } /* Tell the threads to continue */ @@ -654,6 +646,7 @@ static pid_t run_process_tree(int innactive, int active, testfunc func) /* Wait for threads to join */ /* TODO: Error check */ + int ret = 0; for (unsigned int i = 0; i < numthreads; i++) ret &= pthread_join(threads[i], NULL); @@ -677,19 +670,6 @@ static pid_t run_process_tree(int innactive, int active, testfunc func) return status; } -int test_renice(int expected, pid_t this) -{ - int val = game_mode_get_renice(this); - if (val != expected) { - LOG_ERROR("nice value was incorrect for thread %d!\nExpected: %d, Was: %d\n", - this, - expected, - val); - return -1; - } - return 0; -} - int run_renice_tests(struct GameModeConfig *config) { /* read configuration "renice" (1..20) */ @@ -731,7 +711,7 @@ int run_renice_tests(struct GameModeConfig *config) } /* Check multiprocess nice works as well */ - val = run_process_tree(0, (int)renice, test_renice); + val = run_tests_on_process_tree(0, (int)renice, game_mode_get_renice); if (val != 0) { LOG_ERROR("Multithreaded renice tests failed!\n"); ret = -1; @@ -740,19 +720,6 @@ int run_renice_tests(struct GameModeConfig *config) return ret; } -int test_ioprio(int expected, pid_t this) -{ - int val = game_mode_get_ioprio(this); - if (val != expected) { - LOG_ERROR("ioprio value was incorrect for thread %d!\nExpected: %d, Was: %d\n", - this, - expected, - val); - return -1; - } - return 0; -} - int run_ioprio_tests(struct GameModeConfig *config) { /* read configuration "ioprio" */ @@ -798,7 +765,7 @@ int run_ioprio_tests(struct GameModeConfig *config) } /* Check multiprocess nice works as well */ - val = run_process_tree(IOPRIO_DEFAULT, (int)ioprio, test_ioprio); + val = run_tests_on_process_tree(IOPRIO_DEFAULT, (int)ioprio, game_mode_get_ioprio); if (val != 0) { LOG_ERROR("Multithreaded ioprio tests failed!\n"); ret = -1; From ef29b35258ec3382981cd3acc1442f9494a8019a Mon Sep 17 00:00:00 2001 From: Marc Di Luzio Date: Mon, 13 May 2019 18:52:38 +0100 Subject: [PATCH 24/28] Make sure we don't leave a hanging gamemode request from the child process Otherwise other tests might have to wait for the reaper thread to clean it up --- daemon/gamemode-tests.c | 1 + 1 file changed, 1 insertion(+) diff --git a/daemon/gamemode-tests.c b/daemon/gamemode-tests.c index 8e0ed4f..3129935 100644 --- a/daemon/gamemode-tests.c +++ b/daemon/gamemode-tests.c @@ -627,6 +627,7 @@ static pid_t run_tests_on_process_tree(int innactive, int active, int (*func)(pi fail |= (active != func(info[i].this)); if (fail) { LOG_ERROR("values for threads were not set correctly!\n"); + gamemode_request_end(); exit(-1); } From 0d018d91a863c91d1d1ade938603b707073eef4d Mon Sep 17 00:00:00 2001 From: Marc Di Luzio Date: Mon, 13 May 2019 18:58:45 +0100 Subject: [PATCH 25/28] Implement some error logging/checking and clean up some TODO comments --- daemon/gamemode-tests.c | 17 +++++++++++------ 1 file changed, 11 insertions(+), 6 deletions(-) diff --git a/daemon/gamemode-tests.c b/daemon/gamemode-tests.c index 3129935..086acc6 100644 --- a/daemon/gamemode-tests.c +++ b/daemon/gamemode-tests.c @@ -561,9 +561,14 @@ static void *fake_thread_wait(void *arg) * First to sync that all threads have started * Second to sync all threads exiting */ - /* TODO: Error handle */ - pthread_barrier_wait(info->barrier); - pthread_barrier_wait(info->barrier); + int ret = 0; + ret = pthread_barrier_wait(info->barrier); + if (ret != 0 && ret != PTHREAD_BARRIER_SERIAL_THREAD) + FATAL_ERROR("pthread_barrier_wait failed in child with error %d!\n", ret); + + ret = pthread_barrier_wait(info->barrier); + if (ret != 0 && ret != PTHREAD_BARRIER_SERIAL_THREAD) + FATAL_ERROR("pthread_barrier_wait failed in child with error %d!\n", ret); return NULL; } @@ -646,11 +651,13 @@ static pid_t run_tests_on_process_tree(int innactive, int active, int (*func)(pi pthread_barrier_wait(&barrier); /* Wait for threads to join */ - /* TODO: Error check */ int ret = 0; for (unsigned int i = 0; i < numthreads; i++) ret &= pthread_join(threads[i], NULL); + if (ret != 0) + LOG_ERROR("Thread cleanup in multithreaded tests failed!\n"); + /* We're done, so return the error code generated */ exit(ret); } @@ -663,7 +670,6 @@ static pid_t run_tests_on_process_tree(int innactive, int active, int (*func)(pi if (WIFEXITED(wstatus)) status = WEXITSTATUS(wstatus); else { - /* TODO: Gather some error information */ LOG_ERROR("Multithreaded child exited abnormally!\n"); status = -1; } @@ -993,7 +999,6 @@ int game_mode_run_client_tests(void) return -1; /* Controls whether we require a supervisor to actually make requests */ - /* TODO: This effects all tests below */ if (config_get_require_supervisor(config) != 0) { LOG_ERROR("Tests currently unsupported when require_supervisor is set\n"); return -1; From 41d35fa12a03fe5ec65ff6dc181e39c313a9d5d0 Mon Sep 17 00:00:00 2001 From: Marc Di Luzio Date: Wed, 15 May 2019 18:17:50 +0100 Subject: [PATCH 26/28] Add a zeroed final option for getopt_long for correctness --- daemon/main.c | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/daemon/main.c b/daemon/main.c index 525fce3..a66f409 100644 --- a/daemon/main.c +++ b/daemon/main.c @@ -114,7 +114,8 @@ int main(int argc, char *argv[]) { "test", no_argument, 0, 't' }, { "status", optional_argument, 0, 's' }, { "help", no_argument, 0, 'h' }, - { "version", no_argument, 0, 'v' } }; + { "version", no_argument, 0, 'v' }, + { NULL, 0, NULL, 0 }, }; static const char *short_options = "dls::r::tvh"; while ((opt = getopt_long(argc, argv, short_options, long_options, 0)) != -1) { From f401b49b1ac52f531ae066c5fe0b0386ce4be8a2 Mon Sep 17 00:00:00 2001 From: Marc Di Luzio Date: Wed, 15 May 2019 18:20:36 +0100 Subject: [PATCH 27/28] Close the openned task dirs to prevent a leak --- daemon/gamemode-ioprio.c | 2 ++ daemon/gamemode-sched.c | 2 ++ 2 files changed, 4 insertions(+) diff --git a/daemon/gamemode-ioprio.c b/daemon/gamemode-ioprio.c index 620e604..6af6779 100644 --- a/daemon/gamemode-ioprio.c +++ b/daemon/gamemode-ioprio.c @@ -188,4 +188,6 @@ void game_mode_apply_ioprio(const GameModeContext *self, const pid_t client, int } } } + + closedir(client_task_dir); } diff --git a/daemon/gamemode-sched.c b/daemon/gamemode-sched.c index 0414b96..2c82d64 100644 --- a/daemon/gamemode-sched.c +++ b/daemon/gamemode-sched.c @@ -148,6 +148,8 @@ void game_mode_apply_renice(const GameModeContext *self, const pid_t client, int strerror(errno)); } } + + closedir(client_task_dir); } void game_mode_apply_scheduling(const GameModeContext *self, const pid_t client) From a27e741beb5316ef4b3747099c16eefb821f0c25 Mon Sep 17 00:00:00 2001 From: Marc Di Luzio Date: Wed, 15 May 2019 18:43:42 +0100 Subject: [PATCH 28/28] Reformat long_options now that travis matches --- daemon/main.c | 14 ++++++-------- 1 file changed, 6 insertions(+), 8 deletions(-) diff --git a/daemon/main.c b/daemon/main.c index a66f409..f0910e3 100644 --- a/daemon/main.c +++ b/daemon/main.c @@ -108,14 +108,12 @@ int main(int argc, char *argv[]) int opt = 0; /* Options struct for getopt_long */ - static struct option long_options[] = { { "daemonize", no_argument, 0, 'd' }, - { "log-to-syslog", no_argument, 0, 'l' }, - { "request", optional_argument, 0, 'r' }, - { "test", no_argument, 0, 't' }, - { "status", optional_argument, 0, 's' }, - { "help", no_argument, 0, 'h' }, - { "version", no_argument, 0, 'v' }, - { NULL, 0, NULL, 0 }, }; + static struct option long_options[] = { + { "daemonize", no_argument, 0, 'd' }, { "log-to-syslog", no_argument, 0, 'l' }, + { "request", optional_argument, 0, 'r' }, { "test", no_argument, 0, 't' }, + { "status", optional_argument, 0, 's' }, { "help", no_argument, 0, 'h' }, + { "version", no_argument, 0, 'v' }, { NULL, 0, NULL, 0 }, + }; static const char *short_options = "dls::r::tvh"; while ((opt = getopt_long(argc, argv, short_options, long_options, 0)) != -1) {