linux-kernel - Re: [PATCH v4 05/10] sched/topology: Define and assign 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: <jhj8serixd4.mognet@arm.com>
Date:   Thu, 06 Aug 2020 17:18:45 +0100
From:   Valentin Schneider <valentin.schneider@....com>
To:     Ingo Molnar <mingo@...nel.org>
Cc:     linux-kernel@...r.kernel.org, peterz@...radead.org,
        vincent.guittot@...aro.org, dietmar.eggemann@....com,
        morten.rasmussen@....com, Quentin Perret <qperret@...gle.com>
Subject: Re: [PATCH v4 05/10] sched/topology: Define and assign sched_domain flag metadata

On 06/08/20 15:07, Ingo Molnar wrote:
> * Valentin Schneider <valentin.schneider@....com> wrote:
>
>> +#ifndef SD_FLAG
>> +#define SD_FLAG(x, y, z)
>> +#endif
>
> AFAICS there's not a single use of sd_flags.h that doesn't come with
> its own SD_FLAG definition, so I suppose this should be:
>
> #ifndef SD_FLAG
> # error "Should not happen."
> #endif
>
> ?
>

I can give this a try; for the context, I copied uapi/asm-generic/unistd.h
without thinking too much, and that does:

  #ifndef __SYSCALL
  #define __SYSCALL(x, y)
  #endif

> Also, some nits:
>
>> +/*
>> + * Expected flag uses
>> + *
>> + * SHARED_CHILD: These flags are meant to be set from the base domain upwards.
>> + * If a domain has this flag set, all of its children should have it set. This
>> + * is usually because the flag describes some shared resource (all CPUs in that
>> + * domain share the same foobar), or because they are tied to a scheduling
>> + * behaviour that we want to disable at some point in the hierarchy for
>> + * scalability reasons.
>
> s/foobar/resource
>
> ?
>

That's better indeed, I think that's a remnant of when I was listing a lot
of things that could be shared.

>> +/*
>> + * cross-node balancing
>> + *
>> + * SHARED_PARENT: Set for all NUMA levels above NODE.
>> + */
>> +SD_FLAG(SD_NUMA,                12, SDF_SHARED_PARENT)
>
> s/cross-node/Cross-node
>
> BTW., is there any particular reason why these need to be defines with
> a manual enumeration of flag values - couldn't we generate
> auto-enumerated C enums instead or so?
>

I remember exploring a few different options there, but it's been a while
already. I think one of the reasons I kept some form of explicit assignment
is to avoid making reading

  /proc/sys/kernel/sched_domain/cpu*/domain*/flags

more convoluted than it is now (i.e. you have to go fetch the
bit -> name translation in the source code).

In the grand scheme of things I'd actually like to have this file output
the names of the flags rather than their values (since I now save them when
SCHED_DEBUG=y), but I didn't find a simple way to hack the existing SD ctl
table (sd_alloc_ctl_domain_table() and co) into doing this.

Now as to making this fully automagic, I *think* I could do something like
having a first enum to set up an ordering:

  #define SD_FLAG(name, ...) __##name,
  enum {
    #include <linux/sched/sd_flags.h>
  };

A second one to have powers of 2:

  #define SD_FLAG(name, ...) name = 1 << __##name,
  enum {
    #include <linux/sched/sd_flags.h>
  };

And finally the metadata array assignment might be doable with:

  #define SD_FLAG(_name, mflags) [__##_name] = { .meta_flags = mflags, .name = #_name },

Or, if there is a way somehow to directly get powers of 2 out of an enum:

  #define SD_FLAG(_name, mflags) [_ffs(_name)] = { .meta_flags = mflags, .name = #_name },

>> +#ifdef CONFIG_SCHED_DEBUG
>> +#define SD_FLAG(_name, idx, mflags) [idx] = {.meta_flags = mflags, .name = #_name},
>
> s/{./{ .
> s/e}/e }
>
> Thanks,
>
>       Ingo