Hello Domenico Cerasuolo,
The patch 7c967f267b1d: "selftests: cgroup: add test_zswap with no kmem bypass test" from Jun 21, 2023 (linux-next), leads to the following Smatch static checker warning:
./tools/testing/selftests/cgroup/test_zswap.c:228 test_no_kmem_bypass() error: uninitialized symbol 'test_group'.
./tools/testing/selftests/cgroup/test_zswap.c 148 static int test_no_kmem_bypass(const char *root) 149 { 150 size_t min_free_kb_high, min_free_kb_low, min_free_kb_original; 151 struct no_kmem_bypass_child_args *values; 152 size_t trigger_allocation_size; 153 int wait_child_iteration = 0; 154 long stored_pages_threshold; 155 struct sysinfo sys_info; 156 int ret = KSFT_FAIL; 157 int child_status; 158 char *test_group; 159 pid_t child_pid; 160 161 /* Read sys info and compute test values accordingly */ 162 if (sysinfo(&sys_info) != 0) 163 return KSFT_FAIL; 164 if (sys_info.totalram > 5000000000) 165 return KSFT_SKIP; 166 values = mmap(0, sizeof(struct no_kmem_bypass_child_args), PROT_READ | 167 PROT_WRITE, MAP_SHARED | MAP_ANONYMOUS, -1, 0); 168 if (values == MAP_FAILED) 169 return KSFT_FAIL; 170 if (read_min_free_kb(&min_free_kb_original)) 171 return KSFT_FAIL; 172 min_free_kb_high = sys_info.totalram / 2000; 173 min_free_kb_low = sys_info.totalram / 500000; 174 values->target_alloc_bytes = (sys_info.totalram - min_free_kb_high * 1000) + 175 sys_info.totalram * 5 / 100; 176 stored_pages_threshold = sys_info.totalram / 5 / 4096; 177 trigger_allocation_size = sys_info.totalram / 20; 178 179 /* Set up test memcg */ 180 if (cg_write(root, "cgroup.subtree_control", "+memory")) 181 goto out;
test_group is not initialized here. Undoing stuff which hasn't been done is the canonical bug with "goto out;" labels.
182 test_group = cg_name(root, "kmem_bypass_test"); 183 if (!test_group) 184 goto out; 185 186 /* Spawn memcg child and wait for it to allocate */ 187 set_min_free_kb(min_free_kb_low); 188 if (cg_create(test_group))
test_group is allocated but not created. Is it okay to call destroy on this path?
189 goto out; 190 values->child_allocated = false; 191 child_pid = cg_run_nowait(test_group, no_kmem_bypass_child, values); 192 if (child_pid < 0) 193 goto out; 194 while (!values->child_allocated && wait_child_iteration++ < 10000) 195 usleep(1000); 196 197 /* Try to wakeup kswapd and let it push child memory to zswap */ 198 set_min_free_kb(min_free_kb_high); 199 for (int i = 0; i < 20; i++) { 200 size_t stored_pages; 201 char *trigger_allocation = malloc(trigger_allocation_size); 202 203 if (!trigger_allocation) 204 break; 205 for (int i = 0; i < trigger_allocation_size; i += 4095) 206 trigger_allocation[i] = 'b'; 207 usleep(100000); 208 free(trigger_allocation); 209 if (get_zswap_stored_pages(&stored_pages)) 210 break; 211 if (stored_pages < 0) 212 break; 213 /* If memory was pushed to zswap, verify it belongs to memcg */ 214 if (stored_pages > stored_pages_threshold) { 215 int zswapped = cg_read_key_long(test_group, "memory.stat", "zswapped "); 216 int delta = stored_pages * 4096 - zswapped; 217 int result_ok = delta < stored_pages * 4096 / 4; 218 219 ret = result_ok ? KSFT_PASS : KSFT_FAIL; 220 break; 221 } 222 } 223 224 kill(child_pid, SIGTERM); 225 waitpid(child_pid, &child_status, 0); 226 out: 227 set_min_free_kb(min_free_kb_original); --> 228 cg_destroy(test_group); 229 free(test_group); 230 return ret; 231 }
regards, dan carpenter
On Mon, Jul 31, 2023 at 4:18 PM Dan Carpenter dan.carpenter@linaro.org wrote:
Hello Domenico Cerasuolo,
The patch 7c967f267b1d: "selftests: cgroup: add test_zswap with no kmem bypass test" from Jun 21, 2023 (linux-next), leads to the following Smatch static checker warning:
./tools/testing/selftests/cgroup/test_zswap.c:228 test_no_kmem_bypass() error: uninitialized symbol 'test_group'.
./tools/testing/selftests/cgroup/test_zswap.c 148 static int test_no_kmem_bypass(const char *root) 149 { 150 size_t min_free_kb_high, min_free_kb_low, min_free_kb_original; 151 struct no_kmem_bypass_child_args *values; 152 size_t trigger_allocation_size; 153 int wait_child_iteration = 0; 154 long stored_pages_threshold; 155 struct sysinfo sys_info; 156 int ret = KSFT_FAIL; 157 int child_status; 158 char *test_group; 159 pid_t child_pid; 160 161 /* Read sys info and compute test values accordingly */ 162 if (sysinfo(&sys_info) != 0) 163 return KSFT_FAIL; 164 if (sys_info.totalram > 5000000000) 165 return KSFT_SKIP; 166 values = mmap(0, sizeof(struct no_kmem_bypass_child_args), PROT_READ | 167 PROT_WRITE, MAP_SHARED | MAP_ANONYMOUS, -1, 0); 168 if (values == MAP_FAILED) 169 return KSFT_FAIL; 170 if (read_min_free_kb(&min_free_kb_original)) 171 return KSFT_FAIL; 172 min_free_kb_high = sys_info.totalram / 2000; 173 min_free_kb_low = sys_info.totalram / 500000; 174 values->target_alloc_bytes = (sys_info.totalram - min_free_kb_high * 1000) + 175 sys_info.totalram * 5 / 100; 176 stored_pages_threshold = sys_info.totalram / 5 / 4096; 177 trigger_allocation_size = sys_info.totalram / 20; 178 179 /* Set up test memcg */ 180 if (cg_write(root, "cgroup.subtree_control", "+memory")) 181 goto out;
test_group is not initialized here. Undoing stuff which hasn't been done is the canonical bug with "goto out;" labels.
True, this goto out and next one should just be return.
182 test_group = cg_name(root, "kmem_bypass_test"); 183 if (!test_group) 184 goto out; 185 186 /* Spawn memcg child and wait for it to allocate */ 187 set_min_free_kb(min_free_kb_low); 188 if (cg_create(test_group))
test_group is allocated but not created. Is it okay to call destroy on this path?
It is, cg_destroy() just returns 0 if the path doesn't exist.
189 goto out; 190 values->child_allocated = false; 191 child_pid = cg_run_nowait(test_group, no_kmem_bypass_child, values); 192 if (child_pid < 0) 193 goto out; 194 while (!values->child_allocated && wait_child_iteration++ < 10000) 195 usleep(1000); 196 197 /* Try to wakeup kswapd and let it push child memory to zswap */ 198 set_min_free_kb(min_free_kb_high); 199 for (int i = 0; i < 20; i++) { 200 size_t stored_pages; 201 char *trigger_allocation = malloc(trigger_allocation_size); 202 203 if (!trigger_allocation) 204 break; 205 for (int i = 0; i < trigger_allocation_size; i += 4095) 206 trigger_allocation[i] = 'b'; 207 usleep(100000); 208 free(trigger_allocation); 209 if (get_zswap_stored_pages(&stored_pages)) 210 break; 211 if (stored_pages < 0) 212 break; 213 /* If memory was pushed to zswap, verify it belongs to memcg */ 214 if (stored_pages > stored_pages_threshold) { 215 int zswapped = cg_read_key_long(test_group, "memory.stat", "zswapped "); 216 int delta = stored_pages * 4096 - zswapped; 217 int result_ok = delta < stored_pages * 4096 / 4; 218 219 ret = result_ok ? KSFT_PASS : KSFT_FAIL; 220 break; 221 } 222 } 223 224 kill(child_pid, SIGTERM); 225 waitpid(child_pid, &child_status, 0); 226 out: 227 set_min_free_kb(min_free_kb_original);
--> 228 cg_destroy(test_group); 229 free(test_group); 230 return ret; 231 }
regards, dan carpenter
Thanks again, will fix this too!
Domenico
linux-kselftest-mirror@lists.linaro.org