linux-kernel - Re: [PATCH v3 2/2] cgroup/rstat: Selftests for niced CPU statistics

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite for Android: free password hash cracker in your pocket

[<prev] [next>] [<thread-prev] [day] [month] [year] [list]

Message-ID: <CAN+CAwO8XEAkoBDc03Zveaci9hASaFvk8ybQ2Mwoy_VacqgRfA@mail.gmail.com>
Date: Mon, 30 Sep 2024 14:07:22 -0400
From: Joshua Hahn <joshua.hahnjy@...il.com>
To: Michal Koutný <mkoutny@...e.com>
Cc: tj@...nel.org, cgroups@...r.kernel.org, hannes@...xchg.org, 
	linux-kernel@...r.kernel.org, linux-kselftest@...r.kernel.org, 
	lizefan.x@...edance.com, shuah@...nel.org
Subject: Re: [PATCH v3 2/2] cgroup/rstat: Selftests for niced CPU statistics

On Thu, Sep 26, 2024 at 2:10 PM Michal Koutný <mkoutny@...e.com> wrote:
>
> On Mon, Sep 23, 2024 at 07:20:06AM GMT, Joshua Hahn <joshua.hahnjy@...il.com> 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 *)&param);
>
> Notice that cg_run() does fork itself internally.
> So you can call hog_cpus_timed(cpucg, (void *)&param) 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