Received-SPF: pass (google.com: domain of linux-kernel+bounces-17182-linux.lists.archive=gmail.com@vger.kernel.org designates 139.178.88.99 as permitted sender) client-ip=139.178.88.99;
Precedence: bulk
MIME-Version: 1.0
References: <20240104171553.2080674-1-lukasz.luba@arm.com> <20240104171553.2080674-12-lukasz.luba@arm.com>
In-Reply-To: <20240104171553.2080674-12-lukasz.luba@arm.com>
From: "Rafael J. Wysocki" <rafael@kernel.org>
Date: Thu, 4 Jan 2024 20:47:19 +0100
Message-ID: <CAJZ5v0hqRkDmhRBfB4g-2EH2piv-KOQdwad7rVoSK8FzZKg=TA@mail.gmail.com>
Subject: Re: [PATCH v6 11/23] PM: EM: Add API for updating the runtime
 modifiable EM
To: Lukasz Luba <lukasz.luba@arm.com>
Cc: linux-kernel@vger.kernel.org, linux-pm@vger.kernel.org, rafael@kernel.org, 
	dietmar.eggemann@arm.com, rui.zhang@intel.com, amit.kucheria@verdurent.com, 
	amit.kachhap@gmail.com, daniel.lezcano@linaro.org, viresh.kumar@linaro.org, 
	len.brown@intel.com, pavel@ucw.cz, mhiramat@kernel.org, qyousef@layalina.io, 
	wvw@google.com
Content-Type: text/plain; charset="UTF-8"
Content-Transfer-Encoding: quoted-printable

I don't really like using the API TLA in patch subjects, because it
does not really say much.  IMO a subject like this would be better:

"PM: EM: Introduce em_dev_update_perf_domain() for EM updates"

On Thu, Jan 4, 2024 at 6:15=E2=80=AFPM Lukasz Luba <lukasz.luba@arm.com> wr=
ote:
>
> Add API function em_dev_update_perf_domain() which allows to safely
> change the EM.

"... which allows the EM to be changed safely."

New paragraph:

> The concurrent modifiers are protected by the mutex
> to serialize them. Removal of the old memory is asynchronous and
> handled by the RCU mechanisms.

"Concurrent updaters are serialized with a mutex and the removal of
memory that will not be used any more is carried out with the help of
RCU."

>
> Signed-off-by: Lukasz Luba <lukasz.luba@arm.com>
> ---
>  include/linux/energy_model.h |  8 +++++++
>  kernel/power/energy_model.c  | 41 ++++++++++++++++++++++++++++++++++++
>  2 files changed, 49 insertions(+)
>
> diff --git a/include/linux/energy_model.h b/include/linux/energy_model.h
> index 753d70d0ce7e..f33257ed83fd 100644
> --- a/include/linux/energy_model.h
> +++ b/include/linux/energy_model.h
> @@ -183,6 +183,8 @@ struct em_data_callback {
>
>  struct em_perf_domain *em_cpu_get(int cpu);
>  struct em_perf_domain *em_pd_get(struct device *dev);
> +int em_dev_update_perf_domain(struct device *dev,
> +                             struct em_perf_table __rcu *new_table);
>  int em_dev_register_perf_domain(struct device *dev, unsigned int nr_stat=
es,
>                                 struct em_data_callback *cb, cpumask_t *s=
pan,
>                                 bool microwatts);
> @@ -376,6 +378,12 @@ struct em_perf_table __rcu *em_allocate_table(struct=
 em_perf_domain *pd)
>         return NULL;
>  }
>  static inline void em_free_table(struct em_perf_table __rcu *table) {}
> +static inline
> +int em_dev_update_perf_domain(struct device *dev,
> +                             struct em_perf_table __rcu *new_table)
> +{
> +       return -EINVAL;
> +}
>  #endif
>
>  #endif
> diff --git a/kernel/power/energy_model.c b/kernel/power/energy_model.c
> index bbc406db0be1..496dc00835c6 100644
> --- a/kernel/power/energy_model.c
> +++ b/kernel/power/energy_model.c
> @@ -220,6 +220,47 @@ static int em_allocate_perf_table(struct em_perf_dom=
ain *pd,
>         return 0;
>  }
>
> +/**
> + * em_dev_update_perf_domain() - Update runtime EM table for a device
> + * @dev                : Device for which the EM is to be updated
> + * @table      : The new EM table that is going to be used from now

This is called "new_table" below.

> + *
> + * Update EM runtime modifiable table for the @dev using the provided @t=
able.
> + *
> + * This function uses mutex to serialize writers, so it must not be call=
ed

"uses a mutex"

> + * from non-sleeping context.

"a non-sleeping context".

> + *
> + * Return 0 on success or a proper error in case of failure.

It is not clear what "a proper error" means.  It would be better to
simply say "or an error code on failure" IMO.

> + */
> +int em_dev_update_perf_domain(struct device *dev,
> +                             struct em_perf_table __rcu *new_table)
> +{
> +       struct em_perf_table __rcu *old_table;
> +       struct em_perf_domain *pd;
> +
> +       /* Serialize update/unregister or concurrent updates */
> +       mutex_lock(&em_pd_mutex);
> +
> +       if (!dev || !dev->em_pd) {

dev need not be checked under the lock.

> +               mutex_unlock(&em_pd_mutex);
> +               return -EINVAL;
> +       }
> +       pd =3D dev->em_pd;
> +
> +       em_table_inc(new_table);
> +
> +       old_table =3D pd->em_table;
> +       rcu_assign_pointer(pd->em_table, new_table);
> +
> +       em_cpufreq_update_efficiencies(dev, new_table->state);
> +
> +       em_table_dec(old_table);
> +
> +       mutex_unlock(&em_pd_mutex);
> +       return 0;
> +}
> +EXPORT_SYMBOL_GPL(em_dev_update_perf_domain);
> +
>  static int em_create_runtime_table(struct em_perf_domain *pd)
>  {
>         struct em_perf_table __rcu *table;
> --