linux-doc.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
From: Valentin Schneider <valentin.schneider@arm.com>
To: Beata Michalska <beata.michalska@arm.com>, linux-kernel@vger.kernel.org
Cc: peterz@infradead.org, mingo@redhat.com, juri.lelli@redhat.com,
	vincent.guittot@linaro.org, dietmar.eggemann@arm.com,
	corbet@lwn.net, linux-doc@vger.kernel.org
Subject: Re: [RFC PATCH v2 2/3] sched/topology: Rework CPU capacity asymmetry detection
Date: Wed, 05 May 2021 19:17:39 +0100	[thread overview]
Message-ID: <87czu5xcxo.mognet@arm.com> (raw)
In-Reply-To: <1619602363-1305-3-git-send-email-beata.michalska@arm.com>


Nitpicks ahead...

On 28/04/21 10:32, Beata Michalska wrote:
> @@ -1958,65 +1958,308 @@ static bool topology_span_sane(struct sched_domain_topology_level *tl,
>
>       return true;
>  }
> +/**
> + * Asym capacity bits
> + */
> +
> +/**
> + * Cached cpu masks for those sched domains, at a given topology level,
> + * that do represent CPUs with asymmetric capacities.
> + *
> + * Each topology level will get the cached data assigned,
> + * with asym cap sched_flags (SD_ASYM_CPUCAPACITY and SD_ASYM_CPUCAPACITY_FULL
> + * accordingly) and the corresponding cpumask for:
> + * - domains that do span CPUs with different capacities
> + * - domains where all CPU capacities are visible for all CPUs within
> + *   the domain
> + *
> + * Within a single topology level there might be domains
> + * with different scope of asymmetry:
> + *	none     -> .
> + *	partial  -> SD_ASYM_CPUCAPACITY
> + *	full     -> SD_ASYM_CPUCAPACITY|SD_ASYM_CPUCAPACITY_FULL
> + */
> +struct asym_cache_data {
> +
  ^
That should go

[...]

> -	if (!asym)
> -		return NULL;
> +	/* No asymmetry detected so skip the rest */
> +	if (!(cap_count > 1))
> +		goto leave;
> +
> +	if (!alloc_cpumask_var(&cpu_mask, GFP_KERNEL))
> +		goto leave;
>
> +	/* Get the number of topology levels */
> +	for_each_sd_topology(tl) level_count++;
>       /*
> -	 * Examine topology from all CPU's point of views to detect the lowest
> -	 * sched_domain_topology_level where a highest capacity CPU is visible
> -	 * to everyone.
> +	 * Allocate an array to store cached data per each topology level
>        */

That comment can be squashed into a single line.

> -	for_each_cpu(i, cpu_map) {
> -		unsigned long max_capacity = arch_scale_cpu_capacity(i);
> -		int tl_id = 0;
> +	scan_data = kcalloc(level_count, sizeof(*scan_data), GFP_KERNEL);
> +	if (!scan_data) {
> +		free_cpumask_var(cpu_mask);
> +		goto leave;
> +	}
>
> -		for_each_sd_topology(tl) {
> -			if (tl_id < asym_level)
> -				goto next_level;
> +	level_count = 0;
> +
> +	for_each_sd_topology(tl) {
> +		unsigned int local_cap_count;
> +		bool full_asym = true;
> +		const struct cpumask *mask;
> +		struct asym_cache_data *data = &scan_data[level_count++];
>
> -			for_each_cpu_and(j, tl->mask(i), cpu_map) {
> -				unsigned long capacity;
> +#ifdef CONFIG_NUMA
> +		/*
> +		 * For NUMA we might end-up in a sched domain that spans numa
> +		 * nodes with cpus with different capacities which would not be
> +		 * caught  by the above scan as those will have separate
                          ^
Stray whitespace >>>>>>>>>^

> +		 * cpumasks - subject to numa level
> +		 * @see: sched_domains_curr_level & sd_numa_mask
> +		 * Considered to be a no-go
> +		 */
> +		if (WARN_ON_ONCE(tl->numa_level && !full_asym))
> +			goto leave;
> +#endif
>
> -				capacity = arch_scale_cpu_capacity(j);
> +		if (asym_tl) {
> +			data->sched_flags = SD_ASYM_CPUCAPACITY |
> +					    SD_ASYM_CPUCAPACITY_FULL;
> +			continue;
> +		}
>
> -				if (capacity <= max_capacity)
> -					continue;
> +		cpumask_copy(cpu_mask, cpu_map);
> +		cpu = cpumask_first(cpu_mask);
> +
> +		while (cpu < nr_cpu_ids) {
> +			int i;
> +
> +			/*
> +			 * Tracking each CPU capacity 'scan' id to distinguish
> +			 * discovered capacity sets between different CPU masks
> +			 * at each topology level: capturing unique capacity
> +			 * values at each scan stage
> +			 */
> +			++scan_id;
> +			local_cap_count = 0;
>
> -				max_capacity = capacity;
> -				asym_level = tl_id;
> -				asym_tl = tl;
> +			mask = tl->mask(cpu);
> +			for_each_cpu_and(i, mask, cpu_map) {
> +
  ^
This should go.

> +				capacity = arch_scale_cpu_capacity(i);
> +
> +				list_for_each_entry(entry, &capacity_set, link) {
> +					if (entry->capacity == capacity &&
> +					    entry->scan_level < scan_id) {
> +						entry->scan_level = scan_id;
> +						++local_cap_count;
> +					}
> +				}
> +				__cpumask_clear_cpu(i, cpu_mask);
>                       }
> -next_level:
> -			tl_id++;
> +			if (cap_count != local_cap_count)
> +				full_asym = false;
> +			if (local_cap_count > 1) {
> +				int flags = (cap_count != local_cap_count)
> +					 ? SD_ASYM_CPUCAPACITY
> +					 : SD_ASYM_CPUCAPACITY
> +					 | SD_ASYM_CPUCAPACITY_FULL;

I haven't found many ternaries split over several lines, but those seem to
still follow the "operand at EOL" thing. The end result isn't particularly
pretty here (which is quite subjective, I know); another way to express
this would be:

                                int flags = SD_ASYM_CPUCAPACITY |
                                        SD_ASYM_CPUCAPACITY_FULL *
                                        (cap_count == local_cap_count);

which is... Meh.

  reply	other threads:[~2021-05-05 18:17 UTC|newest]

Thread overview: 8+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2021-04-28  9:32 [RFC PATCH v2 0/3] " Beata Michalska
2021-04-28  9:32 ` [RFC PATCH v2 1/3] sched/core: Introduce SD_ASYM_CPUCAPACITY_FULL sched_domain flag Beata Michalska
2021-04-28  9:32 ` [RFC PATCH v2 2/3] sched/topology: Rework CPU capacity asymmetry detection Beata Michalska
2021-05-05 18:17   ` Valentin Schneider [this message]
2021-04-28  9:32 ` [RFC PATCH v2 3/3] sched/doc: Update the CPU capacity asymmetry bits Beata Michalska
2021-04-28 17:19   ` Randy Dunlap
2021-05-10 16:33     ` Beata Michalska
2021-05-05 18:17 ` [RFC PATCH v2 0/3] Rework CPU capacity asymmetry detection Valentin Schneider

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=87czu5xcxo.mognet@arm.com \
    --to=valentin.schneider@arm.com \
    --cc=beata.michalska@arm.com \
    --cc=corbet@lwn.net \
    --cc=dietmar.eggemann@arm.com \
    --cc=juri.lelli@redhat.com \
    --cc=linux-doc@vger.kernel.org \
    --cc=linux-kernel@vger.kernel.org \
    --cc=mingo@redhat.com \
    --cc=peterz@infradead.org \
    --cc=vincent.guittot@linaro.org \
    --subject='Re: [RFC PATCH v2 2/3] sched/topology: Rework CPU capacity asymmetry detection' \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).