Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 23.128.96.18 as permitted sender) client-ip=23.128.96.18;
Date:   Wed, 19 May 2021 13:30:05 +0200
From:   Peter Zijlstra <peterz@infradead.org>
To:     Beata Michalska <beata.michalska@arm.com>
Cc:     linux-kernel@vger.kernel.org, mingo@redhat.com,
        juri.lelli@redhat.com, vincent.guittot@linaro.org,
        valentin.schneider@arm.com, dietmar.eggemann@arm.com,
        corbet@lwn.net, rdunlap@infradead.org, linux-doc@vger.kernel.org
Subject: Re: [PATCH v4 2/3] sched/topology: Rework CPU capacity asymmetry
 detection
Message-ID: <YKT2vbluMgcu94M6@hirez.programming.kicks-ass.net>
References: <1621239831-5870-1-git-send-email-beata.michalska@arm.com>
 <1621239831-5870-3-git-send-email-beata.michalska@arm.com>
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <1621239831-5870-3-git-send-email-beata.michalska@arm.com>
Precedence: bulk


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

>  }