From e59df3bc0a0df1dd117f28f498a771a6f251bf75 Mon Sep 17 00:00:00 2001 From: Arne Redlich Date: Wed, 21 Sep 2022 10:15:25 +0200 Subject: [PATCH 1/9] Makefile: allow overriding the compiler Signed-off-by: Arne Redlich --- Makefile | 3 +-- 1 file changed, 1 insertion(+), 2 deletions(-) diff --git a/Makefile b/Makefile index ac70441..5d9f95e 100644 --- a/Makefile +++ b/Makefile @@ -1,4 +1,4 @@ -CC=gcc +CC?=gcc CFLAGS=-I. -Wall -g LDFLAGS= OBJS=predict.o adaptivemmd.o @@ -18,4 +18,3 @@ adaptivemmd: $(OBJS) clean: rm -f $(OBJS) adaptivemmd cscope.* - From ea9105139a1cdf4f44445520abb73f6c31db6677 Mon Sep 17 00:00:00 2001 From: Arne Redlich Date: Wed, 21 Sep 2022 09:18:18 +0200 Subject: [PATCH 2/9] Zero-initialize buffer in check_permissions Spotted by valgrind: ==1728072== 1 errors in context 1 of 2: ==1728072== Syscall param write(buf) points to uninitialised byte(s) ==1728072== at 0x4F4E104: write (write.c:27) ==1728072== by 0x10BE8F: check_permissions (adaptivemmd.c:813) ==1728072== by 0x10CFC4: main (adaptivemmd.c:1361) ==1728072== Address 0x1ffefffdb3 is on thread 1's stack ==1728072== in frame #1, created by check_permissions (adaptivemmd.c:790) ==1728072== Uninitialised value was created by a stack allocation ==1728072== at 0x10BD49: check_permissions (adaptivemmd.c:790) ==1728072== ==1728072== ==1728072== 5 errors in context 2 of 2: ==1728072== Conditional jump or move depends on uninitialised value(s) ==1728072== at 0x4C34D08: strlen (vg_replace_strmem.c:458) ==1728072== by 0x10BE75: check_permissions (adaptivemmd.c:813) ==1728072== by 0x10CFC4: main (adaptivemmd.c:1361) ==1728072== Uninitialised value was created by a stack allocation ==1728072== at 0x10BD49: check_permissions (adaptivemmd.c:790) ==1728072== ==1728072== ERROR SUMMARY: 6 errors from 2 contexts (suppressed: 0 from 0) Signed-off-by: Arne Redlich --- adaptivemmd.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/adaptivemmd.c b/adaptivemmd.c index 6139c2a..0016ca9 100644 --- a/adaptivemmd.c +++ b/adaptivemmd.c @@ -789,7 +789,7 @@ static int check_permissions(void) { int fd; - char tmpstr[40]; + char tmpstr[40] = {0}; /* * Make sure running kernel supports watermark_scale_factor file From 0088a3058f60faa8cfb3932251b58bc22ba771ec Mon Sep 17 00:00:00 2001 From: Arne Redlich Date: Wed, 21 Sep 2022 09:27:53 +0200 Subject: [PATCH 3/9] check_permissions: close fd in error paths Signed-off-by: Arne Redlich --- adaptivemmd.c | 2 ++ 1 file changed, 2 insertions(+) diff --git a/adaptivemmd.c b/adaptivemmd.c index 0016ca9..0634152 100644 --- a/adaptivemmd.c +++ b/adaptivemmd.c @@ -802,6 +802,7 @@ check_permissions(void) /* Can we write to this file */ if (read(fd, tmpstr, sizeof(tmpstr)) < 0) { log_err("Can not read "RESCALE_WMARK" (%s)", strerror(errno)); + close(fd); return 0; } close(fd); @@ -812,6 +813,7 @@ check_permissions(void) if (write(fd, tmpstr, strlen(tmpstr)) < 0) { log_err("Can not write to "RESCALE_WMARK" (%s)", strerror(errno)); + close(fd); return 0; } close(fd); From b9b06a9ed2c2545079a8649bb4d22749449fe199 Mon Sep 17 00:00:00 2001 From: Arne Redlich Date: Thu, 22 Sep 2022 23:27:30 +0200 Subject: [PATCH 4/9] Fix potential memleak in no_pages_reclaimed Pointed out by clang analyzer. Signed-off-by: Arne Redlich --- adaptivemmd.c | 10 ++++++---- 1 file changed, 6 insertions(+), 4 deletions(-) diff --git a/adaptivemmd.c b/adaptivemmd.c index 0634152..aedf0cf 100644 --- a/adaptivemmd.c +++ b/adaptivemmd.c @@ -511,14 +511,15 @@ no_pages_reclaimed() FILE *fp = NULL; size_t len = 100; char *line = malloc(len); - unsigned long val, reclaimed; + unsigned long val = 0; + unsigned long reclaimed = 0; char desc[100]; fp = fopen(VMSTAT, "r"); if (!fp) - return 0; + goto out; - total_cache_pages = reclaimed = 0; + total_cache_pages = 0; while ((fgets(line, len, fp) != NULL)) { sscanf(line, "%s %lu\n", desc, &val ); if (strcmp(desc, "pgsteal_kswapd") == 0) @@ -533,8 +534,9 @@ no_pages_reclaimed() total_cache_pages += val; } - free(line); fclose(fp); +out: + free(line); return reclaimed; } From 188169069afa45ef54c2733b3e418a7e3f158683 Mon Sep 17 00:00:00 2001 From: Arne Redlich Date: Mon, 26 Sep 2022 11:13:12 +0200 Subject: [PATCH 5/9] Fix compiler warnings from clang $ CC=clang make clang -c -o predict.o predict.c -I. -Wall -g predict.c:223:146: warning: absolute value function 'abs' given an argument of type 'long long' but has parameter of type 'int' which may cause truncation of value [-Wabsolute-value] log_info(2, "Consumption rate on node %d=%ld pages/msec, reclaim rate is %ld pages/msec, Free pages=%ld, low wmark=%ld, high wmark=%ld", nid, abs(m[0]), reclaim_rate, frag_vec[0].free_pages, low_wmark, high_wmark); ^ predict.c:223:146: note: use function 'llabs' instead log_info(2, "Consumption rate on node %d=%ld pages/msec, reclaim rate is %ld pages/msec, Free pages=%ld, low wmark=%ld, high wmark=%ld", nid, abs(m[0]), reclaim_rate, frag_vec[0].free_pages, low_wmark, high_wmark); ^~~ llabs ./predict.h:79:25: note: expanded from macro 'log_info' log_msg(LOG_INFO, __VA_ARGS__) ^ predict.c:227:8: warning: absolute value function 'abs' given an argument of type 'long long' but has parameter of type 'int' which may cause truncation of value [-Wabsolute-value] / abs(m[0]); ^ predict.c:227:8: note: use function 'llabs' instead / abs(m[0]); ^~~ llabs predict.c:247:147: warning: absolute value function 'abs' given an argument of type 'long long' but has parameter of type 'int' which may cause truncation of value [-Wabsolute-value] log_info(3, "Consumption rate on node %d=%ld pages/msec, reclaim rate is %ld pages/msec, Free pages=%ld, low wmark=%ld, high wmark=%ld", nid, abs(m[0]), reclaim_rate, frag_vec[0].free_pages, low_wmark, high_wmark); ^ predict.c:247:147: note: use function 'llabs' instead log_info(3, "Consumption rate on node %d=%ld pages/msec, reclaim rate is %ld pages/msec, Free pages=%ld, low wmark=%ld, high wmark=%ld", nid, abs(m[0]), reclaim_rate, frag_vec[0].free_pages, low_wmark, high_wmark); ^~~ llabs ./predict.h:79:25: note: expanded from macro 'log_info' log_msg(LOG_INFO, __VA_ARGS__) ^ 3 warnings generated. Signed-off-by: Arne Redlich --- adaptivemmd.c | 8 ++++---- predict.c | 6 +++--- 2 files changed, 7 insertions(+), 7 deletions(-) diff --git a/adaptivemmd.c b/adaptivemmd.c index aedf0cf..4b060dc 100644 --- a/adaptivemmd.c +++ b/adaptivemmd.c @@ -76,8 +76,8 @@ unsigned long min_wmark[MAX_NUMANODES], low_wmark[MAX_NUMANODES]; unsigned long high_wmark[MAX_NUMANODES], managed_pages[MAX_NUMANODES]; -unsigned long total_free_pages, total_cache_pages, total_hugepages, base_psize; -long compaction_rate, reclaim_rate; +unsigned long total_free_pages, total_cache_pages, base_psize; +long compaction_rate, reclaim_rate, total_hugepages; struct lsq_struct page_lsq[MAX_NUMANODES][MAX_ORDER]; int dry_run; int debug_mode, verbose, del_lock = 0; @@ -315,7 +315,7 @@ update_hugepages() { DIR *dp; struct dirent *ep; - unsigned long newhpages = 0; + long newhpages = 0; int rc = -1; dp = opendir(HUGEPAGESINFO); @@ -343,7 +343,7 @@ update_hugepages() if (newhpages) { unsigned long tmp; - tmp = abs(newhpages - total_hugepages); + tmp = llabs(newhpages - total_hugepages); /* * If number of hugepages changes from 0 to a * positive number, percentage calculation will diff --git a/predict.c b/predict.c index b26b99e..ee5d96e 100644 --- a/predict.c +++ b/predict.c @@ -220,11 +220,11 @@ predict(struct frag_info *frag_vec, struct lsq_struct *lsq, if (frag_vec[0].free_pages <= high_wmark) { retval |= MEMPREDICT_RECLAIM; log_info(2, "Reclamation recommended due to free pages being below high watermark"); - log_info(2, "Consumption rate on node %d=%ld pages/msec, reclaim rate is %ld pages/msec, Free pages=%ld, low wmark=%ld, high wmark=%ld", nid, abs(m[0]), reclaim_rate, frag_vec[0].free_pages, low_wmark, high_wmark); + log_info(2, "Consumption rate on node %d=%ld pages/msec, reclaim rate is %ld pages/msec, Free pages=%ld, low wmark=%ld, high wmark=%ld", nid, llabs(m[0]), reclaim_rate, frag_vec[0].free_pages, low_wmark, high_wmark); } else { time_taken = (frag_vec[0].free_pages - high_wmark) - / abs(m[0]); + / llabs(m[0]); /* * Time to reclaim frag_vec[0].free_pages - high_wmark @@ -244,7 +244,7 @@ predict(struct frag_info *frag_vec, struct lsq_struct *lsq, */ if (time_taken <= (3*time_to_catchup)) { log_info(3, "Reclamation recommended due to high memory consumption rate"); - log_info(3, "Consumption rate on node %d=%ld pages/msec, reclaim rate is %ld pages/msec, Free pages=%ld, low wmark=%ld, high wmark=%ld", nid, abs(m[0]), reclaim_rate, frag_vec[0].free_pages, low_wmark, high_wmark); + log_info(3, "Consumption rate on node %d=%ld pages/msec, reclaim rate is %ld pages/msec, Free pages=%ld, low wmark=%ld, high wmark=%ld", nid, llabs(m[0]), reclaim_rate, frag_vec[0].free_pages, low_wmark, high_wmark); log_info(3, "Time to below high watermark= %ld msec, time to catch up=%ld msec", time_taken, time_to_catchup); retval |= MEMPREDICT_RECLAIM; } From 7463cecea3c8a4e88b74b1f812a240028dc7ffda Mon Sep 17 00:00:00 2001 From: Arne Redlich Date: Mon, 26 Sep 2022 13:33:43 +0200 Subject: [PATCH 6/9] rescale_watermarks: fix double close when dry_run == true Signed-off-by: Arne Redlich --- adaptivemmd.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/adaptivemmd.c b/adaptivemmd.c index 4b060dc..26014eb 100644 --- a/adaptivemmd.c +++ b/adaptivemmd.c @@ -758,7 +758,7 @@ rescale_watermarks(int scale_up) log_info(1, "Adjusting watermarks. Current watermark scale factor = %s", scaled_wmark); if (dry_run) - goto out; + return; log_info(1, "New watermark scale factor = %ld", scaled_watermark); sprintf(scaled_wmark, "%ld\n", scaled_watermark); From b25031ac81b41fdac22e4328d3a4560502e94169 Mon Sep 17 00:00:00 2001 From: Arne Redlich Date: Mon, 26 Sep 2022 14:02:02 +0200 Subject: [PATCH 7/9] Format string fixes Annotate log_msg with a format attribute and fix the resulting compiler warnings. Signed-off-by: Arne Redlich --- adaptivemmd.c | 4 ++-- predict.c | 8 ++++---- predict.h | 2 +- 3 files changed, 7 insertions(+), 7 deletions(-) diff --git a/adaptivemmd.c b/adaptivemmd.c index 26014eb..ec28171 100644 --- a/adaptivemmd.c +++ b/adaptivemmd.c @@ -1237,7 +1237,7 @@ parse_config() * instead to doing it through adaptivemmd */ if (val > MAX_NEGDENTRY) - log_err("Bad value for negative dentry cap = %d (>%d). Proceeding with default of %d", val, MAX_NEGDENTRY, neg_dentry_pct); + log_err("Bad value for negative dentry cap = %ld (>%d). Proceeding with default of %d", val, MAX_NEGDENTRY, neg_dentry_pct); else if (val < 1) neg_dentry_pct = 1; else @@ -1419,7 +1419,7 @@ main(int argc, char **argv) */ base_psize = getpagesize()/1024; - pr_info("adaptivemmd "VERSION" started (verbose=%d, aggressiveness=%d, maxgap=%d)", verbose, aggressiveness, maxgap); + pr_info("adaptivemmd "VERSION" started (verbose=%d, aggressiveness=%d, maxgap=%lu)", verbose, aggressiveness, maxgap); one_time_initializations(); diff --git a/predict.c b/predict.c index ee5d96e..e888014 100644 --- a/predict.c +++ b/predict.c @@ -220,7 +220,7 @@ predict(struct frag_info *frag_vec, struct lsq_struct *lsq, if (frag_vec[0].free_pages <= high_wmark) { retval |= MEMPREDICT_RECLAIM; log_info(2, "Reclamation recommended due to free pages being below high watermark"); - log_info(2, "Consumption rate on node %d=%ld pages/msec, reclaim rate is %ld pages/msec, Free pages=%ld, low wmark=%ld, high wmark=%ld", nid, llabs(m[0]), reclaim_rate, frag_vec[0].free_pages, low_wmark, high_wmark); + log_info(2, "Consumption rate on node %d=%lld pages/msec, reclaim rate is %ld pages/msec, Free pages=%lld, low wmark=%lu, high wmark=%lu", nid, llabs(m[0]), reclaim_rate, frag_vec[0].free_pages, low_wmark, high_wmark); } else { time_taken = (frag_vec[0].free_pages - high_wmark) @@ -244,7 +244,7 @@ predict(struct frag_info *frag_vec, struct lsq_struct *lsq, */ if (time_taken <= (3*time_to_catchup)) { log_info(3, "Reclamation recommended due to high memory consumption rate"); - log_info(3, "Consumption rate on node %d=%ld pages/msec, reclaim rate is %ld pages/msec, Free pages=%ld, low wmark=%ld, high wmark=%ld", nid, llabs(m[0]), reclaim_rate, frag_vec[0].free_pages, low_wmark, high_wmark); + log_info(3, "Consumption rate on node %d=%lld pages/msec, reclaim rate is %ld pages/msec, Free pages=%lld, low wmark=%lu, high wmark=%lu", nid, llabs(m[0]), reclaim_rate, frag_vec[0].free_pages, low_wmark, high_wmark); log_info(3, "Time to below high watermark= %ld msec, time to catch up=%ld msec", time_taken, time_to_catchup); retval |= MEMPREDICT_RECLAIM; } @@ -320,7 +320,7 @@ predict(struct frag_info *frag_vec, struct lsq_struct *lsq, if (higher_order_pages < (m[order] * x_cross)) { log_info(2, "Compaction recommended on node %d. Running out of order %d pages", nid, order); if (order < (MAX_ORDER -1)) - log_info(3, "No. of free order %d pages = %ld base pages, consumption rate=%ld pages/msec", order, (frag_vec[order+1].free_pages - frag_vec[order].free_pages), m[order]); + log_info(3, "No. of free order %d pages = %lld base pages, consumption rate=%lld pages/msec", order, (frag_vec[order+1].free_pages - frag_vec[order].free_pages), m[order]); log_info(3, "Current compaction rate=%ld pages/msec", compaction_rate); retval |= MEMPREDICT_COMPACT; break; @@ -351,7 +351,7 @@ predict(struct frag_info *frag_vec, struct lsq_struct *lsq, if (time_taken >= time_to_catchup) { log_info(3, "Compaction recommended on node %d. Order %d pages consumption rate is high", nid, order); if (order < (MAX_ORDER -1)) - log_info(3, "No. of free order %d pages = %ld base pages, consumption rate=%ld pages/msec", order, (frag_vec[order+1].free_pages - frag_vec[order].free_pages), m[order]); + log_info(3, "No. of free order %d pages = %lld base pages, consumption rate=%lld pages/msec", order, (frag_vec[order+1].free_pages - frag_vec[order].free_pages), m[order]); log_info(3, "Current compaction rate=%ld pages/msec, Exhaustion in %ld msec", compaction_rate, time_taken); retval |= MEMPREDICT_COMPACT; break; diff --git a/predict.h b/predict.h index 9363e06..ab3f128 100644 --- a/predict.h +++ b/predict.h @@ -81,7 +81,7 @@ unsigned long predict(struct frag_info *, struct lsq_struct *, /* Use pr_info to log info irrespective of verbosity level */ #define pr_info(...) log_msg(LOG_INFO, __VA_ARGS__) -extern void log_msg(int level, char *fmt, ...); +extern void log_msg(int level, char *fmt, ...) __attribute__((format(printf, 2, 3))); #ifdef __cplusplus } From ebb9220eba3bb1207ce301b850dca435cec81dae Mon Sep 17 00:00:00 2001 From: Arne Redlich Date: Thu, 22 Sep 2022 23:20:13 +0200 Subject: [PATCH 8/9] update_zone_watermarks: fix potential memleaks + uninitialized variables Pointed out by clang analyzer. While at it, change the return type to void as it always returns 0 / no caller looks at it. Signed-off-by: Arne Redlich --- adaptivemmd.c | 18 +++++++++--------- 1 file changed, 9 insertions(+), 9 deletions(-) diff --git a/adaptivemmd.c b/adaptivemmd.c index ec28171..7e82ff2 100644 --- a/adaptivemmd.c +++ b/adaptivemmd.c @@ -377,7 +377,8 @@ update_hugepages() #define ZONE_HIGH "high" #define ZONE_MNGD "managed" #define ZONE_PGST "pagesets" -int + +void update_zone_watermarks() { FILE *fp = NULL; @@ -387,13 +388,16 @@ update_zone_watermarks() fp = fopen(ZONEINFO, "r"); if (!fp) - return 0; + goto out_free; while ((fgets(line, len, fp) != NULL)) { if (strncmp(line, "Node", 4) == 0) { char node[FLDLEN], zone[FLDLEN], zone_name[FLDLEN]; int nid; - unsigned long min, low, high, managed; + unsigned long min = 0; + unsigned long low = 0; + unsigned long high = 0; + unsigned long managed = 0; sscanf(line, "%s %d, %s %8s\n", node, &nid, zone, zone_name); if ((current_node == -1) || (current_node != nid)) { @@ -413,10 +417,6 @@ update_zone_watermarks() * Ignore pages in DMA zone for x86 and x86-64. */ if (!skip_dmazone || (strncmp("DMA", zone_name, FLDLEN) != 0)) { - /* - * We found the normal zone. Now look for - * line "pages free" - */ if (fgets(line, len, fp) == NULL) goto out; @@ -449,9 +449,9 @@ update_zone_watermarks() } out: - free(line); fclose(fp); - return 0; +out_free: + free(line); } /* From a8a4e0e386f7d9447f5723e8d67e93f3272ce73b Mon Sep 17 00:00:00 2001 From: Arne Redlich Date: Wed, 28 Sep 2022 15:38:06 +0200 Subject: [PATCH 9/9] update_zone_watermarks: fix zoneinfo parsing The parser used to break out of the zone parsing state when encountering a "pagesets" token, which is however not always present - use "protection:" instead. Signed-off-by: Arne Redlich --- adaptivemmd.c | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/adaptivemmd.c b/adaptivemmd.c index 7e82ff2..edcb4dc 100644 --- a/adaptivemmd.c +++ b/adaptivemmd.c @@ -376,7 +376,7 @@ update_hugepages() #define ZONE_LOW "low" #define ZONE_HIGH "high" #define ZONE_MNGD "managed" -#define ZONE_PGST "pagesets" +#define ZONE_PROT "protection:" void update_zone_watermarks() @@ -436,7 +436,7 @@ update_zone_watermarks() high = val; if (strncmp(name, ZONE_MNGD, sizeof(ZONE_MNGD)) == 0) managed = val; - if (strncmp(name, ZONE_PGST, sizeof(ZONE_PGST)) == 0) + if (strncmp(name, ZONE_PROT, sizeof(ZONE_PROT)) == 0) break; }