Re: [PATCH v4 10/18] PM: EM: Add RCU mechanism which safely cleans the old data

[Lukasz Luba]

On 9/29/23 13:59, Rafael J. Wysocki wrote:
> On Fri, Sep 29, 2023 at 11:36 AM Lukasz Luba <lukasz.luba@xxxxxxx> wrote:

[snip]

> > We had a few internal reviews and there were voices where saying that
> > it's better to have 2 identical tables: 'default_table' and
> > 'runtime_table' to make sure it's visible everywhere when it's used.
> > That made the need to actually have also the 'state' table inside.
> > I don't see it as a big problem, though.
>
> What I'm trying to say is that you can allocate runtime_table along
> with the table pointed to by its state field in one invocation of
> kzalloc() (say).
>
> Having just one memory region to free eventually instead of two of
> them would help to avoid some complexity, especially in the next
> patch.

I think, I know what you mean, basically:

------------------------------
struct em_perf_table {
	struct rcu_head rcu;
	struct em_perf_state state[];
}

kzalloc(sizeof(struct em_perf_table) + N * sizeof(struct em_perf_state))

------

IMO that should also be OK in the rest of places.
I agree the alloc/free code would be smaller.

Let me do that than.

> > > > +}
> > > > +
> > > > +static void em_perf_runtime_table_set(struct device *dev,
> > > > +                                     struct em_perf_table *runtime_table)
> > > > +{
> > > > +       struct em_perf_domain *pd = dev->em_pd;
> > > > +       struct em_perf_table *tmp;
> > > > +
> > > > +       tmp = pd->runtime_table;
> > > > +
> > > > +       rcu_assign_pointer(pd->runtime_table, runtime_table);
> > > > +
> > > > +       em_cpufreq_update_efficiencies(dev, runtime_table->state);
> > > > +
> > > > +       /* Don't free default table since it's used by other frameworks. */
> > >
> > > Apparently, some frameworks are only going to use the default table
> > > while the runtime-updatable table will be used somewhere else at the
> > > same time.
> > >
> > > I'm not really sure if this is a good idea.
> >
> > Runtime table is only for driving the task placement in the EAS.
> >
> > The thermal gov IPA won't make better decisions because it already
> > has the mechanism to accumulate the error that it made.
> >
> > The same applies to DTPM, which works in a more 'configurable' way,
> > rather that hard optimization mechanism (like EAS).
>
> My understanding of the above is that the other EM users don't really
> care that much so they can get away with using the default table all
> the time, but EAS needs more accuracy, so the table used by it needs
> to be adjusted in certain situations.

Yes

> Fair enough, I'm assuming that you've done some research around it.
> Still, this is rather confusing.

Yes, I have presented those ~2y ago in Android Gerrit world
(got feedback from a few vendors) and in a few Linux conferences.

For now we don't plan to have this feature for the thermal
governor or something similar.

> > > > +       if (tmp != pd->default_table)
> > > > +               call_rcu(&tmp->rcu, em_destroy_rt_table_rcu);
> >
> > The em_destroy_rt_table_rcu() is used here ^^^^^^