Re: [PATCH v3 2/2] cgroup/rstat: Selftests for niced CPU statistics
> My motivation comes from debugging cgroup selftests when strace is quite > useful and your implementation adds the unnecessary fork which makes the > strace (slightly) less readable. This makes sense, thank you for the context. I hadn't considered debugging considerations much, but I can imagine that it becomes harder to read once the code & strace becomes clogged up. > > Do you think that this increase in granularity / accuracy is worth the > > increase in code complexity? I do agree that it would be much easier > > to read if there was no fork. > > I think both changes (no cg_run or cpu_hog_func_param extension) could > be reasonably small changes (existing usages of cpu_hog_func_param > extension would default to zero nice, so the actual change would only be > in hog_cpus_timed()). I think I will stick with the no cg_run option. Initially, I had wanted to use it to maintain the same style with the other selftests in test_cpu.c, but I think it creates more unnecessary unreadability. Thank you again, Joshua
Re: [PATCH v3 2/2] cgroup/rstat: Selftests for niced CPU statistics
On Mon, Sep 30, 2024 at 02:07:22PM GMT, Joshua Hahn wrote: > The reason I used a fork in the testing is so that I could isolate the niced > portion of the test to only the CPU hog. If I were to nice(1) --> cg_hog() > in a single process without forking, this would mean that the cleanup portion > of the test would also be run as a niced process, The cleanup runs in a parent process and nice is called after fork in a child in those considered cases (at least that's what I meant). > contributing to the stat and potentially dirtying the value (which is > tested for accuracy via `values_close`). Yes, a test that randomly fails (false negative) is a nuisance. One fork is needed, the second doesn't divide different priority tasks. > What do you think? My motivation comes from debugging cgroup selftests when strace is quite useful and your implementation adds the unnecessary fork which makes the strace (slightly) less readable. > Do you think that this increase in granularity / accuracy is worth the > increase in code complexity? I do agree that it would be much easier > to read if there was no fork. I think both changes (no cg_run or cpu_hog_func_param extension) could be reasonably small changes (existing usages of cpu_hog_func_param extension would default to zero nice, so the actual change would only be in hog_cpus_timed()). > Alternatively, I can add a new parameter to cpu_hog_func_param that > takes in a nice value. For this however, I am afraid of changing the > function signature of existing utility functions, since it would mean > breaking support for older functions or others currently working on this. The function is internal to the cgroup selftests and others can rebase, so it doesn't have to stick to a particular signature. HTH, Michal signature.asc Description: PGP signature
Re: [PATCH v3 2/2] cgroup/rstat: Selftests for niced CPU statistics
On Thu, Sep 26, 2024 at 2:10 PM Michal Koutný wrote: > > On Mon, Sep 23, 2024 at 07:20:06AM GMT, Joshua Hahn > wrote: > > +/* > > + * Creates a nice process that consumes CPU and checks that the elapsed > > + * usertime in the cgroup is close to the expected time. > > + */ > > + user_usec = cg_read_key_long(cpucg, "cpu.stat", "user_usec"); > > + nice_usec = cg_read_key_long(cpucg, "cpu.stat", "nice_usec"); > > + if (user_usec != 0 || nice_usec != 0) > > + goto cleanup; > > Can you please distinguish a check between non-zero nice_usec and > non-existent nice_usec (KSFT_FAIL vs KSFT_SKIP)? So that the selftest is > usable on older kernels too. Yes, this sounds good to me -- I will include it in a v4, which I am hoping to send out soon. > > + > > + /* > > + * We fork here to create a new process that can be niced without > > + * polluting the nice value of other selftests > > + */ > > + pid = fork(); > > + if (pid < 0) { > > + goto cleanup; > > + } else if (pid == 0) { > > + struct cpu_hog_func_param param = { > > + .nprocs = 1, > > + .ts = { > > + .tv_sec = usage_seconds, > > + .tv_nsec = 0, > > + }, > > + .clock_type = CPU_HOG_CLOCK_PROCESS, > > + }; > > + > > + /* Try to keep niced CPU usage as constrained to hog_cpu as > > possible */ > > + nice(1); > > + cg_run(cpucg, hog_cpus_timed, (void *)¶m); > > Notice that cg_run() does fork itself internally. > So you can call hog_cpus_timed(cpucg, (void *)¶m) directly, no > need for the fork with cg_run(). (Alternatively substitute fork in this > test with the fork in cg_run() but with extension of cpu_hog_func_params > with the nice value.) > > Thanks, > Michal Thank you for your feedback, Michal. The reason I used a fork in the testing is so that I could isolate the niced portion of the test to only the CPU hog. If I were to nice(1) --> cg_hog() in a single process without forking, this would mean that the cleanup portion of the test would also be run as a niced process, contributing to the stat and potentially dirtying the value (which is tested for accuracy via `values_close`). The other thing that I considered when writing this was that while it is possible to make a process nicer, it is impossible to make a process less nice. This would mean that the comparison & cleanup portions would also be run nicely if I do not call fork(). What do you think? Do you think that this increase in granularity / accuracy is worth the increase in code complexity? I do agree that it would be much easier to read if there was no fork. Alternatively, I can add a new parameter to cpu_hog_func_param that takes in a nice value. For this however, I am afraid of changing the function signature of existing utility functions, since it would mean breaking support for older functions or others currently working on this. Thank you for your detailed feedback again -- I will also change up the diffstat and indentation issues you brought up from the first part of the patch. Joshua
Re: [PATCH v3 2/2] cgroup/rstat: Selftests for niced CPU statistics
On Mon, Sep 23, 2024 at 07:20:06AM GMT, Joshua Hahn wrote: > +/* > + * Creates a nice process that consumes CPU and checks that the elapsed > + * usertime in the cgroup is close to the expected time. > + */ > +static int test_cpucg_nice(const char *root) > +{ > + int ret = KSFT_FAIL; > + int status; > + long user_usec, nice_usec; > + long usage_seconds = 2; > + long expected_nice_usec = usage_seconds * USEC_PER_SEC; > + char *cpucg; > + pid_t pid; > + > + cpucg = cg_name(root, "cpucg_test"); > + if (!cpucg) > + goto cleanup; > + > + if (cg_create(cpucg)) > + goto cleanup; > + > + user_usec = cg_read_key_long(cpucg, "cpu.stat", "user_usec"); > + nice_usec = cg_read_key_long(cpucg, "cpu.stat", "nice_usec"); > + if (user_usec != 0 || nice_usec != 0) > + goto cleanup; Can you please distinguish a check between non-zero nice_usec and non-existent nice_usec (KSFT_FAIL vs KSFT_SKIP)? So that the selftest is usable on older kernels too. > + > + /* > + * We fork here to create a new process that can be niced without > + * polluting the nice value of other selftests > + */ > + pid = fork(); > + if (pid < 0) { > + goto cleanup; > + } else if (pid == 0) { > + struct cpu_hog_func_param param = { > + .nprocs = 1, > + .ts = { > + .tv_sec = usage_seconds, > + .tv_nsec = 0, > + }, > + .clock_type = CPU_HOG_CLOCK_PROCESS, > + }; > + > + /* Try to keep niced CPU usage as constrained to hog_cpu as > possible */ > + nice(1); > + cg_run(cpucg, hog_cpus_timed, (void *)¶m); Notice that cg_run() does fork itself internally. So you can call hog_cpus_timed(cpucg, (void *)¶m) directly, no need for the fork with cg_run(). (Alternatively substitute fork in this test with the fork in cg_run() but with extension of cpu_hog_func_params with the nice value.) Thanks, Michal signature.asc Description: PGP signature