On Mon, Jul 31, 2023 at 4:18 PM Dan Carpenter <dan.carpenter@xxxxxxxxxx> 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
