Re: [PATCH v6 12/23] PM: EM: Add helpers to read under RCU lock the EM table

[Lukasz Luba]

On 1/4/24 19:55, Rafael J. Wysocki wrote:
> On Thu, Jan 4, 2024 at 6:15 PM Lukasz Luba <lukasz.luba@xxxxxxx> wrote:
> > To use the runtime modifiable EM table there is a need to use RCU
> > read locking properly. Add helper functions for the device drivers and
> > frameworks to make sure it's done properly.
> >
> > Signed-off-by: Lukasz Luba <lukasz.luba@xxxxxxx>
> > ---
> >   include/linux/energy_model.h | 19 +++++++++++++++++++
> >   1 file changed, 19 insertions(+)
> >
> > diff --git a/include/linux/energy_model.h b/include/linux/energy_model.h
> > index f33257ed83fd..cfaf5d8b1aad 100644
> > --- a/include/linux/energy_model.h
> > +++ b/include/linux/energy_model.h
> > @@ -338,6 +338,20 @@ static inline int em_pd_nr_perf_states(struct em_perf_domain *pd)
> >          return pd->nr_perf_states;
> >   }
> >
> > +static inline struct em_perf_state *em_get_table(struct em_perf_domain *pd)
> > +{
> > +       struct em_perf_table __rcu *table;
> > +
> > +       rcu_read_lock();
> > +       table = rcu_dereference(pd->em_table);
> > +       return table->state;
> > +}
> > +
> > +static inline void em_put_table(void)
> > +{
> > +       rcu_read_unlock();
> > +}
>
> The lack of symmetry between em_get_table() and em_put_table() is kind
> of confusing.
>
> I don't really like these wrappers.
>
> IMO it would be better to use rcu_read_lock()/rcu_read_unlock()
> directly everywhere they are needed and there can be a wrapper around
> rcu_dereference(pd->em_table), something like
>
> static inline struct em_perf_state *em_perf_state_from_pd(struct
> em_perf_domain *pd)
> {
>          return rcu_dereference(pd->em_table)->state;
> }

Fair enough, I will change this and use explicit rcu_read_lock/unlock()
in the thermal/DTPM code together with this above function.
I will add comment to it that it needs to be called under the RCU read
section locked.

Then also it would be easier to handle the function names in patch 10/23
that you have also commented.

> > +
> >   #else
> >   struct em_data_callback {};
> >   #define EM_ADV_DATA_CB(_active_power_cb, _cost_cb) { }
> > @@ -384,6 +398,11 @@ int em_dev_update_perf_domain(struct device *dev,
> >   {
> >          return -EINVAL;
> >   }
> > +static inline struct em_perf_state *em_get_table(struct em_perf_domain *pd)
> > +{
> > +       return NULL;
> > +}
> > +static inline void em_put_table(void) {}
> >   #endif
> >
> >   #endif
> > --