Re: [PATCH v4 2/3] sched/topology: Rework CPU capacity asymmetry detection

[Beata Michalska <beata.michalska@arm.com>]

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 { }
> 
> >  }