linux-kernel - Re: [PATCH v4 20/24] PM / devfreq: tegra30: Optimize upper average watermark selection

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives
Hash Suite: Windows password security audit tool. GUI, reports in PDF.
[<prev] [next>] [<thread-prev] [day] [month] [year] [list]
Message-ID: <2c6a20e0-2e56-ea5a-76e6-bf17994fc7df@samsung.com>
Date:   Wed, 24 Jul 2019 20:19:09 +0900
From:   Chanwoo Choi <cw00.choi@...sung.com>
To:     Dmitry Osipenko <digetx@...il.com>
Cc:     Thierry Reding <thierry.reding@...il.com>,
        MyungJoo Ham <myungjoo.ham@...sung.com>,
        Kyungmin Park <kyungmin.park@...sung.com>,
        Jonathan Hunter <jonathanh@...dia.com>,
        Tomeu Vizoso <tomeu.vizoso@...labora.com>,
        linux-pm@...r.kernel.org, linux-tegra@...r.kernel.org,
        linux-kernel@...r.kernel.org
Subject: Re: [PATCH v4 20/24] PM / devfreq: tegra30: Optimize upper average
 watermark selection

On 19. 7. 24. 오후 8:17, Chanwoo Choi wrote:
> On 19. 7. 20. 오전 2:52, Dmitry Osipenko wrote:
>> 19.07.2019 9:11, Chanwoo Choi пишет:
>>> On 19. 7. 19. 오후 3:09, Chanwoo Choi wrote:
>>>> On 19. 7. 19. 오전 11:21, Dmitry Osipenko wrote:
>>>>> В Fri, 19 Jul 2019 11:06:05 +0900
>>>>> Chanwoo Choi <cw00.choi@...sung.com> пишет:
>>>>>
>>>>>> On 19. 7. 19. 오전 10:59, Dmitry Osipenko wrote:
>>>>>>> В Fri, 19 Jul 2019 10:36:30 +0900
>>>>>>> Chanwoo Choi <cw00.choi@...sung.com> пишет:
>>>>>>>   
>>>>>>>> On 19. 7. 8. 오전 7:32, Dmitry Osipenko wrote:  
>>>>>>>>> I noticed that CPU may be crossing the dependency threshold very
>>>>>>>>> frequently for some workloads and this results in a lot of
>>>>>>>>> interrupts which could be avoided if MCALL client is keeping
>>>>>>>>> actual EMC frequency at a higher rate.
>>>>>>>>>
>>>>>>>>> Signed-off-by: Dmitry Osipenko <digetx@...il.com>
>>>>>>>>> ---
>>>>>>>>>  drivers/devfreq/tegra30-devfreq.c | 23 ++++++++++++++++++-----
>>>>>>>>>  1 file changed, 18 insertions(+), 5 deletions(-)
>>>>>>>>>
>>>>>>>>> diff --git a/drivers/devfreq/tegra30-devfreq.c
>>>>>>>>> b/drivers/devfreq/tegra30-devfreq.c index
>>>>>>>>> c3cf87231d25..4d582809acb6 100644 ---
>>>>>>>>> a/drivers/devfreq/tegra30-devfreq.c +++
>>>>>>>>> b/drivers/devfreq/tegra30-devfreq.c @@ -314,7 +314,8 @@ static
>>>>>>>>> void tegra_actmon_get_lower_upper(struct tegra_devfreq *tegra, }
>>>>>>>>>  
>>>>>>>>>  static void tegra_devfreq_update_avg_wmark(struct tegra_devfreq
>>>>>>>>> *tegra,
>>>>>>>>> -					   struct
>>>>>>>>> tegra_devfreq_device *dev)
>>>>>>>>> +					   struct
>>>>>>>>> tegra_devfreq_device *dev,
>>>>>>>>> +					   unsigned long freq)
>>>>>>>>>  {
>>>>>>>>>  	unsigned long avg_threshold, lower, upper;
>>>>>>>>>  
>>>>>>>>> @@ -323,6 +324,15 @@ static void
>>>>>>>>> tegra_devfreq_update_avg_wmark(struct tegra_devfreq *tegra,
>>>>>>>>> avg_threshold = dev->config->avg_dependency_threshold;
>>>>>>>>> avg_threshold = avg_threshold * ACTMON_SAMPLING_PERIOD; 
>>>>>>>>> +	/*
>>>>>>>>> +	 * If cumulative EMC frequency selection is higher than
>>>>>>>>> the
>>>>>>>>> +	 * device's, then there is no need to set upper watermark
>>>>>>>>> to
>>>>>>>>> +	 * a lower value because it will result in unnecessary
>>>>>>>>> upper
>>>>>>>>> +	 * interrupts.
>>>>>>>>> +	 */
>>>>>>>>> +	if (freq * ACTMON_SAMPLING_PERIOD > upper)
>>>>>>>>> +		upper = freq * ACTMON_SAMPLING_PERIOD;    
>>>>>>>>
>>>>>>>> Also, 'upper value is used on the patch5. You can combine this code
>>>>>>>> to patch5 or if this patch depends on the cpu notifier, you can
>>>>>>>> combine it to the patch of adding cpu notifier without separate
>>>>>>>> patch.  
>>>>>>>
>>>>>>> Well okay, I'll try to squash some of the patches in the next
>>>>>>> revision. Usually I'm receiving comments in the other direction,
>>>>>>> asking to separate patches into smaller changes ;) So that's more a
>>>>>>> personal preference of each maintainer, I'd say.
>>>>>>>   
>>>>>>
>>>>>> Right. We have to make the patch with atomic attribute.
>>>>>> But, if there are patches which touch the same code
>>>>>> in the same patchset. We can squash or do refactorig
>>>>>> of this code.
>>>>>
>>>>> The main benefit of having smaller logical changes is that when there is
>>>>> a bug, it's easier to narrow down the offending change using bisection.
>>>>> And it's just easier to review smaller patches, of course.
>>>>
>>>> I agree that the patch should contain the atomic feature.
>>>> To remove the some communication confusion between us,
>>>> I don't mean that you have to merge patches to only one patch.
>>>
>>> If each patch has the atomic attribute, it have to be made as the separate patch.
>>> But, if some patches are included in the the following two case,
>>> can combine patches to one patch.
>>>
>>>>
>>>> It is important to remove the following two cases on the same patchset.
>>>>
>>>> 1. the front patch adds the code and then later patch remove the added code.
>>
>> Okay, I agree that this is applicable to patch #11.
>>
>>>> 2. the front patch changes the code and the later patch again modified
>>>>    the changed code of the front patch
>>
>> If patch A adds a new feature and then patch B adds another new feature
>> on top of A, do you consider each of these patches as atomic?
> 
> Yes, if patch A and patch A have the different role

Sorry for my mistake. Modify the sentence as following:

Yes, if patch A and patch B have the different role
for the same device driver, it is possible to make them
as the separate patches.


> 
> 
>>
>> [snip]
>>
>>
> 
> 


-- 
Best Regards,
Chanwoo Choi
Samsung Electronics