Messages in this thread | | | Date | Fri, 6 Oct 2023 09:03:44 +0100 | Subject | Re: [PATCH v4 09/18] PM: EM: Introduce runtime modifiable table | From | 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@arm.com> 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@arm.com/
| |