Re: [PATCH] coresight: etm4x: lazily allocate memory for save_state

From: Suzuki K Poulose <suzuki.poulose@arm.com>
To: andrew.murray@arm.com
Cc: mathieu.poirier@linaro.org, alexander.shishkin@linux.intel.com,
	coresight@lists.linaro.org, leo.yan@linaro.org,
	Sudeep.Holla@arm.com, linux-arm-kernel@lists.infradead.org,
	mike.leach@linaro.org
Subject: Re: [PATCH] coresight: etm4x: lazily allocate memory for save_state
Date: Tue, 30 Jul 2019 11:15:34 +0100	[thread overview]
Message-ID: <e3b4f639-e7e5-874b-0b1d-085a5e45cd6c@arm.com> (raw)
In-Reply-To: <20190726112444.GA56241@e119886-lin.cambridge.arm.com>

On 26/07/2019 12:24, Andrew Murray wrote:
> On Tue, Jul 23, 2019 at 10:05:37AM +0100, Suzuki K Poulose wrote:
>>
>>
>> On 22/07/2019 21:32, Mathieu Poirier wrote:
>>> Hi Andrew,
>>>
>>> Sorry for the late reply - you patch got lost under the pile.
>>>
>>> On Fri, 12 Jul 2019 at 09:01, Andrew Murray <andrew.murray@arm.com> wrote:
>>>>
>>>> I had intended to lazily allocate memory for the save_state structure when
>>>> it is first used. Following is a patch that I will squash into "[PATCH v3 5/6]
>>>> coresight: etm4x: save/restore state across CPU low power states" on my
>>>> next respin. I thought I'd share it here to get some feedback along with
>>>> the rest of v3.
>>>>
>>>> Thanks,
>>>>
>>>> Andrew Murray
>>>>
>>>> ---
>>>>    drivers/hwtracing/coresight/coresight-etm4x.c | 14 +++++++++++---
>>>>    drivers/hwtracing/coresight/coresight-etm4x.h |  2 +-
>>>>    2 files changed, 12 insertions(+), 4 deletions(-)
>>>>
>>>> diff --git a/drivers/hwtracing/coresight/coresight-etm4x.c b/drivers/hwtracing/coresight/coresight-etm4x.c
>>>> index b0bd8158bf13..cd02372194bc 100644
>>>> --- a/drivers/hwtracing/coresight/coresight-etm4x.c
>>>> +++ b/drivers/hwtracing/coresight/coresight-etm4x.c
>>>> @@ -1112,6 +1112,13 @@ static int etm4_cpu_save(struct etmv4_drvdata *drvdata)
>>>>           struct etmv4_save_state *state;
>>>>           struct device *etm_dev = &drvdata->csdev->dev;
>>>>
>>>> +       if (!drvdata->save_state) {
>>>> +               drvdata->save_state = devm_kmalloc(etm_dev,
>>>> +                               sizeof(struct etmv4_save_state), GFP_KERNEL);
>>>
>>> GFP_KERNEL may sleep and will not work in the context where
>>> etm4_cpu_save() is called.
>>
>> Thats right and it is not worth making this GFP_ATOMIC either. We could simply
>> decide this at probe time or when the save_restore is modified dynamically via
>> callbacks.
> 
> I think it is simpler to change this to GFP_ATOMIC and leave it where it is.
> 
> As pm_save_enable can change at run time, we can't rely solely on allocating

Or we could make it static. You either need it on the system, irrespective of
what else happens or you don't need it at all. I agree that it helps with
testing.

> memory for this at probe time. We'd have to allocate memory in two places 1)
> a module_parm_cb callback for when the pm_save_enable parameter changes at
> run-time and 2) at probe time to find out the initial value of the
> pm_save_enable which may be set by kernel command line.
> 
> As the module_parm_cb callback is file-static we wouldn't know which drvdata
> to allocate the memory for. We could allocate it for any etmdrvdata member
> that isn't NULL - but this seems to add a lot of complexity - is this worth
> it to avoid a GFP_ATOMIC allocation? (If we fail the allocation we can return
> NOTIFY_BAD and stop the PM event.)

Ok, fair enough. If we are going for the GFP_ATOMIC allocation, please could we
release it once we have restored ? We don't want to be holding onto the ATOMIC
allocated memory forever.

Cheers
Suzuki

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel