lkml.org 
[lkml]   [2018]   [Jul]   [10]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
/
SubjectRe: [PATCH 2/2] clocksource: arm_arch_timer: set arch_mem_timer cpumask to cpu_possible_mask
From
Date


On 10/07/18 13:21, Thomas Gleixner wrote:
> On Tue, 10 Jul 2018, Sudeep Holla wrote:
>
>>
>>
>> On 09/07/18 23:09, Thomas Gleixner wrote:
>>> On Mon, 9 Jul 2018, Sudeep Holla wrote:
>>>
>>>> Currently, arch_mem_timer cpumask is set to cpu_all_mask which should be
>>>> fine. However, cpu_possible_mask is more accurate and if there are other
>>>> clockevent source in the system which are set to cpu_possible_mask, then
>>>> having cpu_all_mask may result in issue.
>>>>
>>>> E.g. on a platform with arm,sp804 timer with rating 300 and
>>>> cpu_possible_mask and this arch_mem_timer timer with rating 400 and
>>>> cpu_all_mask, tick_check_preferred may choose both preferred as the
>>>> cpumasks are not equal though they must be.
>>>>
>>>> This issue was root caused incorrectly initially and a fix was merged as
>>>> commit 1332a9055801 ("tick: Prefer a lower rating device only if it's CPU
>>>> local device").
>>>
>>> To avoid that in the future we really should fix the decision logic to mask
>>> out the non possible CPUs from the supplied masks.
>>
>> Sure, I can do that. But do you want that as a fix for v4.18 ?
>
> No, that's for 4.19
>

Ah OK, I assume you are fine with this patch as fix for v4.18.
>> I think we may need this check at another place in tick_setup_device
>>
>> 213 /*
>> 214 * When the device is not per cpu, pin the interrupt to the
>> 215 * current cpu:
>> 216 */
>> 217 if (!cpumask_equal(newdev->cpumask, cpumask))
>> 218 irq_set_affinity(newdev->irq, cpumask);
>>
>> Does it make sense trim dev->cpumask when registering the clockevents
>> device itself instead of adding check at place where this cpumask
>> can be used ? So that any future user of those masks need not have to
>> take care of that.
>
> The problem is you cannot trim it because cpu_all_mask is global and const.
>

Indeed, again forgot it just a pointer to the global one, duh!

>> Also only few ARM clocksource drivers use cpu_all_mask which could be
>> result of copy-paste, we can even fix them too.
>>
>> arm_arch_timer.c: clk->cpumask = cpu_all_mask;
>> tegra20_timer.c: tegra_clockevent.cpumask = cpu_all_mask;
>> timer-atcpit100.c: .cpumask = cpu_all_mask,
>> timer-keystone.c: event_dev->cpumask = cpu_all_mask;
>> zevio-timer.c: timer->clkevt.cpumask = cpu_all_mask;
>
> Yes, that makes sense. What we could do is warn, when cpu_all_mask is set
> at registration time and replace the pointer with cpu_possible_mask.
>

I like this approach than having to bitwise and with cpu_possible_mask
at all the necessary place. I will cook up a patch.
--
Regards,
Sudeep

\
 
 \ /
  Last update: 2018-07-10 15:26    [W:0.114 / U:0.376 seconds]
©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site