linux-kernel - Re: [PATCH 7/9] sched,debug: Convert sysctl sched

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: Windows password security audit tool. GUI, reports in PDF.

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

Message-ID: <87czv6nzhz.mognet@arm.com>
Date:   Wed, 07 Apr 2021 11:46:32 +0100
From:   Valentin Schneider <valentin.schneider@....com>
To:     Peter Zijlstra <peterz@...radead.org>, mingo@...nel.org,
        mgorman@...e.de, juri.lelli@...hat.com, vincent.guittot@...aro.org,
        dietmar.eggemann@....com, rostedt@...dmis.org, bsegall@...gle.com,
        bristot@...hat.com, joshdon@...gle.com
Cc:     linux-kernel@...r.kernel.org, peterz@...radead.org, greg@...ah.com
Subject: Re: [PATCH 7/9] sched,debug: Convert sysctl sched_domains to debugfs

On 26/03/21 11:33, Peter Zijlstra wrote:
> Stop polluting sysctl, move to debugfs for SCHED_DEBUG stuff.
>
> Signed-off-by: Peter Zijlstra (Intel) <peterz@...radead.org>
> ---
>  kernel/sched/debug.c    |  255 ++++++++++--------------------------------------
>  kernel/sched/sched.h    |    2
>  kernel/sched/topology.c |    1
>  3 files changed, 59 insertions(+), 199 deletions(-)
>

I do very much like to see a simple pair of seq_puts() replacing the
mess I put in there!

One comment below.

> --- a/kernel/sched/debug.c
> +++ b/kernel/sched/debug.c
> +void register_sched_domain_sysctl(void)
> +{
> +	int cpu, i;
>
>       if (!cpumask_available(sd_sysctl_cpus)) {
>               if (!alloc_cpumask_var(&sd_sysctl_cpus, GFP_KERNEL))
>                       return;
> -	}
> -
> -	if (!init_done) {
> -		init_done = true;
> -		/* init to possible to not have holes in @cpu_entries */
>               cpumask_copy(sd_sysctl_cpus, cpu_possible_mask);
>       }
>
> -	for_each_cpu(i, sd_sysctl_cpus) {
> -		struct ctl_table *e = cpu_idx[i];
> +	if (!sd_dentry)
> +		sd_dentry = debugfs_create_dir("domains", debugfs_sched);
>
> -		if (e->child)
> -			sd_free_ctl_entry(&e->child);
> +	for_each_cpu(cpu, sd_sysctl_cpus) {
> +		struct sched_domain *sd;
> +		struct dentry *d_cpu;
> +		char buf[32];
> +
> +		snprintf(buf, sizeof(buf), "cpu%d", cpu);
> +		debugfs_remove(debugfs_lookup(buf, sd_dentry));
> +		d_cpu = debugfs_create_dir(buf, sd_dentry);
> +
> +		i = 0;
> +		for_each_domain(cpu, sd) {
> +			struct dentry *d_sd;
>
> -		if (!e->procname) {
> -			snprintf(buf, 32, "cpu%d", i);
> -			e->procname = kstrdup(buf, GFP_KERNEL);
> -		}
> -		e->mode = 0555;
> -		e->child = sd_alloc_ctl_cpu_table(i);
> +			snprintf(buf, sizeof(buf), "domain%d", i);
> +			d_sd = debugfs_create_dir(buf, d_cpu);
>
> -		__cpumask_clear_cpu(i, sd_sysctl_cpus);

That seems to be the only place we cleared a CPU in that cpumask, and I
don't see its replacement, which would go against:

  bbdacdfed2f5 ("sched/debug: Optimize sched_domain sysctl generation")

With my very limited understanding of debugfs and sysctl, it seems that
before we would have some stuff saved in sd_ctl_table and free/reinit just
the bits we needed. With debugfs_remove(), I think we wipe everything
clean, or did I read that wrong?