Messages in this thread | | | From | Valentin Schneider <> | Subject | Re: [PATCH v4 05/10] sched/topology: Define and assign sched_domain flag metadata | Date | Thu, 06 Aug 2020 17:18:45 +0100 |
| |
On 06/08/20 15:07, Ingo Molnar wrote: > * Valentin Schneider <valentin.schneider@arm.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
| |