Re: [PATCH v4 09/18] PM: EM: Introduce runtime modifiable table

[Lukasz Luba]

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

[snip]

> > > >           em_debug_remove_pd(dev);
> > > >
> > > > +       runtime_table = pd->runtime_table;
> > > > +
> > > > +       rcu_assign_pointer(pd->runtime_table, NULL);
> > > > +       synchronize_rcu();
> > >
> > > Is it really a good idea to call this under a mutex?
> >
> > This is the unregistration of the EM code path, so a thermal
> > driver which gets some IRQs might not be aware that the EM
> > is going to vanish. That's why those two code paths: update
> > & unregister are protected with the same lock.
> >
> > This synchronize_rcu() won't be long,
>
> Are you sure?  This potentially waits for all of the CPUs in the
> system to go through a quiescent state which may take a while in
> principle.
>
> In any case, though, this effectively makes everyone waiting for the
> mutex also wait for the grace period to elapse and they may not care
> about it.

My apologies for the delay, I had to check this. Yes, should be possible
and safe to not wait here as you described on this synchronize_rcu().

What I have drawn in other response to patch 11/18 [1] should still be
true.

Thanks, I will remove this sync call from here.

> > but makes sure that when we free(dev->em_pd) we don't leak runtime_table.
> >
> > > > +
> > > >           kfree(pd->default_table->state);
> > > >           kfree(pd->default_table);
> > > >           kfree(dev->em_pd);
> > > > +
> > >
> > > Unrelated change.
> >
> > ACK
> >
> > > >           dev->em_pd = NULL;
> > > >           mutex_unlock(&em_pd_mutex);
> > > >    }
> > > > --
> > >
> > > So this really adds a pointer to a table that can be dynamically
> > > updated to struct em_perf_domain without any users so far.  It is not
> > > used anywhere as of this patch AFAICS, which is not what the changelog
> > > is saying.
> >
> > Good catch. I will adjust the changlog in header and say:
> >
> > 'Add infrastructure and mechanisms for the new runtime table.
> > The runtime modifiable EM data is used by the Energy Aware Scheduler
> > (EAS)for the task placement.
>
> I would make it more clear that this is going to happen after some
> other subsequent changes.

OK, I will add that information too.

[1] 
https://lore.kernel.org/lkml/91d6e9be-d50c-d157-55a0-79134cbd01fb@xxxxxxx/