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@xxxxxxxxxxx> пишет:
>>>
>>>> On 19. 7. 19. 오전 10:59, Dmitry Osipenko wrote:
>>>>> В Fri, 19 Jul 2019 10:36:30 +0900
>>>>> Chanwoo Choi <cw00.choi@xxxxxxxxxxx> пишет:
>>>>>
>>>>>> 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@xxxxxxxxx>
>>>>>>> ---
>>>>>>> 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?
[snip]
