Received: by 2002:a05:6358:3188:b0:123:57c1:9b43 with SMTP id q8csp1734034rwd; Mon, 15 May 2023 02:10:35 -0700 (PDT) X-Google-Smtp-Source: ACHHUZ7O/5WFs5r+G7RRBeP+WOArmZ73afY+RbL6+7ZfiVMVszkWhbXrlRCoYiBWfDwx6hXta4xP X-Received: by 2002:a05:6a21:6d8e:b0:105:66d3:8572 with SMTP id wl14-20020a056a216d8e00b0010566d38572mr7020976pzb.24.1684141834650; Mon, 15 May 2023 02:10:34 -0700 (PDT) ARC-Seal: i=1; a=rsa-sha256; t=1684141834; cv=none; d=google.com; s=arc-20160816; b=qdnk+gjwUv8A+3iORqVXfvskBeGodMofp++IF0hjLh2MiDlMmWYtTjAO5H7bnjVgQg w6dC5IV80dUmfkYHwXX0jUeu72vPFId/a5UDDIL1AKrcucOrC1+KpoO87XXjadfqOxOc SyMgIk/rXCEHti6wZLQc3H/Cn9GrFyNnGI4F93cxeOzMRUhmtsKlxMdVUVeZVIxcBMYO Q2RUcp91kVPsPoM/IBn0scZZUfSqSFkJjWcZ+25cqSO4DwpfEGE8zK5566mSQOm1BIZ0 pw2TpkhLSX52eKyTD7UE1e4MMEFiuj3zWfGYgD3JWoYA1vB7aQk6ejjrOCgcvmzLWC6v mN0Q== ARC-Message-Signature: i=1; a=rsa-sha256; c=relaxed/relaxed; d=google.com; s=arc-20160816; h=list-id:precedence:content-transfer-encoding:in-reply-to :content-language:references:cc:to:subject:from:user-agent :mime-version:date:message-id; bh=uHHMaw8J1QwqeS0jdF8LI9tLXXVT9thqlZRrolXwRc8=; b=JJlcmXY64+sTsKxsu+ZcEknKlPUyx7lXF+MEPuMS1mE0waXf3iONWAScM0b1/vdOEz zI+aixnzy1kd1/+QH0QlwySn2rPJudUwTR/1vu3BsFl9IW+kjGaWPQh8NYkrbzSG1ENt ACLbQmXwgYO9kprdQITns8ZX/7skPDcDKMjSEuvNSDkcx8SrdoWd4nGMOd4rFvp3fmBI Z9C2KZ25FuCRvlB7DqgS1zmdn0VNEIdLyxL/LP0KnVl8smtAqfnD1AGT+d9ocFo+n5BM LAtndAokLZRGFRjirm9sU0oZvWVlLYv3Gutd/wxX0DVhvC+gs9B4k4Cpqo9zrsl2P9SQ j5BQ== ARC-Authentication-Results: i=1; mx.google.com; spf=pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) smtp.mailfrom=linux-kernel-owner@vger.kernel.org; dmarc=fail (p=NONE sp=NONE dis=NONE) header.from=arm.com Return-Path: Received: from out1.vger.email (out1.vger.email. [2620:137:e000::1:20]) by mx.google.com with ESMTP id j63-20020a638042000000b00530b70aac4asi5442330pgd.91.2023.05.15.02.10.22; Mon, 15 May 2023 02:10:34 -0700 (PDT) Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) client-ip=2620:137:e000::1:20; Authentication-Results: mx.google.com; spf=pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) smtp.mailfrom=linux-kernel-owner@vger.kernel.org; dmarc=fail (p=NONE sp=NONE dis=NONE) header.from=arm.com Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S238096AbjEOIsU (ORCPT + 99 others); Mon, 15 May 2023 04:48:20 -0400 Received: from lindbergh.monkeyblade.net ([23.128.96.19]:33120 "EHLO lindbergh.monkeyblade.net" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S237581AbjEOIsQ (ORCPT ); Mon, 15 May 2023 04:48:16 -0400 Received: from foss.arm.com (foss.arm.com [217.140.110.172]) by lindbergh.monkeyblade.net (Postfix) with ESMTP id 1D35BFF; Mon, 15 May 2023 01:48:12 -0700 (PDT) Received: from usa-sjc-imap-foss1.foss.arm.com (unknown [10.121.207.14]) by usa-sjc-mx-foss1.foss.arm.com (Postfix) with ESMTP id 490334B3; Mon, 15 May 2023 01:48:56 -0700 (PDT) Received: from [192.168.1.12] (unknown [172.31.20.19]) by usa-sjc-imap-foss1.foss.arm.com (Postfix) with ESMTPSA id 9FA443F67D; Mon, 15 May 2023 01:48:08 -0700 (PDT) Message-ID: <27520bc9-21f4-b4d6-3159-39542a93cfca@arm.com> Date: Mon, 15 May 2023 10:48:03 +0200 MIME-Version: 1.0 User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:102.0) Gecko/20100101 Thunderbird/102.10.0 From: Pierre Gondois Subject: Re: [PATCH 10/17] PM: EM: Add runtime update interface to modify EM power To: Lukasz Luba Cc: dietmar.eggemann@arm.com, rui.zhang@intel.com, amit.kucheria@verdurent.com, amit.kachhap@gmail.com, daniel.lezcano@linaro.org, viresh.kumar@linaro.org, len.brown@intel.com, pavel@ucw.cz, ionela.voinescu@arm.com, linux-pm@vger.kernel.org, rostedt@goodmis.org, linux-kernel@vger.kernel.org, rafael@kernel.org, mhiramat@kernel.org References: <20230314103357.26010-1-lukasz.luba@arm.com> <20230314103357.26010-11-lukasz.luba@arm.com> <94e7bbaa-3a25-6634-f5af-fff4932a44b9@arm.com> Content-Language: en-US In-Reply-To: <94e7bbaa-3a25-6634-f5af-fff4932a44b9@arm.com> Content-Type: text/plain; charset=UTF-8; format=flowed Content-Transfer-Encoding: 8bit X-Spam-Status: No, score=-8.0 required=5.0 tests=BAYES_00,NICE_REPLY_A, RCVD_IN_DNSWL_MED,SPF_HELO_NONE,SPF_NONE,T_SCC_BODY_TEXT_LINE autolearn=ham autolearn_force=no version=3.4.6 X-Spam-Checker-Version: SpamAssassin 3.4.6 (2021-04-09) on lindbergh.monkeyblade.net Precedence: bulk List-ID: X-Mailing-List: linux-kernel@vger.kernel.org Hi Lukasz, On 5/10/23 08:55, Lukasz Luba wrote: > Hi Pierre, > > On 4/11/23 16:40, Pierre Gondois wrote: >> Hello Lukasz, >> >> On 3/14/23 11:33, Lukasz Luba wrote: >>> Add an interface which allows to modify EM power data at runtime. >>> The new power information is populated by the provided callback, which >>> is called for each performance state. The CPU frequencies' efficiency is >>> re-calculated since that might be affected as well. The old EM memory >>> is going to be freed later using RCU mechanism. >>> >>> Signed-off-by: Lukasz Luba >>> --- >>>   include/linux/energy_model.h |   8 +++ >>>   kernel/power/energy_model.c  | 109 +++++++++++++++++++++++++++++++++++ >>>   2 files changed, 117 insertions(+) >>> >>> diff --git a/include/linux/energy_model.h b/include/linux/energy_model.h >>> index a616006a8130..e1772aa6c843 100644 >>> --- a/include/linux/energy_model.h >>> +++ b/include/linux/energy_model.h >>> @@ -202,6 +202,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_data_callback *cb, >>> +                  void *priv); >>>   int em_dev_register_perf_domain(struct device *dev, unsigned int >>> nr_states, >>>                   struct em_data_callback *cb, cpumask_t *span, >>>                   bool microwatts); >>> @@ -382,6 +384,12 @@ static inline int em_pd_nr_perf_states(struct >>> em_perf_domain *pd) >>>   { >>>       return 0; >>>   } >>> +static inline >>> +int em_dev_update_perf_domain(struct device *dev, struct >>> em_data_callback *cb, >>> +                  void *priv) >>> +{ >>> +    return -EINVAL; >>> +} >>>   #endif >>>   #endif >>> diff --git a/kernel/power/energy_model.c b/kernel/power/energy_model.c >>> index 87962b877376..e0e8fba3d02b 100644 >>> --- a/kernel/power/energy_model.c >>> +++ b/kernel/power/energy_model.c >> >> [snip] >> >>> @@ -531,9 +628,21 @@ void em_dev_unregister_perf_domain(struct device >>> *dev) >>>       tmp = pd->runtime_table; >>> +    /* >>> +     * Safely destroy runtime modifiable EM. By using the call >>> +     * synchronize_rcu() we make sure we don't progress till last user >>> +     * finished the RCU section and our update got applied. >>> +     */ >>>       rcu_assign_pointer(pd->runtime_table, NULL); >>>       synchronize_rcu(); >>> +    /* >>> +     * After the sync no updates will be in-flight, so free the old >>> +     * memory. >>> +     */ >>> +    if (tmp->state != pd->table) >>> +        kfree(tmp->state); >>> + >> >> NIT: I think that the call 'kfree(pd->default_table->state)' which is >> done in >> the patch: >>   PM: EM: Refactor struct em_perf_domain and add default_table >> should be done here, otherwise this bit of memory is not freed. > > In this patch 10/17 there is no 'default_table' field yet, so cannot > be freed in this patch's code. I copy/pasted the statement: 'kfree(pd->default_table->state)' but I meant that the dynamic/runtime 'state' structure is freed, but the 'state' structure belonging to the default table is not freed. I.e. there should be the following call: 'kfree(pd->table->state)' in this patch, which would be updated to 'kfree(pd->default_table->state)' in the patch: PM: EM: Refactor struct em_perf_domain and add default_table Ultimately, all the memory is freed with all the patches applied, so this is just a NIT about re-ordering (if this comment is indeed accurate). > > >>>       kfree(tmp); >>>       kfree(dev->em_pd->table); > > ^^^^ in this current code we have the clean-up. > Here we clean the dev->em_pd->table, which is our conceptual > 'default_table' in current code (before refactoring in 13/17) > > > In the patch 13/17 that you was referring to, there is also similar > but new cleaning process: > ------------------->8--------------------------- > - kfree(dev->em_pd->table); > + kfree(pd->default_table->state); > + kfree(pd->default_table); > ------------------8<---------------------------- > > So, it should be good. > > Regards, > Lukasz Regards, Pierre