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
Date: Wed, 19 May 2021 13:30:05 +0200 [thread overview]
Message-ID: <YKT2vbluMgcu94M6@hirez.programming.kicks-ass.net> (raw)
In-Reply-To: <1621239831-5870-3-git-send-email-beata.michalska@arm.com>
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 { }
> }
next prev parent reply other threads:[~2021-05-19 11:30 UTC|newest]
Thread overview: 20+ messages / expand[flat|nested] mbox.gz Atom feed top
2021-05-17 8:23 [PATCH v4 0/3] Rework CPU capacity asymmetry detection Beata Michalska
2021-05-17 8:23 ` [PATCH v4 1/3] sched/core: Introduce SD_ASYM_CPUCAPACITY_FULL sched_domain flag Beata Michalska
2021-05-18 13:39 ` Vincent Guittot
2021-05-18 14:27 ` Beata Michalska
2021-05-18 14:53 ` Vincent Guittot
2021-05-18 15:09 ` Beata Michalska
2021-05-18 15:28 ` Vincent Guittot
2021-05-18 15:47 ` Beata Michalska
2021-05-18 15:56 ` Vincent Guittot
2021-05-18 16:34 ` Beata Michalska
2021-05-17 8:23 ` [PATCH v4 2/3] sched/topology: Rework CPU capacity asymmetry detection Beata Michalska
2021-05-17 12:04 ` Valentin Schneider
2021-05-17 13:18 ` Beata Michalska
2021-05-17 15:06 ` Valentin Schneider
2021-05-18 14:40 ` Beata Michalska
2021-05-18 15:53 ` Valentin Schneider
2021-05-18 17:10 ` Beata Michalska
2021-05-19 11:30 ` Peter Zijlstra [this message]
2021-05-19 19:48 ` Beata Michalska
2021-05-17 8:23 ` [PATCH v4 3/3] sched/doc: Update the CPU capacity asymmetry bits Beata Michalska
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=YKT2vbluMgcu94M6@hirez.programming.kicks-ass.net \
--to=peterz@infradead.org \
--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=rdunlap@infradead.org \
--cc=valentin.schneider@arm.com \
--cc=vincent.guittot@linaro.org \
/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
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
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).