On Wed, May 19, 2021 at 01:30:05PM +0200, Peter Zijlstra wrote:
>
> Mostly style nits, since I read you're already looking at reworking this
> due to other feedback, do with it what you like.
>
Will apply your remarks on whatever ends up in the new version, which should be
most of it. To be out soon.
Thank You
---
BR
B.
> 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 { }
>
> > }
