Re: [PATCH v6 11/23] PM: EM: Add API for updating the runtime modifiable EM

[Lukasz Luba]

On 1/4/24 19:47, Rafael J. Wysocki wrote:
> 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"

Fair enough

> On Thu, Jan 4, 2024 at 6:15 PM Lukasz Luba <lukasz.luba@xxxxxxx> wrote:
> > Add API function em_dev_update_perf_domain() which allows to safely
> > change the EM.
>
> "... which allows the EM to be changed safely."

OK

> 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."

OK

> > Signed-off-by: Lukasz Luba <lukasz.luba@xxxxxxx>
> > ---
> >   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_states,
> >                                  struct em_data_callback *cb, cpumask_t *span,
> >                                  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_domain *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.

good catch

> > + *
> > + * Update EM runtime modifiable table for the @dev using the provided @table.
> > + *
> > + * This function uses mutex to serialize writers, so it must not be called
>
> "uses a mutex"

OK

> > + * from non-sleeping context.
>
> "a non-sleeping context".

OK

> > + *
> > + * 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.

Agree, I'll change it.

> > + */
> > +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.

True, I will put it about the lock.

> > +               mutex_unlock(&em_pd_mutex);
> > +               return -EINVAL;
> > +       }
> > +       pd = dev->em_pd;
> > +
> > +       em_table_inc(new_table);
> > +
> > +       old_table = 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;
> > --


Thank you for the review!

Regards,
Lukasz