Mostly style nits, since I read you're already looking at reworking this
due to other feedback, do with it what you like.
On Mon, May 17, 2021 at 09:23:50AM +0100, Beata Michalska wrote:
> @@ -1989,66 +1989,96 @@ static bool topology_span_sane(struct sched_domain_topology_level *tl,
>
> return true;
> }
+ whitespace
> +/**
> + * Asym capacity bits
> + */
> +struct asym_cap_data {
> + struct list_head link;
> + unsigned long capacity;
> + struct cpumask *cpu_mask;
> +};
+ whitespace
> /*
> + * Set of available CPUs grouped by their corresponding capacities
> + * Each list entry contains a CPU mask reflecting CPUs that share the same
> + * capacity.
> + * The lifespan of data is unlimited.
> */
> +static LIST_HEAD(asym_cap_list);
>
> +/*
> + * Verify whether given CPU at a given topology level belongs to a sched domain
> + * that does span CPUs with different capacities.
> + * Provides sd_flags reflecting the asymmetry scope.
> + */
> +static inline int
> +asym_cpu_capacity_classify(struct sched_domain_topology_level *tl, int cpu)
> +{
> + int sd_asym_flags = SD_ASYM_CPUCAPACITY | SD_ASYM_CPUCAPACITY_FULL;
> + const struct cpumask *tl_mask = tl->mask(cpu);
> + struct asym_cap_data *entry;
> + int asym_cap_count = 0;
> +
> + if (list_is_singular(&asym_cap_list))
> + goto leave;
> +
> + list_for_each_entry(entry, &asym_cap_list, link) {
> + if (cpumask_intersects(tl_mask, entry->cpu_mask))
> + ++asym_cap_count;
> + else
> + sd_asym_flags &= ~SD_ASYM_CPUCAPACITY_FULL;
> }
> + WARN_ON_ONCE(!asym_cap_count);
> +leave:
> + return asym_cap_count > 1 ? sd_asym_flags : 0;
> +}
>
>
- whitespace
> +/*
> + * Build-up/update list of CPUs grouped by their capacities
> + */
> +static void asym_cpu_capacity_scan(const struct cpumask *cpu_map)
> +{
> + struct asym_cap_data *entry, *next;
> + int cpu;
>
> + if (!list_empty(&asym_cap_list))
> + list_for_each_entry(entry, &asym_cap_list, link)
> + cpumask_clear(entry->cpu_mask);
two nits:
- the if() needs { } because while what follows is strictly a single
statement, it is multi-line, so coding style requires { }.
- the if() is strictly superfluous, if the list is empty the
list_for_each_entry() iteration already doesn't do anything.
>
> + entry = list_first_entry_or_null(&asym_cap_list,
> + struct asym_cap_data, link);
Please align line-breaks at the most nested (, vim can help you do this
with: set cino=(0:0, if you're using that other editor, I'm sure you can
convince it to align properly too :-)
>
> + for_each_cpu(cpu, cpu_map) {
> + unsigned long capacity = arch_scale_cpu_capacity(cpu);
>
> + if (entry && capacity == entry->capacity)
> + goto next;
>
> + list_for_each_entry(entry, &asym_cap_list, link)
> + if (capacity == entry->capacity)
> + goto next;
{ } again
> +
> + entry = kzalloc(sizeof(*entry) + cpumask_size(), GFP_KERNEL);
> + if (entry) {
> + entry->capacity = capacity;
> + entry->cpu_mask = (struct cpumask *)((char *)entry +
> + sizeof(*entry));
alignment again
> + list_add(&entry->link, &asym_cap_list);
> }
> + WARN_ONCE(!entry,
> + "Failed to allocate memory for capacity asymmetry detection\n");
alignment again
(also, eeew, if this lives, perhaps a find_asym_data(capacity) helper
might make it better:
if (!entry || entry->capacity != capacity)
entry = find_asym_data(capacity);
)
> +next:
> + __cpumask_set_cpu(cpu, entry->cpu_mask);
> }
>
> + list_for_each_entry_safe(entry, next, &asym_cap_list, link) {
> + if (cpumask_empty(entry->cpu_mask)) {
> + list_del(&entry->link);
> + kfree(entry);
> + }
> + }
See, this has { }
> }
