From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: X-Spam-Checker-Version: SpamAssassin 3.4.0 (2014-02-07) on aws-us-west-2-korg-lkml-1.web.codeaurora.org X-Spam-Level: X-Spam-Status: No, score=-5.3 required=3.0 tests=DKIMWL_WL_HIGH,DKIM_SIGNED, DKIM_VALID,HEADER_FROM_DIFFERENT_DOMAINS,MAILING_LIST_MULTI,SIGNED_OFF_BY, SPF_HELO_NONE,SPF_PASS,USER_AGENT_SANE_1 autolearn=unavailable autolearn_force=no version=3.4.0 Received: from mail.kernel.org (mail.kernel.org [198.145.29.99]) by smtp.lore.kernel.org (Postfix) with ESMTP id 31776C433E0 for ; Wed, 3 Jun 2020 16:13:03 +0000 (UTC) Received: from bombadil.infradead.org (bombadil.infradead.org [198.137.202.133]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by mail.kernel.org (Postfix) with ESMTPS id F3794206E6 for ; Wed, 3 Jun 2020 16:13:02 +0000 (UTC) Authentication-Results: mail.kernel.org; dkim=pass (2048-bit key) header.d=lists.infradead.org header.i=@lists.infradead.org header.b="ZZHYfoB7" DMARC-Filter: OpenDMARC Filter v1.3.2 mail.kernel.org F3794206E6 Authentication-Results: mail.kernel.org; dmarc=none (p=none dis=none) header.from=arm.com Authentication-Results: mail.kernel.org; spf=none smtp.mailfrom=linux-mediatek-bounces+linux-mediatek=archiver.kernel.org@lists.infradead.org DKIM-Signature: v=1; a=rsa-sha256; q=dns/txt; c=relaxed/relaxed; d=lists.infradead.org; s=bombadil.20170209; h=Sender:Content-Type: Content-Transfer-Encoding:Cc:List-Subscribe:List-Help:List-Post:List-Archive: List-Unsubscribe:List-Id:In-Reply-To:MIME-Version:Date:Message-ID:From: References:To:Subject:Reply-To:Content-ID:Content-Description:Resent-Date: Resent-From:Resent-Sender:Resent-To:Resent-Cc:Resent-Message-ID:List-Owner; bh=uv+XCuHaHSaB76xk+7z2yRr02AuSqlIL7yKOEPtr9JY=; b=ZZHYfoB78g7JumwkxjMMR7wv4 WWPuPOivpSJmJ0Z7/qIQ1Wc4nw/chewIuLJDk5U3MEUhkSpLWyl2WnILRIcz17tsMUKOpNKgDETXR tMtzRcNhFiaBHUnfOoBQ45/I9IlIX2LW3bunwp3MsIZTqdBmzEIgVIuD+ur1k/BD1BMWQCsNKYs4y LqoPM2lIulum7aa3JN6Klz4P0ojgpmR4aSf4U1Rze5TBEb5iPvqzbb5O5h0Y6sOUuFFFD/JNLDNvY oAD7/SmsxrgOEoOimhNB+kQg4gi/BGh7M9M6AKz64QUTVD5BbWYq1N8iPo51ZgShOKamKm7oBkWtz Mw4GecwKw==; Received: from localhost ([127.0.0.1] helo=bombadil.infradead.org) by bombadil.infradead.org with esmtp (Exim 4.92.3 #3 (Red Hat Linux)) id 1jgW0c-00075i-9G; Wed, 03 Jun 2020 16:12:54 +0000 Received: from foss.arm.com ([217.140.110.172]) by bombadil.infradead.org with esmtp (Exim 4.92.3 #3 (Red Hat Linux)) id 1jgW0S-0006xs-W2; Wed, 03 Jun 2020 16:12:46 +0000 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 AB8CA55D; Wed, 3 Jun 2020 09:12:43 -0700 (PDT) Received: from [10.37.12.87] (unknown [10.37.12.87]) by usa-sjc-imap-foss1.foss.arm.com (Postfix) with ESMTPSA id 05E993F52E; Wed, 3 Jun 2020 09:12:32 -0700 (PDT) Subject: Re: [PATCH v8 4/8] PM / EM: add support for other devices than CPUs in Energy Model To: "Rafael J. Wysocki" References: <20200527095854.21714-1-lukasz.luba@arm.com> <20200527095854.21714-5-lukasz.luba@arm.com> <7201e161-6952-6e28-4036-bd0f0353ec30@arm.com> From: Lukasz Luba Message-ID: Date: Wed, 3 Jun 2020 17:12:31 +0100 User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:60.0) Gecko/20100101 Thunderbird/60.9.0 MIME-Version: 1.0 In-Reply-To: Content-Language: en-US X-CRM114-Version: 20100106-BlameMichelson ( TRE 0.8.0 (BSD) ) MR-646709E3 X-CRM114-CacheID: sfid-20200603_091245_122750_F75727CE X-CRM114-Status: GOOD ( 29.30 ) X-BeenThere: linux-mediatek@lists.infradead.org X-Mailman-Version: 2.1.29 Precedence: list List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Cc: Nishanth Menon , Juri Lelli , Peter Zijlstra , Viresh Kumar , Liviu Dudau , dri-devel , Bjorn Andersson , Benjamin Segall , alyssa.rosenzweig@collabora.com, Fabio Estevam , Matthias Kaehlcke , Rob Herring , Amit Kucheria , Lorenzo Pieralisi , Vincent Guittot , Kevin Hilman , Andy Gross , Daniel Lezcano , steven.price@arm.com, Chanwoo Choi , Ingo Molnar , dl-linux-imx , "Zhang, Rui" , Mel Gorman , orjan.eide@arm.com, Daniel Vetter , Linux PM , linux-arm-msm , Sascha Hauer , Steven Rostedt , "moderated list:ARM/Mediatek SoC..." , Matthias Brugger , Linux OMAP Mailing List , Dietmar Eggemann , Linux ARM , David Airlie , Tomeu Vizoso , Quentin Perret , Stephen Boyd , Randy Dunlap , "Rafael J. Wysocki" , Linux Kernel Mailing List , Bartlomiej Zolnierkiewicz , Sascha Hauer , Sudeep Holla , patrick.bellasi@matbug.net, Shawn Guo Content-Transfer-Encoding: 7bit Content-Type: text/plain; charset="us-ascii"; Format="flowed" Sender: "Linux-mediatek" Errors-To: linux-mediatek-bounces+linux-mediatek=archiver.kernel.org@lists.infradead.org On 6/3/20 4:40 PM, Rafael J. Wysocki wrote: > On Wed, Jun 3, 2020 at 5:26 PM Lukasz Luba wrote: >> >> >> >> On 6/3/20 4:13 PM, Rafael J. Wysocki wrote: >>> On Tue, Jun 2, 2020 at 1:31 PM Lukasz Luba wrote: >>>> >>>> Hi Daniel, >>>> >>>> On 6/1/20 10:44 PM, Daniel Lezcano wrote: >>>>> On 27/05/2020 11:58, Lukasz Luba wrote: >>>>>> Add support for other devices than CPUs. The registration function >>>>>> does not require a valid cpumask pointer and is ready to handle new >>>>>> devices. Some of the internal structures has been reorganized in order to >>>>>> keep consistent view (like removing per_cpu pd pointers). >>>>>> >>>>>> Signed-off-by: Lukasz Luba >>>>>> --- >>>>> >>>>> [ ... ] >>>>> >>>>>> } >>>>>> EXPORT_SYMBOL_GPL(em_register_perf_domain); >>>>>> + >>>>>> +/** >>>>>> + * em_dev_unregister_perf_domain() - Unregister Energy Model (EM) for a device >>>>>> + * @dev : Device for which the EM is registered >>>>>> + * >>>>>> + * Try to unregister the EM for the specified device (but not a CPU). >>>>>> + */ >>>>>> +void em_dev_unregister_perf_domain(struct device *dev) >>>>>> +{ >>>>>> + if (IS_ERR_OR_NULL(dev) || !dev->em_pd) >>>>>> + return; >>>>>> + >>>>>> + if (_is_cpu_device(dev)) >>>>>> + return; >>>>>> + >>>>>> + mutex_lock(&em_pd_mutex); >>>>> >>>>> Is the mutex really needed? >>>> >>>> I just wanted to align this unregister code with register. Since there >>>> is debugfs dir lookup and the device's EM existence checks I thought it >>>> wouldn't harm just to lock for a while and make sure the registration >>>> path is not used. These two paths shouldn't affect each other, but with >>>> modules loading/unloading I wanted to play safe. >>>> >>>> I can change it maybe to just dmb() and the end of the function if it's >>>> a big performance problem in this unloading path. What do you think? >>> >>> I would rather leave the mutex locking as is. >>> >>> However, the question to ask is what exactly may go wrong without that >>> locking in place? Is there any specific race condition that you are >>> concerned about? >>> >> >> I tried to test this with module loading & unloading with panfrost >> driver and CPU hotplug (which should bail out quickly) and was OK. >> I don't see any particular race. I don't too much about the >> debugfs code, though. That's why I tried to protect from some >> scripts/services which try to re-load the driver. >> >> Apart from that, maybe just this dev->em = NULL to be populated to all >> CPUs, which mutex_unlock synchronizes for free here. > > If it may run concurrently with the registration for the same device, > the locking is necessary, but in that case the !dev->em_pd check needs > to go under the mutex too IMO, or you may end up leaking the pd if the > registration can run between that check and the point at which the > mutex is taken. They don't run concurrently for the same device and users of that EM are already gone. I just wanted to be sure that everything is cleaned and synced properly. Here is some example of the directories under /sys/kernel/debug/energy_model cpu0, cpu4, gpu, dsp, etc The only worry that I had was the debugfs dir name, which is a string from dev_name() and will be the same for the next registration if module is re-loaded. So the 'name' is reused and debugfs_create_dir() and debugfs_remove_recursive() uses this fsnotify, but they are operating under inode_lock/unlock() on the parent dir 'energy_model'. Then there is also this debugfs_lookup() which is slightly different. That's why I put a mutex to separate all registration and unregistration for all devices. It should work without the mutex in unregister path, but I think it does not harm to take it just in case and also have the CPU variable sync for free. > > Apart from this your kerneldoc comments might me improved IMO. > > First of all, you can use @dev inside of a kerneldoc (if @dev > represents an argument of the documented function) and that will > produce the right output automatically. OK > > Second, it is better to avoid saying things like "Try to unregister > ..." in kerneldoc comments (the "Try to" part is redundant). Simply > say "Unregister ..." instead. Good point, thanks, I will use "Unregister ..." then. > > Thanks! > Shell I send a 'resend patch' which changes these @dev and 'unregister' comments? Or wait for you to finish reviewing the other patches and send v9? _______________________________________________ Linux-mediatek mailing list Linux-mediatek@lists.infradead.org http://lists.infradead.org/mailman/listinfo/linux-mediatek From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: X-Spam-Checker-Version: SpamAssassin 3.4.0 (2014-02-07) on aws-us-west-2-korg-lkml-1.web.codeaurora.org X-Spam-Level: X-Spam-Status: No, score=-5.3 required=3.0 tests=DKIMWL_WL_HIGH,DKIM_SIGNED, DKIM_VALID,HEADER_FROM_DIFFERENT_DOMAINS,MAILING_LIST_MULTI,SIGNED_OFF_BY, SPF_HELO_NONE,SPF_PASS,USER_AGENT_SANE_1 autolearn=ham autolearn_force=no version=3.4.0 Received: from mail.kernel.org (mail.kernel.org [198.145.29.99]) by smtp.lore.kernel.org (Postfix) with ESMTP id 0E4A5C433DF for ; Wed, 3 Jun 2020 16:12:58 +0000 (UTC) Received: from bombadil.infradead.org (bombadil.infradead.org [198.137.202.133]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by mail.kernel.org (Postfix) with ESMTPS id D143E205CB for ; Wed, 3 Jun 2020 16:12:57 +0000 (UTC) Authentication-Results: mail.kernel.org; dkim=pass (2048-bit key) header.d=lists.infradead.org header.i=@lists.infradead.org header.b="kYljDdM7" DMARC-Filter: OpenDMARC Filter v1.3.2 mail.kernel.org D143E205CB Authentication-Results: mail.kernel.org; dmarc=none (p=none dis=none) header.from=arm.com Authentication-Results: mail.kernel.org; spf=none smtp.mailfrom=linux-arm-kernel-bounces+infradead-linux-arm-kernel=archiver.kernel.org@lists.infradead.org DKIM-Signature: v=1; a=rsa-sha256; q=dns/txt; c=relaxed/relaxed; d=lists.infradead.org; s=bombadil.20170209; h=Sender:Content-Type: Content-Transfer-Encoding:Cc:List-Subscribe:List-Help:List-Post:List-Archive: List-Unsubscribe:List-Id:In-Reply-To:MIME-Version:Date:Message-ID:From: References:To:Subject:Reply-To:Content-ID:Content-Description:Resent-Date: Resent-From:Resent-Sender:Resent-To:Resent-Cc:Resent-Message-ID:List-Owner; bh=8sAHlJveVLK4rtlYxL6f6kYRpfS3V1s3g59u8xLm33k=; b=kYljDdM7wxtqwBFsS4yAthV9g q+keS/D4jBbIYBWkdqowdFz3TnoB+rbnNF70O1LDSR1FbXYTuIlJkxuUhTnL4Tc6g1EPI9Pxuz0se Y3qH98zUMSQaDiU8vp3Mex2w0hBMGlfmfhYx6oksbkKB0gtdqnq6Ub0SfXGd0MVWVkv914Xbj+fgo vUROtdmsZGy8l0jWq4O5bywUi5xODHBP8FSHLyLrXFoMcjdpApEfth0co+Mhc8JD3cOSWnhOD/4tW lW0wD+vDJTalA0imxlvOUhaAEfOVpgu3NjUxJq6/Vmhzy4OukvfOMqItqZN7oJ1EK/i4yaoqRRrSh zODWXlu+w==; Received: from localhost ([127.0.0.1] helo=bombadil.infradead.org) by bombadil.infradead.org with esmtp (Exim 4.92.3 #3 (Red Hat Linux)) id 1jgW0V-0006yI-Fb; Wed, 03 Jun 2020 16:12:47 +0000 Received: from foss.arm.com ([217.140.110.172]) by bombadil.infradead.org with esmtp (Exim 4.92.3 #3 (Red Hat Linux)) id 1jgW0S-0006xs-W2; Wed, 03 Jun 2020 16:12:46 +0000 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 AB8CA55D; Wed, 3 Jun 2020 09:12:43 -0700 (PDT) Received: from [10.37.12.87] (unknown [10.37.12.87]) by usa-sjc-imap-foss1.foss.arm.com (Postfix) with ESMTPSA id 05E993F52E; Wed, 3 Jun 2020 09:12:32 -0700 (PDT) Subject: Re: [PATCH v8 4/8] PM / EM: add support for other devices than CPUs in Energy Model To: "Rafael J. Wysocki" References: <20200527095854.21714-1-lukasz.luba@arm.com> <20200527095854.21714-5-lukasz.luba@arm.com> <7201e161-6952-6e28-4036-bd0f0353ec30@arm.com> From: Lukasz Luba Message-ID: Date: Wed, 3 Jun 2020 17:12:31 +0100 User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:60.0) Gecko/20100101 Thunderbird/60.9.0 MIME-Version: 1.0 In-Reply-To: Content-Language: en-US X-CRM114-Version: 20100106-BlameMichelson ( TRE 0.8.0 (BSD) ) MR-646709E3 X-CRM114-CacheID: sfid-20200603_091245_122750_F75727CE X-CRM114-Status: GOOD ( 29.30 ) X-BeenThere: linux-arm-kernel@lists.infradead.org X-Mailman-Version: 2.1.29 Precedence: list List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Cc: Nishanth Menon , Juri Lelli , Peter Zijlstra , Viresh Kumar , Liviu Dudau , dri-devel , Bjorn Andersson , Benjamin Segall , alyssa.rosenzweig@collabora.com, Fabio Estevam , Matthias Kaehlcke , Rob Herring , Amit Kucheria , Lorenzo Pieralisi , Kevin Hilman , Andy Gross , Daniel Lezcano , steven.price@arm.com, Chanwoo Choi , Ingo Molnar , dl-linux-imx , "Zhang, Rui" , Mel Gorman , orjan.eide@arm.com, Daniel Vetter , Linux PM , linux-arm-msm , Sascha Hauer , Steven Rostedt , "moderated list:ARM/Mediatek SoC..." , Matthias Brugger , Linux OMAP Mailing List , Dietmar Eggemann , Linux ARM , David Airlie , Tomeu Vizoso , Quentin Perret , Stephen Boyd , Randy Dunlap , "Rafael J. Wysocki" , Linux Kernel Mailing List , Bartlomiej Zolnierkiewicz , Sascha Hauer , Sudeep Holla , patrick.bellasi@matbug.net, Shawn Guo Content-Transfer-Encoding: 7bit Content-Type: text/plain; charset="us-ascii"; Format="flowed" Sender: "linux-arm-kernel" Errors-To: linux-arm-kernel-bounces+infradead-linux-arm-kernel=archiver.kernel.org@lists.infradead.org On 6/3/20 4:40 PM, Rafael J. Wysocki wrote: > On Wed, Jun 3, 2020 at 5:26 PM Lukasz Luba wrote: >> >> >> >> On 6/3/20 4:13 PM, Rafael J. Wysocki wrote: >>> On Tue, Jun 2, 2020 at 1:31 PM Lukasz Luba wrote: >>>> >>>> Hi Daniel, >>>> >>>> On 6/1/20 10:44 PM, Daniel Lezcano wrote: >>>>> On 27/05/2020 11:58, Lukasz Luba wrote: >>>>>> Add support for other devices than CPUs. The registration function >>>>>> does not require a valid cpumask pointer and is ready to handle new >>>>>> devices. Some of the internal structures has been reorganized in order to >>>>>> keep consistent view (like removing per_cpu pd pointers). >>>>>> >>>>>> Signed-off-by: Lukasz Luba >>>>>> --- >>>>> >>>>> [ ... ] >>>>> >>>>>> } >>>>>> EXPORT_SYMBOL_GPL(em_register_perf_domain); >>>>>> + >>>>>> +/** >>>>>> + * em_dev_unregister_perf_domain() - Unregister Energy Model (EM) for a device >>>>>> + * @dev : Device for which the EM is registered >>>>>> + * >>>>>> + * Try to unregister the EM for the specified device (but not a CPU). >>>>>> + */ >>>>>> +void em_dev_unregister_perf_domain(struct device *dev) >>>>>> +{ >>>>>> + if (IS_ERR_OR_NULL(dev) || !dev->em_pd) >>>>>> + return; >>>>>> + >>>>>> + if (_is_cpu_device(dev)) >>>>>> + return; >>>>>> + >>>>>> + mutex_lock(&em_pd_mutex); >>>>> >>>>> Is the mutex really needed? >>>> >>>> I just wanted to align this unregister code with register. Since there >>>> is debugfs dir lookup and the device's EM existence checks I thought it >>>> wouldn't harm just to lock for a while and make sure the registration >>>> path is not used. These two paths shouldn't affect each other, but with >>>> modules loading/unloading I wanted to play safe. >>>> >>>> I can change it maybe to just dmb() and the end of the function if it's >>>> a big performance problem in this unloading path. What do you think? >>> >>> I would rather leave the mutex locking as is. >>> >>> However, the question to ask is what exactly may go wrong without that >>> locking in place? Is there any specific race condition that you are >>> concerned about? >>> >> >> I tried to test this with module loading & unloading with panfrost >> driver and CPU hotplug (which should bail out quickly) and was OK. >> I don't see any particular race. I don't too much about the >> debugfs code, though. That's why I tried to protect from some >> scripts/services which try to re-load the driver. >> >> Apart from that, maybe just this dev->em = NULL to be populated to all >> CPUs, which mutex_unlock synchronizes for free here. > > If it may run concurrently with the registration for the same device, > the locking is necessary, but in that case the !dev->em_pd check needs > to go under the mutex too IMO, or you may end up leaking the pd if the > registration can run between that check and the point at which the > mutex is taken. They don't run concurrently for the same device and users of that EM are already gone. I just wanted to be sure that everything is cleaned and synced properly. Here is some example of the directories under /sys/kernel/debug/energy_model cpu0, cpu4, gpu, dsp, etc The only worry that I had was the debugfs dir name, which is a string from dev_name() and will be the same for the next registration if module is re-loaded. So the 'name' is reused and debugfs_create_dir() and debugfs_remove_recursive() uses this fsnotify, but they are operating under inode_lock/unlock() on the parent dir 'energy_model'. Then there is also this debugfs_lookup() which is slightly different. That's why I put a mutex to separate all registration and unregistration for all devices. It should work without the mutex in unregister path, but I think it does not harm to take it just in case and also have the CPU variable sync for free. > > Apart from this your kerneldoc comments might me improved IMO. > > First of all, you can use @dev inside of a kerneldoc (if @dev > represents an argument of the documented function) and that will > produce the right output automatically. OK > > Second, it is better to avoid saying things like "Try to unregister > ..." in kerneldoc comments (the "Try to" part is redundant). Simply > say "Unregister ..." instead. Good point, thanks, I will use "Unregister ..." then. > > Thanks! > Shell I send a 'resend patch' which changes these @dev and 'unregister' comments? Or wait for you to finish reviewing the other patches and send v9? _______________________________________________ linux-arm-kernel mailing list linux-arm-kernel@lists.infradead.org http://lists.infradead.org/mailman/listinfo/linux-arm-kernel From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: X-Spam-Checker-Version: SpamAssassin 3.4.0 (2014-02-07) on aws-us-west-2-korg-lkml-1.web.codeaurora.org X-Spam-Level: X-Spam-Status: No, score=-5.2 required=3.0 tests=HEADER_FROM_DIFFERENT_DOMAINS, MAILING_LIST_MULTI,SIGNED_OFF_BY,SPF_HELO_NONE,SPF_PASS,URIBL_BLOCKED, USER_AGENT_SANE_1 autolearn=unavailable autolearn_force=no version=3.4.0 Received: from mail.kernel.org (mail.kernel.org [198.145.29.99]) by smtp.lore.kernel.org (Postfix) with ESMTP id 86C60C433E0 for ; Thu, 4 Jun 2020 07:12:21 +0000 (UTC) Received: from gabe.freedesktop.org (gabe.freedesktop.org [131.252.210.177]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by mail.kernel.org (Postfix) with ESMTPS id 62C5B206DC for ; Thu, 4 Jun 2020 07:12:21 +0000 (UTC) DMARC-Filter: OpenDMARC Filter v1.3.2 mail.kernel.org 62C5B206DC Authentication-Results: mail.kernel.org; dmarc=none (p=none dis=none) header.from=arm.com Authentication-Results: mail.kernel.org; spf=none smtp.mailfrom=dri-devel-bounces@lists.freedesktop.org Received: from gabe.freedesktop.org (localhost [127.0.0.1]) by gabe.freedesktop.org (Postfix) with ESMTP id 77CF26E27C; Thu, 4 Jun 2020 07:11:54 +0000 (UTC) Received: from foss.arm.com (foss.arm.com [217.140.110.172]) by gabe.freedesktop.org (Postfix) with ESMTP id 3D4D189B55 for ; Wed, 3 Jun 2020 16:12:44 +0000 (UTC) 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 AB8CA55D; Wed, 3 Jun 2020 09:12:43 -0700 (PDT) Received: from [10.37.12.87] (unknown [10.37.12.87]) by usa-sjc-imap-foss1.foss.arm.com (Postfix) with ESMTPSA id 05E993F52E; Wed, 3 Jun 2020 09:12:32 -0700 (PDT) Subject: Re: [PATCH v8 4/8] PM / EM: add support for other devices than CPUs in Energy Model To: "Rafael J. Wysocki" References: <20200527095854.21714-1-lukasz.luba@arm.com> <20200527095854.21714-5-lukasz.luba@arm.com> <7201e161-6952-6e28-4036-bd0f0353ec30@arm.com> From: Lukasz Luba Message-ID: Date: Wed, 3 Jun 2020 17:12:31 +0100 User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:60.0) Gecko/20100101 Thunderbird/60.9.0 MIME-Version: 1.0 In-Reply-To: Content-Language: en-US X-Mailman-Approved-At: Thu, 04 Jun 2020 07:11:42 +0000 X-BeenThere: dri-devel@lists.freedesktop.org X-Mailman-Version: 2.1.29 Precedence: list List-Id: Direct Rendering Infrastructure - Development List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Cc: Nishanth Menon , Juri Lelli , Peter Zijlstra , Viresh Kumar , Liviu Dudau , dri-devel , Bjorn Andersson , Benjamin Segall , alyssa.rosenzweig@collabora.com, Matthias Kaehlcke , Amit Kucheria , Lorenzo Pieralisi , Vincent Guittot , Kevin Hilman , Andy Gross , Daniel Lezcano , steven.price@arm.com, Chanwoo Choi , Ingo Molnar , dl-linux-imx , "Zhang, Rui" , Mel Gorman , orjan.eide@arm.com, Linux PM , linux-arm-msm , Sascha Hauer , Steven Rostedt , "moderated list:ARM/Mediatek SoC..." , Matthias Brugger , Linux OMAP Mailing List , Dietmar Eggemann , Linux ARM , David Airlie , Tomeu Vizoso , Quentin Perret , Stephen Boyd , Randy Dunlap , "Rafael J. Wysocki" , Linux Kernel Mailing List , Bartlomiej Zolnierkiewicz , Sascha Hauer , Sudeep Holla , patrick.bellasi@matbug.net, Shawn Guo Content-Transfer-Encoding: 7bit Content-Type: text/plain; charset="us-ascii"; Format="flowed" Errors-To: dri-devel-bounces@lists.freedesktop.org Sender: "dri-devel" On 6/3/20 4:40 PM, Rafael J. Wysocki wrote: > On Wed, Jun 3, 2020 at 5:26 PM Lukasz Luba wrote: >> >> >> >> On 6/3/20 4:13 PM, Rafael J. Wysocki wrote: >>> On Tue, Jun 2, 2020 at 1:31 PM Lukasz Luba wrote: >>>> >>>> Hi Daniel, >>>> >>>> On 6/1/20 10:44 PM, Daniel Lezcano wrote: >>>>> On 27/05/2020 11:58, Lukasz Luba wrote: >>>>>> Add support for other devices than CPUs. The registration function >>>>>> does not require a valid cpumask pointer and is ready to handle new >>>>>> devices. Some of the internal structures has been reorganized in order to >>>>>> keep consistent view (like removing per_cpu pd pointers). >>>>>> >>>>>> Signed-off-by: Lukasz Luba >>>>>> --- >>>>> >>>>> [ ... ] >>>>> >>>>>> } >>>>>> EXPORT_SYMBOL_GPL(em_register_perf_domain); >>>>>> + >>>>>> +/** >>>>>> + * em_dev_unregister_perf_domain() - Unregister Energy Model (EM) for a device >>>>>> + * @dev : Device for which the EM is registered >>>>>> + * >>>>>> + * Try to unregister the EM for the specified device (but not a CPU). >>>>>> + */ >>>>>> +void em_dev_unregister_perf_domain(struct device *dev) >>>>>> +{ >>>>>> + if (IS_ERR_OR_NULL(dev) || !dev->em_pd) >>>>>> + return; >>>>>> + >>>>>> + if (_is_cpu_device(dev)) >>>>>> + return; >>>>>> + >>>>>> + mutex_lock(&em_pd_mutex); >>>>> >>>>> Is the mutex really needed? >>>> >>>> I just wanted to align this unregister code with register. Since there >>>> is debugfs dir lookup and the device's EM existence checks I thought it >>>> wouldn't harm just to lock for a while and make sure the registration >>>> path is not used. These two paths shouldn't affect each other, but with >>>> modules loading/unloading I wanted to play safe. >>>> >>>> I can change it maybe to just dmb() and the end of the function if it's >>>> a big performance problem in this unloading path. What do you think? >>> >>> I would rather leave the mutex locking as is. >>> >>> However, the question to ask is what exactly may go wrong without that >>> locking in place? Is there any specific race condition that you are >>> concerned about? >>> >> >> I tried to test this with module loading & unloading with panfrost >> driver and CPU hotplug (which should bail out quickly) and was OK. >> I don't see any particular race. I don't too much about the >> debugfs code, though. That's why I tried to protect from some >> scripts/services which try to re-load the driver. >> >> Apart from that, maybe just this dev->em = NULL to be populated to all >> CPUs, which mutex_unlock synchronizes for free here. > > If it may run concurrently with the registration for the same device, > the locking is necessary, but in that case the !dev->em_pd check needs > to go under the mutex too IMO, or you may end up leaking the pd if the > registration can run between that check and the point at which the > mutex is taken. They don't run concurrently for the same device and users of that EM are already gone. I just wanted to be sure that everything is cleaned and synced properly. Here is some example of the directories under /sys/kernel/debug/energy_model cpu0, cpu4, gpu, dsp, etc The only worry that I had was the debugfs dir name, which is a string from dev_name() and will be the same for the next registration if module is re-loaded. So the 'name' is reused and debugfs_create_dir() and debugfs_remove_recursive() uses this fsnotify, but they are operating under inode_lock/unlock() on the parent dir 'energy_model'. Then there is also this debugfs_lookup() which is slightly different. That's why I put a mutex to separate all registration and unregistration for all devices. It should work without the mutex in unregister path, but I think it does not harm to take it just in case and also have the CPU variable sync for free. > > Apart from this your kerneldoc comments might me improved IMO. > > First of all, you can use @dev inside of a kerneldoc (if @dev > represents an argument of the documented function) and that will > produce the right output automatically. OK > > Second, it is better to avoid saying things like "Try to unregister > ..." in kerneldoc comments (the "Try to" part is redundant). Simply > say "Unregister ..." instead. Good point, thanks, I will use "Unregister ..." then. > > Thanks! > Shell I send a 'resend patch' which changes these @dev and 'unregister' comments? Or wait for you to finish reviewing the other patches and send v9? _______________________________________________ dri-devel mailing list dri-devel@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/dri-devel