Linux-ARM-Kernel Archive on lore.kernel.org
 help / color / Atom feed
From: Ionela Voinescu <ionela.voinescu@arm.com>
To: "Rafael J. Wysocki" <rafael@kernel.org>
Cc: Catalin Marinas <catalin.marinas@arm.com>,
	Linux PM <linux-pm@vger.kernel.org>,
	Peter Zijlstra <peterz@infradead.org>,
	Viresh Kumar <viresh.kumar@linaro.org>,
	"Rafael J. Wysocki" <rjw@rjwysocki.net>,
	Russell King - ARM Linux <linux@armlinux.org.uk>,
	Linux Kernel Mailing List <linux-kernel@vger.kernel.org>,
	Ingo Molnar <mingo@redhat.com>,
	Sudeep Holla <sudeep.holla@arm.com>,
	Will Deacon <will@kernel.org>,
	Dietmar Eggemann <dietmar.eggemann@arm.com>,
	Linux ARM <linux-arm-kernel@lists.infradead.org>
Subject: Re: [PATCH v2 4/7] cpufreq: report whether cpufreq supports Frequency Invariance (FI)
Date: Wed, 29 Jul 2020 15:39:13 +0100
Message-ID: <20200729143903.GC12941@arm.com> (raw)
In-Reply-To: <CAJZ5v0jvvEvxs2dwsGd1kerTT3pJTJj6fVg0ndtdeUhiq+K_UQ@mail.gmail.com>

Hi,

On Monday 27 Jul 2020 at 16:02:18 (+0200), Rafael J. Wysocki wrote:
> On Wed, Jul 22, 2020 at 11:38 AM Ionela Voinescu
> <ionela.voinescu@arm.com> wrote:
[..]
> > +static inline
> > +void enable_cpufreq_freq_invariance(struct cpufreq_driver *driver)
> > +{
> > +       if ((driver->target || driver->target_index || driver->fast_switch) &&
> > +           !driver->setpolicy) {
> > +
> > +               static_branch_enable_cpuslocked(&cpufreq_set_freq_scale);
> > +               pr_debug("%s: Driver %s can provide frequency invariance.",
> > +                        __func__, driver->name);
> > +       } else
> > +               pr_err("%s: Driver %s cannot provide frequency invariance.",
> > +               __func__, driver->name);
> 
> This doesn't follow the kernel coding style (the braces around the
> pr_err() statement are missing).
> 

I'll fix this.

Also, depending on the result of the discussion below, it might be best
for this to be a warning, not an error.

> Besides, IMO on architectures where arch_set_freq_scale() is empty,
> this should be empty as well.
>

Yes, you are right, there are two aspects here:
 - (1) Whether a driver *can* provide frequency invariance. IOW, whether
   it implements the callbacks that result in the call to
   arch_set_freq_scale().

 - (2) Whether cpufreq/driver *does* provide frequency invariance. IOW,
   whether the call to arch_set_freq_scale() actually results in the
   setting of the scale factor.

Even when creating this v2 I was going back and forth between the options
for this:

(a) cpufreq should report whether it *can* provide frequency invariance
    (as described at (1)). If we go for this, for clarity I should change

    s/cpufreq_set_freq_scale/cpufreq_can_set_freq_scale_key
    s/cpufreq_sets_freq_scale()/cpufreq_can_set_freq_scale()

    Through this, cpufreq only reports that it calls
    arch_set_freq_scale(), independent on whether that call results in a
    scale factor being set. Then it would be up to the caller to ensure
    this information is used with a proper definition of
    arch_set_freq_scale().

(b) cpufreq should report whether it *does* provide frequency invariance

    A way of doing this is to use a arch_set_freq_scale define (as done
    for other arch functions, for example arch_scale_freq_tick()) and
    guard this enable_cpufreq_freq_invariance() function based on that
    definition.
    Therefore, cpufreq_sets_freq_scale() would report whether
    enable_cpufreq_freq_invariance() was successful and there is an
    external definition of arch_set_freq_scale() that sets the scale
    factor.


The current version is somewhat a combination of (a) and (b):
cpufreq_set_freq_scale would initially be enabled if the proper callbacks
are implemented (a), but later if the weak version of
arch_set_freq_scale() is called, we disabled it (b) (as can be seen below).

[..]
> >  __weak void arch_set_freq_scale(struct cpumask *cpus, unsigned long cur_freq,
> >                 unsigned long max_freq)
> >  {
> > +       if (cpufreq_sets_freq_scale())
> > +               static_branch_disable_cpuslocked(&cpufreq_set_freq_scale);
> > +
> >  }
> >  EXPORT_SYMBOL_GPL(arch_set_freq_scale);

I suppose a clear (a) or (b) solution might be better here.

IMO, given that (b) cannot actually guarantee that a scale factor is set
through arch_set_freq_scale() given cpufreq information about current and
maximum frequencies, for me (a) is preferred as it conveys the only
information that cpufreq can convey - the fact that it *can* set the scale
factor, not that it *does*.

Can you please confirm whether you still prefer (b), given the details
above?

Thank you,
Ionela.

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

  reply index

Thread overview: 35+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2020-07-22  9:37 [PATCH v2 0/7] cpufreq: improve frequency invariance support Ionela Voinescu
2020-07-22  9:37 ` [PATCH v2 1/7] cpufreq: move invariance setter calls in cpufreq core Ionela Voinescu
2020-07-27 13:48   ` Rafael J. Wysocki
2020-07-29  9:03     ` Ionela Voinescu
2020-07-30  3:41     ` Viresh Kumar
2020-08-03 13:26       ` Ionela Voinescu
2020-08-03 13:46         ` Rafael J. Wysocki
2020-08-03 14:16           ` Ionela Voinescu
2020-07-22  9:37 ` [PATCH v2 2/7] cpufreq: set invariance scale factor on transition end Ionela Voinescu
2020-07-27 13:52   ` Rafael J. Wysocki
2020-07-29  9:14     ` Ionela Voinescu
2020-07-30  4:13   ` Viresh Kumar
2020-08-03 13:58     ` Ionela Voinescu
2020-08-04  6:26       ` Viresh Kumar
2020-08-05 10:35         ` Ionela Voinescu
2020-07-22  9:37 ` [PATCH v2 3/7] arch_topology: disable frequency invariance for CONFIG_BL_SWITCHER Ionela Voinescu
2020-07-30  4:24   ` Viresh Kumar
2020-07-30 10:29     ` Dietmar Eggemann
2020-07-31 15:48       ` Sudeep Holla
2020-08-03 14:39         ` Ionela Voinescu
2020-08-04  6:30       ` Viresh Kumar
2020-08-10  9:01         ` Ionela Voinescu
2020-07-22  9:37 ` [PATCH v2 4/7] cpufreq: report whether cpufreq supports Frequency Invariance (FI) Ionela Voinescu
2020-07-27 14:02   ` Rafael J. Wysocki
2020-07-29 14:39     ` Ionela Voinescu [this message]
2020-07-30  4:43   ` Viresh Kumar
2020-08-03 15:24     ` Ionela Voinescu
2020-08-04  6:46       ` Viresh Kumar
2020-08-05 10:35         ` Ionela Voinescu
2020-07-22  9:37 ` [PATCH v2 5/7] arch_topology, cpufreq, sched/core: constify arch_* cpumasks Ionela Voinescu
2020-07-30 11:43   ` Catalin Marinas
2020-07-22  9:37 ` [PATCH v2 6/7] arch_topology, arm, arm64: define arch_scale_freq_invariant() Ionela Voinescu
2020-07-30 11:44   ` [PATCH v2 6/7] arch_topology,arm,arm64: " Catalin Marinas
2020-07-22  9:37 ` [PATCH v2 7/7] cpufreq: make schedutil the default for arm and arm64 Ionela Voinescu
2020-07-30  4:54   ` Viresh Kumar

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=20200729143903.GC12941@arm.com \
    --to=ionela.voinescu@arm.com \
    --cc=catalin.marinas@arm.com \
    --cc=dietmar.eggemann@arm.com \
    --cc=linux-arm-kernel@lists.infradead.org \
    --cc=linux-kernel@vger.kernel.org \
    --cc=linux-pm@vger.kernel.org \
    --cc=linux@armlinux.org.uk \
    --cc=mingo@redhat.com \
    --cc=peterz@infradead.org \
    --cc=rafael@kernel.org \
    --cc=rjw@rjwysocki.net \
    --cc=sudeep.holla@arm.com \
    --cc=viresh.kumar@linaro.org \
    --cc=will@kernel.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

Linux-ARM-Kernel Archive on lore.kernel.org

Archives are clonable:
	git clone --mirror https://lore.kernel.org/linux-arm-kernel/0 linux-arm-kernel/git/0.git
	git clone --mirror https://lore.kernel.org/linux-arm-kernel/1 linux-arm-kernel/git/1.git

	# If you have public-inbox 1.1+ installed, you may
	# initialize and index your mirror using the following commands:
	public-inbox-init -V2 linux-arm-kernel linux-arm-kernel/ https://lore.kernel.org/linux-arm-kernel \
		linux-arm-kernel@lists.infradead.org
	public-inbox-index linux-arm-kernel

Example config snippet for mirrors

Newsgroup available over NNTP:
	nntp://nntp.lore.kernel.org/org.infradead.lists.linux-arm-kernel


AGPL code for this site: git clone https://public-inbox.org/public-inbox.git