All of lore.kernel.org
 help / color / mirror / Atom feed
From: Valentin Schneider <valentin.schneider@arm.com>
To: Beata Michalska <beata.michalska@arm.com>
Cc: linux-kernel@vger.kernel.org, peterz@infradead.org,
	mingo@redhat.com, juri.lelli@redhat.com,
	vincent.guittot@linaro.org, dietmar.eggemann@arm.com,
	corbet@lwn.net, rdunlap@infradead.org, linux-doc@vger.kernel.org
Subject: Re: [PATCH v5 2/3] sched/topology: Rework CPU capacity asymmetry detection
Date: Tue, 25 May 2021 10:53:07 +0100	[thread overview]
Message-ID: <87a6oj6sxo.mognet@arm.com> (raw)
In-Reply-To: <20210524225508.GA14880@e120325.cambridge.arm.com>

On 24/05/21 23:55, Beata Michalska wrote:
> On Mon, May 24, 2021 at 07:01:04PM +0100, Valentin Schneider wrote:
>> On 24/05/21 11:16, Beata Michalska wrote:
>> > This patch also removes the additional -dflags- parameter used when
>>   ^^^^^^^^^^^^^^^^^^^^^^^
>> s/^/Also remove/
> I would kind of ... disagree.
> All the commit msg is constructed using passive structure, the suggestion
> would actually break that. And it does 'sound' bit imperative but I guess
> that is subjective. I'd rather stay with impersonal structure (which is
> applied through out the whole patchset).

It's mainly about the 'This patch' formulation, some take exception to that :-)

>>
>> > building sched domains as the asymmetry flags are now being set
>> > directly in sd_init.
>> >
>>
>> Few nits below, but beyond that:
>>
>> Tested-by: Valentin Schneider <valentin.schneider@arm.com>
>> Reviewed-by: Valentin Schneider <valentin.schneider@arm.com>
>>
> Thanks a lot for the review and testing!
>
>> > +static inline int
>> > +asym_cpu_capacity_classify(struct sched_domain *sd,
>> > +			   const struct cpumask *cpu_map)
>> > +{
>> > +	int sd_asym_flags = SD_ASYM_CPUCAPACITY | SD_ASYM_CPUCAPACITY_FULL;
>> > +	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(sched_domain_span(sd), entry->cpu_mask)) {
>> > +			++asym_cap_count;
>> > +		} else {
>> > +			/*
>> > +			 * CPUs with given capacity might be offline
>> > +			 * so make sure this is not the case
>> > +			 */
>> > +			if (cpumask_intersects(entry->cpu_mask, cpu_map)) {
>> > +				sd_asym_flags &= ~SD_ASYM_CPUCAPACITY_FULL;
>> > +				if (asym_cap_count > 1)
>> > +					break;
>> > +			}
>>
>> Readability nit: That could be made into an else if ().
> It could but then this way the -comment- gets more exposed.
> But that might be my personal perception so I can change that.

As always those are quite subjective! Methink something like this would
still draw attention to the offline case:

               /*
                * Count how many unique capacities this domain covers. If a
                * capacity isn't covered, we need to check if any CPU with
                * that capacity is actually online, otherwise it can be
                * ignored.
                */
                if (cpumask_intersects(sched_domain_span(sd), entry->cpu_mask)) {
                        ++asym_cap_count;
                } else if (cpumask_intersects(entry->cpu_mask, cpu_map)) {
                        sd_asym_flags &= ~SD_ASYM_CPUCAPACITY_FULL;
                        if (asym_cap_count > 1)
                                break;
                }

  parent reply	other threads:[~2021-05-25  9:53 UTC|newest]

Thread overview: 29+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2021-05-24 10:16 [PATCH v5 0/3] Rework CPU capacity asymmetry detection Beata Michalska
2021-05-24 10:16 ` [PATCH v5 1/3] sched/core: Introduce SD_ASYM_CPUCAPACITY_FULL sched_domain flag Beata Michalska
2021-05-24 10:16 ` [PATCH v5 2/3] sched/topology: Rework CPU capacity asymmetry detection Beata Michalska
2021-05-24 18:01   ` Valentin Schneider
2021-05-24 22:55     ` Beata Michalska
2021-05-24 23:19       ` Beata Michalska
2021-05-25  9:53       ` Valentin Schneider [this message]
2021-05-25 10:29         ` Beata Michalska
2021-05-26  9:52           ` Dietmar Eggemann
2021-05-26 12:15             ` Beata Michalska
2021-05-26 12:51               ` Beata Michalska
2021-05-26 18:17                 ` Dietmar Eggemann
2021-05-26 21:40                   ` Beata Michalska
2021-05-27 15:08                     ` Dietmar Eggemann
2021-05-27 17:07                       ` Beata Michalska
2021-06-02 17:17                         ` Dietmar Eggemann
2021-06-02 19:48                           ` Beata Michalska
2021-06-03  9:09                             ` Dietmar Eggemann
2021-06-03  9:24                               ` Beata Michalska
2021-05-26 18:17               ` Dietmar Eggemann
2021-05-26 21:43                 ` Beata Michalska
2021-05-27  7:03             ` Peter Zijlstra
2021-05-27 12:22               ` Dietmar Eggemann
2021-05-27 12:32                 ` Beata Michalska
2021-05-25  8:25   ` Dietmar Eggemann
2021-05-25  9:30     ` Beata Michalska
2021-05-25 11:59       ` Dietmar Eggemann
2021-05-25 14:04         ` Beata Michalska
2021-05-24 10:16 ` [PATCH v5 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=87a6oj6sxo.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=rdunlap@infradead.org \
    --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 an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.