Merge pull request #128 from mgerstner/gpuclockctl_fixes

Gpuclockctl fixes

daemon/daemon_config.c

@@ -362,6 +362,7 @@ static void load_config_files(GameModeConfig *self)
 				if (error) {
 					LOG_MSG("Failed to parse config file - error on line %d!\n", error);
 				}
+				fclose(f);
 			}
 			free(path);
 		}

daemon/external-helper.c

@@ -36,11 +36,75 @@ POSSIBILITY OF SUCH DAMAGE.
 
 #include <linux/limits.h>
 #include <stdio.h>
+#include <sys/time.h>
+#include <sys/types.h>
 #include <sys/wait.h>
-#include <time.h>
 #include <unistd.h>
 
-static const int default_timeout = 5;
+static const int DEFAULT_TIMEOUT = 5;
+
+static int read_child_stdout(int pipe_fd, char buffer[EXTERNAL_BUFFER_MAX], int tsec)
+{
+	fd_set fds;
+	struct timeval timeout;
+	int num_readable = 0;
+	ssize_t buffer_bytes_read = 0;
+	ssize_t just_read = 0;
+	bool buffer_full = false;
+	char discard_buffer[EXTERNAL_BUFFER_MAX];
+
+	/* Set up the timout */
+	timeout.tv_sec = tsec;
+	timeout.tv_usec = 0;
+
+	FD_ZERO(&fds);
+
+	/* Wait for the child to finish up with a timout */
+	while (true) {
+		FD_SET(pipe_fd, &fds);
+		num_readable = select(pipe_fd + 1, &fds, NULL, NULL, &timeout);
+
+		if (num_readable < 0) {
+			if (errno == EINTR) {
+				continue;
+			} else {
+				LOG_ERROR("sigtimedwait failed: %s\n", strerror(errno));
+				return -1;
+			}
+		} else if (num_readable == 0) {
+			return -2;
+		}
+
+		if (!buffer_full) {
+			just_read = read(pipe_fd,
+			                 buffer + buffer_bytes_read,
+			                 EXTERNAL_BUFFER_MAX - (size_t)buffer_bytes_read - 1);
+		} else {
+			just_read = read(pipe_fd, discard_buffer, EXTERNAL_BUFFER_MAX - 1);
+		}
+
+		if (just_read < 0) {
+			return -1;
+		} else if (just_read == 0) {
+			// EOF encountered
+			break;
+		}
+
+		if (!buffer_full) {
+			buffer_bytes_read += just_read;
+
+			if (buffer_bytes_read == EXTERNAL_BUFFER_MAX - 1) {
+				// our buffer is exhausted, discard the rest
+				// of the output
+				buffer_full = true;
+			}
+		}
+	}
+
+	buffer[buffer_bytes_read] = 0;
+
+	return 0;
+}
 
 /**
  * Call an external process
@@ -50,6 +114,7 @@ int run_external_process(const char *const *exec_args, char buffer[EXTERNAL_BUFF
 	pid_t p;
 	int status = 0;
 	int pipes[2];
+	int ret = 0;
 	char internal[EXTERNAL_BUFFER_MAX] = { 0 };
 
 	if (pipe(pipes) == -1) {
@@ -59,20 +124,12 @@ int run_external_process(const char *const *exec_args, char buffer[EXTERNAL_BUFF
 
 	/* Set the default timeout */
 	if (tsec == -1) {
-		tsec = default_timeout;
-	}
-
-	/* set up our signaling for the child and the timout */
-	sigset_t mask;
-	sigset_t omask;
-	sigemptyset(&mask);
-	sigaddset(&mask, SIGCHLD);
-	if (sigprocmask(SIG_BLOCK, &mask, &omask) < 0) {
-		LOG_ERROR("sigprocmask failed: %s\n", strerror(errno));
-		return -1;
+		tsec = DEFAULT_TIMEOUT;
 	}
 
 	if ((p = fork()) < 0) {
+		close(pipes[0]);
+		close(pipes[1]);
 		LOG_ERROR("Failed to fork(): %s\n", strerror(errno));
 		return false;
 	} else if (p == 0) {
@@ -91,39 +148,31 @@ int run_external_process(const char *const *exec_args, char buffer[EXTERNAL_BUFF
 			LOG_ERROR("Failed to execute external process: %s %s\n", exec_args[0], strerror(errno));
 			exit(EXIT_FAILURE);
 		}
-		_exit(EXIT_SUCCESS);
-	}
-
-	/* Set up the timout */
-	struct timespec timeout;
-	timeout.tv_sec = tsec;
-	timeout.tv_nsec = 0;
-
-	/* Wait for the child to finish up with a timout */
-	while (true) {
-		if (sigtimedwait(&mask, NULL, &timeout) < 0) {
-			if (errno == EINTR) {
-				continue;
-			} else if (errno == EAGAIN) {
-				LOG_ERROR("Child process timed out for %s, killing and returning\n", exec_args[0]);
-				kill(p, SIGKILL);
-			} else {
-				LOG_ERROR("sigtimedwait failed: %s\n", strerror(errno));
-				return -1;
-			}
-		}
-		break;
+		// should never be reached
+		abort();
 	}
 
+	// close the write end of the pipe so we get signaled EOF once the
+	// child exits
 	close(pipes[1]);
-	ssize_t output_size = read(pipes[0], internal, EXTERNAL_BUFFER_MAX - 1);
-	if (output_size < 0) {
-		LOG_ERROR("Failed to read from process %s: %s\n", exec_args[0], strerror(errno));
-		return -1;
+	ret = read_child_stdout(pipes[0], internal, tsec);
+	close(pipes[0]);
+
+	if (ret != 0) {
+		if (ret == -2) {
+			LOG_ERROR("Child process timed out for %s, killing and returning\n", exec_args[0]);
+			kill(p, SIGKILL);
+		} else {
+			LOG_ERROR("Failed to read from process %s: %s\n", exec_args[0], strerror(errno));
+		}
+		if (buffer) {
+			// make sure the buffer is a terminated empty string on error
+			buffer[0] = 0;
+		}
+	} else if (buffer) {
+		memcpy(buffer, internal, EXTERNAL_BUFFER_MAX);
 	}
 
-	internal[output_size] = 0;
-
 	if (waitpid(p, &status, 0) < 0) {
 		LOG_ERROR("Failed to waitpid(%d): %s\n", (int)p, strerror(errno));
 		return -1;
@@ -134,12 +183,9 @@ int run_external_process(const char *const *exec_args, char buffer[EXTERNAL_BUFF
 		LOG_ERROR("Child process '%s' exited abnormally\n", exec_args[0]);
 	} else if (WEXITSTATUS(status) != 0) {
 		LOG_ERROR("External process failed with exit code %d\n", WEXITSTATUS(status));
-		LOG_ERROR("Output was: %s\n", buffer ? buffer : internal);
+		LOG_ERROR("Output was: %s\n", internal);
 		return -1;
 	}
 
-	if (buffer)
-		memcpy(buffer, internal, EXTERNAL_BUFFER_MAX);
-
 	return 0;
 }

daemon/governors-query.c

@@ -133,6 +133,7 @@ const char *get_gov_state(void)
 				/* Don't handle the mixed case, this shouldn't ever happen
 				 * But it is a clear sign we shouldn't carry on */
 				LOG_ERROR("Governors malformed: got \"%s\", expected \"%s\"", contents, governor);
+				fclose(f);
 				return "malformed";
 			}
 

daemon/gpu-control.c

@@ -50,7 +50,10 @@ enum GPUVendor gamemode_get_gpu_vendor(long device)
 		return Vendor_Invalid;
 	}
 	char buff[64];
-	if (fgets(buff, 64, file) != NULL) {
+	bool got_line = fgets(buff, 64, file) != NULL;
+	fclose(file);
+
+	if (got_line) {
 		vendor = strtol(buff, NULL, 0);
 	} else {
 		LOG_ERROR("Coudn't read contents of file %s, will not apply optimisations!\n", path);

daemon/gpuclockctl.c

@@ -36,6 +36,8 @@ POSSIBILITY OF SUCH DAMAGE.
 #include "external-helper.h"
 #include "gpu-control.h"
 
+#include <limits.h>
+
 /* NV constants */
 #define NV_CORE_OFFSET_ATTRIBUTE "GPUGraphicsClockOffset"
 #define NV_MEM_OFFSET_ATTRIBUTE "GPUMemoryTransferRateOffset"
@@ -44,6 +46,7 @@ POSSIBILITY OF SUCH DAMAGE.
 #define NV_PCIDEVICE_ATTRIBUTE "PCIDevice"
 #define NV_ATTRIBUTE_FORMAT "[gpu:%ld]/%s"
 #define NV_PERF_LEVEL_FORMAT "[%ld]"
+#define NV_ARG_MAX 128
 
 /* AMD constants */
 #define AMD_DRM_PATH "/sys/class/drm/card%ld/device/%s"
@@ -64,6 +67,30 @@ static void print_usage_and_exit(void)
 	exit(EXIT_FAILURE);
 }
 
+static const char *get_nv_attr(const char *attr)
+{
+	static char out[EXTERNAL_BUFFER_MAX];
+	const char *exec_args[] = { "/usr/bin/nvidia-settings", "-q", attr, "-t", NULL };
+	if (run_external_process(exec_args, out, -1) != 0) {
+		LOG_ERROR("Failed to get %s!\n", attr);
+		out[0] = 0;
+		return NULL;
+	}
+
+	return &out[0];
+}
+
+static int set_nv_attr(const char *attr)
+{
+	const char *exec_args_core[] = { "/usr/bin/nvidia-settings", "-a", attr, NULL };
+	if (run_external_process(exec_args_core, NULL, -1) != 0) {
+		LOG_ERROR("Failed to set %s!\n", attr);
+		return -1;
+	}
+
+	return 0;
+}
+
 /* Get the nvidia driver index for the current GPU */
 static long get_gpu_index_id_nv(struct GameModeGPUInfo *info)
 {
@@ -108,23 +135,21 @@ static long get_max_perf_level_nv(struct GameModeGPUInfo *info)
 	if (!getenv("DISPLAY"))
 		LOG_ERROR("Getting Nvidia parameters requires DISPLAY to be set - will likely fail!\n");
 
-	char arg[128] = { 0 };
-	char buf[EXTERNAL_BUFFER_MAX] = { 0 };
+	char arg[NV_ARG_MAX] = { 0 };
+	const char *attr;
 
-	snprintf(arg, 128, NV_ATTRIBUTE_FORMAT, info->device, NV_PERFMODES_ATTRIBUTE);
-	const char *exec_args[] = { "/usr/bin/nvidia-settings", "-q", arg, "-t", NULL };
-	if (run_external_process(exec_args, buf, -1) != 0) {
-		LOG_ERROR("Failed to get %s!\n", arg);
+	snprintf(arg, NV_ARG_MAX, NV_ATTRIBUTE_FORMAT, info->device, NV_PERFMODES_ATTRIBUTE);
+	if ((attr = get_nv_attr(arg)) == NULL) {
 		return -1;
 	}
 
-	char *ptr = strrchr(buf, ';');
+	char *ptr = strrchr(attr, ';');
 	long level = -1;
 	if (!ptr || sscanf(ptr, "; perf=%ld", &level) != 1) {
 		LOG_ERROR(
 		    "Output didn't match expected format, couldn't discern highest perf level from "
 		    "nvidia-settings!\n");
-		LOG_ERROR("Output:%s\n", buf);
+		LOG_ERROR("Output:%s\n", attr);
 		return -1;
 	}
 
@@ -142,59 +167,53 @@ static int get_gpu_state_nv(struct GameModeGPUInfo *info)
 
 	long perf_level = get_max_perf_level_nv(info);
 
-	char arg[128] = { 0 };
-	char buf[EXTERNAL_BUFFER_MAX] = { 0 };
+	char arg[NV_ARG_MAX] = { 0 };
+	const char *attr;
 	char *end;
 
 	/* Get the GPUGraphicsClockOffset parameter */
 	snprintf(arg,
-	         128,
+	         NV_ARG_MAX,
 	         NV_ATTRIBUTE_FORMAT NV_PERF_LEVEL_FORMAT,
 	         info->device,
 	         NV_CORE_OFFSET_ATTRIBUTE,
 	         perf_level);
-	const char *exec_args_core[] = { "/usr/bin/nvidia-settings", "-q", arg, "-t", NULL };
-	if (run_external_process(exec_args_core, buf, -1) != 0) {
-		LOG_ERROR("Failed to get %s!\n", arg);
+	if ((attr = get_nv_attr(arg)) == NULL) {
 		return -1;
 	}
 
-	info->nv_core = strtol(buf, &end, 10);
-	if (end == buf) {
-		LOG_ERROR("Failed to parse output for \"%s\" output was \"%s\"!\n", arg, buf);
+	info->nv_core = strtol(attr, &end, 10);
+	if (end == attr) {
+		LOG_ERROR("Failed to parse output for \"%s\" output was \"%s\"!\n", arg, attr);
 		return -1;
 	}
 
 	/* Get the GPUMemoryTransferRateOffset parameter */
 	snprintf(arg,
-	         128,
+	         NV_ARG_MAX,
 	         NV_ATTRIBUTE_FORMAT NV_PERF_LEVEL_FORMAT,
 	         info->device,
 	         NV_MEM_OFFSET_ATTRIBUTE,
 	         perf_level);
-	const char *exec_args_mem[] = { "/usr/bin/nvidia-settings", "-q", arg, "-t", NULL };
-	if (run_external_process(exec_args_mem, buf, -1) != 0) {
-		LOG_ERROR("Failed to get %s!\n", arg);
+	if ((attr = get_nv_attr(arg)) == NULL) {
 		return -1;
 	}
 
-	info->nv_mem = strtol(buf, &end, 10);
-	if (end == buf) {
-		LOG_ERROR("Failed to parse output for \"%s\" output was \"%s\"!\n", arg, buf);
+	info->nv_mem = strtol(attr, &end, 10);
+	if (end == attr) {
+		LOG_ERROR("Failed to parse output for \"%s\" output was \"%s\"!\n", arg, attr);
 		return -1;
 	}
 
 	/* Get the GPUPowerMizerMode parameter */
-	snprintf(arg, 128, NV_ATTRIBUTE_FORMAT, info->device, NV_POWERMIZER_MODE_ATTRIBUTE);
-	const char *exec_args_pm[] = { "/usr/bin/nvidia-settings", "-q", arg, "-t", NULL };
-	if (run_external_process(exec_args_pm, buf, -1) != 0) {
-		LOG_ERROR("Failed to get %s!\n", arg);
+	snprintf(arg, NV_ARG_MAX, NV_ATTRIBUTE_FORMAT, info->device, NV_POWERMIZER_MODE_ATTRIBUTE);
+	if ((attr = get_nv_attr(arg)) == NULL) {
 		return -1;
 	}
 
-	info->nv_powermizer_mode = strtol(buf, &end, 10);
-	if (end == buf) {
-		LOG_ERROR("Failed to parse output for \"%s\" output was \"%s\"!\n", arg, buf);
+	info->nv_powermizer_mode = strtol(attr, &end, 10);
+	if (end == attr) {
+		LOG_ERROR("Failed to parse output for \"%s\" output was \"%s\"!\n", arg, attr);
 		return -1;
 	}
 
@@ -218,20 +237,18 @@ static int set_gpu_state_nv(struct GameModeGPUInfo *info)
 
 	long perf_level = get_max_perf_level_nv(info);
 
-	char arg[128] = { 0 };
+	char arg[NV_ARG_MAX] = { 0 };
 
 	/* Set the GPUGraphicsClockOffset parameter */
 	if (info->nv_core != -1) {
 		snprintf(arg,
-		         128,
+		         NV_ARG_MAX,
 		         NV_ATTRIBUTE_FORMAT NV_PERF_LEVEL_FORMAT "=%ld",
 		         info->device,
 		         NV_CORE_OFFSET_ATTRIBUTE,
 		         perf_level,
 		         info->nv_core);
-		const char *exec_args_core[] = { "/usr/bin/nvidia-settings", "-a", arg, NULL };
-		if (run_external_process(exec_args_core, NULL, -1) != 0) {
-			LOG_ERROR("Failed to set %s!\n", arg);
+		if (set_nv_attr(arg) != 0) {
 			status = -1;
 		}
 	}
@@ -239,15 +256,13 @@ static int set_gpu_state_nv(struct GameModeGPUInfo *info)
 	/* Set the GPUMemoryTransferRateOffset parameter */
 	if (info->nv_mem != -1) {
 		snprintf(arg,
-		         128,
+		         NV_ARG_MAX,
 		         NV_ATTRIBUTE_FORMAT NV_PERF_LEVEL_FORMAT "=%ld",
 		         info->device,
 		         NV_MEM_OFFSET_ATTRIBUTE,
 		         perf_level,
 		         info->nv_mem);
-		const char *exec_args_mem[] = { "/usr/bin/nvidia-settings", "-a", arg, NULL };
-		if (run_external_process(exec_args_mem, NULL, -1) != 0) {
-			LOG_ERROR("Failed to set %s!\n", arg);
+		if (set_nv_attr(arg) != 0) {
 			status = -1;
 		}
 	}
@@ -255,14 +270,12 @@ static int set_gpu_state_nv(struct GameModeGPUInfo *info)
 	/* Set the GPUPowerMizerMode parameter if requested */
 	if (info->nv_powermizer_mode != -1) {
 		snprintf(arg,
-		         128,
+		         NV_ARG_MAX,
 		         NV_ATTRIBUTE_FORMAT "=%ld",
 		         info->device,
 		         NV_POWERMIZER_MODE_ATTRIBUTE,
 		         info->nv_powermizer_mode);
-		const char *exec_args_pm[] = { "/usr/bin/nvidia-settings", "-a", arg, NULL };
-		if (run_external_process(exec_args_pm, NULL, -1) != 0) {
-			LOG_ERROR("Failed to set %s!\n", arg);
+		if (set_nv_attr(arg) != 0) {
 			status = -1;
 		}
 	}
@@ -280,8 +293,8 @@ static int get_gpu_state_amd(struct GameModeGPUInfo *info)
 		return -1;
 
 	/* Get the contents of power_dpm_force_performance_level */
-	char path[64];
-	snprintf(path, 64, AMD_DRM_PATH, info->device, "power_dpm_force_performance_level");
+	char path[PATH_MAX];
+	snprintf(path, PATH_MAX, AMD_DRM_PATH, info->device, "power_dpm_force_performance_level");
 
 	FILE *file = fopen(path, "r");
 	if (!file) {
@@ -289,21 +302,26 @@ static int get_gpu_state_amd(struct GameModeGPUInfo *info)
 		return -1;
 	}
 
+	int ret = 0;
+
 	char buff[CONFIG_VALUE_MAX] = { 0 };
 	if (!fgets(buff, CONFIG_VALUE_MAX, file)) {
 		LOG_ERROR("Could not read file %s (%s)!\n", path, strerror(errno));
-		return -1;
+		ret = -1;
 	}
 
 	if (fclose(file) != 0) {
 		LOG_ERROR("Could not close %s after reading (%s)!\n", path, strerror(errno));
-		return -1;
+		ret = -1;
 	}
 
-	/* Copy in the value from the file */
-	strncpy(info->amd_performance_level, buff, CONFIG_VALUE_MAX - 1);
-	info->amd_performance_level[CONFIG_VALUE_MAX - 1] = '\0';
-	return info == NULL;
+	if (ret == 0) {
+		/* Copy in the value from the file */
+		strncpy(info->amd_performance_level, buff, CONFIG_VALUE_MAX - 1);
+		info->amd_performance_level[CONFIG_VALUE_MAX - 1] = '\0';
+	}
+
+	return ret;
 }
 
 /*
@@ -311,8 +329,8 @@ static int get_gpu_state_amd(struct GameModeGPUInfo *info)
  */
 static int set_gpu_state_amd_file(const char *filename, long device, const char *value)
 {
-	char path[64];
-	snprintf(path, 64, AMD_DRM_PATH, device, filename);
+	char path[PATH_MAX];
+	snprintf(path, PATH_MAX, AMD_DRM_PATH, device, filename);
 
 	FILE *file = fopen(path, "w");
 	if (!file) {
@@ -320,17 +338,19 @@ static int set_gpu_state_amd_file(const char *filename, long device, const char
 		return -1;
 	}
 
+	int ret = 0;
+
 	if (fprintf(file, "%s", value) < 0) {
 		LOG_ERROR("Could not write to %s (%s)!\n", path, strerror(errno));
-		return -1;
+		ret = -1;
 	}
 
 	if (fclose(file) != 0) {
 		LOG_ERROR("Could not close %s after writing (%s)!\n", path, strerror(errno));
-		return -1;
+		ret = -1;
 	}
 
-	return 0;
+	return ret;
 }
 
 /**
@@ -390,10 +410,11 @@ static long get_generic_value(const char *val)
  */
 int main(int argc, char *argv[])
 {
+	struct GameModeGPUInfo info;
+	memset(&info, 0, sizeof(info));
+
 	if (argc == 3 && strncmp(argv[2], "get", 3) == 0) {
 		/* Get and verify the vendor and device */
-		struct GameModeGPUInfo info;
-		memset(&info, 0, sizeof(info));
 		info.device = get_device(argv[1]);
 		info.vendor = gamemode_get_gpu_vendor(info.device);
 
@@ -420,8 +441,6 @@ int main(int argc, char *argv[])
 
 	} else if (argc >= 4 && argc <= 7 && strncmp(argv[2], "set", 3) == 0) {
 		/* Get and verify the vendor and device */
-		struct GameModeGPUInfo info;
-		memset(&info, 0, sizeof(info));
 		info.device = get_device(argv[1]);
 		info.vendor = gamemode_get_gpu_vendor(info.device);