[PATCH 1/2] Revert "tick: Prefer a lower rating device only if it's CPU local device"

[PATCH 1/2] Revert "tick: Prefer a lower rating device only if it's CPU local device"

[Sudeep Holla]

This reverts commit 1332a90558013ae4242e3dd7934bdcdeafb06c0d.

The original issue was not because of incorrect checking of cpumask for
both new and old tick device. It was incorrectly analysed was due to the
misunderstanding of the comment and misinterpretation of the return
value from tick_check_preferred. The main issue is with the clockevent
driver that sets the cpumask to cpu_all_mask instead of cpu_possible_mask.

Cc: Thomas Gleixner <tglx@linutronix.de>
Signed-off-by: Sudeep Holla <sudeep.holla@arm.com>
---
 kernel/time/tick-common.c | 3 +--
 1 file changed, 1 insertion(+), 2 deletions(-)

Hi Thomas,

As mentioned in the other thread, this needs to be reverted. Sorry for
the misunderstanding the original issue and producing wrong fix.

Regards,
Sudeep

diff --git a/kernel/time/tick-common.c b/kernel/time/tick-common.c
index b7005dd21ec1..14de3727b18e 100644
--- a/kernel/time/tick-common.c
+++ b/kernel/time/tick-common.c
@@ -277,8 +277,7 @@ static bool tick_check_preferred(struct clock_event_device *curdev,
 	 */
 	return !curdev ||
 		newdev->rating > curdev->rating ||
-	       (!cpumask_equal(curdev->cpumask, newdev->cpumask) &&
-	        !tick_check_percpu(curdev, newdev, smp_processor_id()));
+	       !cpumask_equal(curdev->cpumask, newdev->cpumask);
 }

 /*
--
2.7.4

[PATCH 2/2] clocksource: arm_arch_timer: set arch_mem_timer cpumask to cpu_possible_mask

[Sudeep Holla]

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").

Cc: Mark Rutland <mark.rutland@arm.com>
Cc: Marc Zyngier <marc.zyngier@arm.com>
Signed-off-by: Sudeep Holla <sudeep.holla@arm.com>
---
 drivers/clocksource/arm_arch_timer.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

Hi,

There are few more drivers that set cpu_all_mask, should they also be
changed ? It should be fine as long as it's not compared against
someother non per-CPU timer with shorter cpumask.

Regards,
Sudeep

diff --git a/drivers/clocksource/arm_arch_timer.c b/drivers/clocksource/arm_arch_timer.c
index 57cb2f00fc07..d8c7f5750cdb 100644
--- a/drivers/clocksource/arm_arch_timer.c
+++ b/drivers/clocksource/arm_arch_timer.c
@@ -735,7 +735,7 @@ static void __arch_timer_setup(unsigned type,
 		clk->features |= CLOCK_EVT_FEAT_DYNIRQ;
 		clk->name = "arch_mem_timer";
 		clk->rating = 400;
-		clk->cpumask = cpu_all_mask;
+		clk->cpumask = cpu_possible_mask;
 		if (arch_timer_mem_use_virtual) {
 			clk->set_state_shutdown = arch_timer_shutdown_virt_mem;
 			clk->set_state_oneshot_stopped = arch_timer_shutdown_virt_mem;
--
2.7.4

Re: [PATCH 2/2] clocksource: arm_arch_timer: set arch_mem_timer cpumask to cpu_possible_mask

[Thomas Gleixner]

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.

Thanks,

	tgkx

Re: [PATCH 2/2] clocksource: arm_arch_timer: set arch_mem_timer cpumask to cpu_possible_mask

[Sudeep Holla]

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 ?

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.

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;

-- 
Regards,
Sudeep

Re: [PATCH 2/2] clocksource: arm_arch_timer: set arch_mem_timer cpumask to cpu_possible_mask

[Thomas Gleixner]

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

> 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.

> 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.

Thanks,

	tglx

Re: [PATCH 2/2] clocksource: arm_arch_timer: set arch_mem_timer cpumask to cpu_possible_mask

[Sudeep Holla]

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

Re: [PATCH 2/2] clocksource: arm_arch_timer: set arch_mem_timer cpumask to cpu_possible_mask

[Thomas Gleixner]

On Tue, 10 Jul 2018, Sudeep Holla wrote:
> On 10/07/18 13:21, Thomas Gleixner wrote:
> >> 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.

Appreciated. I'm inclined to take it for 4.18 even. Please send it along
with the fixes for the above obvious failure spots.

Thanks,

	tglx

[tip:timers/urgent] clocksource: arm_arch_timer: Set arch_mem_timer cpumask to cpu_possible_mask

[tip-bot for Sudeep Holla]

Commit-ID:  5e18e412973d6bb1804de1d4d30a891c774b006e
Gitweb:     https://git.kernel.org/tip/5e18e412973d6bb1804de1d4d30a891c774b006e
Author:     Sudeep Holla <sudeep.holla@arm.com>
AuthorDate: Mon, 9 Jul 2018 16:45:36 +0100
Committer:  Thomas Gleixner <tglx@linutronix.de>
CommitDate: Tue, 10 Jul 2018 22:12:47 +0200

clocksource: arm_arch_timer: Set arch_mem_timer cpumask to cpu_possible_mask

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").

Signed-off-by: Sudeep Holla <sudeep.holla@arm.com>
Signed-off-by: Thomas Gleixner <tglx@linutronix.de>
Tested-by: Kevin Hilman <khilman@baylibre.com>
Tested-by: Martin Blumenstingl <martin.blumenstingl@googlemail.com>
Cc: linux-arm-kernel@lists.infradead.org
Cc: Marc Zyngier <marc.zyngier@arm.com>
Cc: Mark Rutland <mark.rutland@arm.com>
Link: https://lkml.kernel.org/r/1531151136-18297-2-git-send-email-sudeep.holla@arm.com

---
 drivers/clocksource/arm_arch_timer.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/clocksource/arm_arch_timer.c b/drivers/clocksource/arm_arch_timer.c
index 57cb2f00fc07..d8c7f5750cdb 100644
--- a/drivers/clocksource/arm_arch_timer.c
+++ b/drivers/clocksource/arm_arch_timer.c
@@ -735,7 +735,7 @@ static void __arch_timer_setup(unsigned type,
 		clk->features |= CLOCK_EVT_FEAT_DYNIRQ;
 		clk->name = "arch_mem_timer";
 		clk->rating = 400;
-		clk->cpumask = cpu_all_mask;
+		clk->cpumask = cpu_possible_mask;
 		if (arch_timer_mem_use_virtual) {
 			clk->set_state_shutdown = arch_timer_shutdown_virt_mem;
 			clk->set_state_oneshot_stopped = arch_timer_shutdown_virt_mem;

Re: [PATCH 1/2] Revert "tick: Prefer a lower rating device only if it's CPU local device"

[Kevin Hilman]

On Mon, Jul 9, 2018 at 8:45 AM Sudeep Holla <sudeep.holla@arm.com> wrote:
>
> This reverts commit 1332a90558013ae4242e3dd7934bdcdeafb06c0d.
>
> The original issue was not because of incorrect checking of cpumask for
> both new and old tick device. It was incorrectly analysed was due to the
> misunderstanding of the comment and misinterpretation of the return
> value from tick_check_preferred. The main issue is with the clockevent
> driver that sets the cpumask to cpu_all_mask instead of cpu_possible_mask.
>
> Cc: Thomas Gleixner <tglx@linutronix.de>
> Signed-off-by: Sudeep Holla <sudeep.holla@arm.com>

Tested-by: Kevin Hilman <khilman@baylibre.com>

And verified to fix a regression on the 32-bit ARM platform mesion8b-odroidc1.

Thanks,

Kevin


> ---
>  kernel/time/tick-common.c | 3 +--
>  1 file changed, 1 insertion(+), 2 deletions(-)
>
> Hi Thomas,
>
> As mentioned in the other thread, this needs to be reverted. Sorry for
> the misunderstanding the original issue and producing wrong fix.
>
> Regards,
> Sudeep
>
> diff --git a/kernel/time/tick-common.c b/kernel/time/tick-common.c
> index b7005dd21ec1..14de3727b18e 100644
> --- a/kernel/time/tick-common.c
> +++ b/kernel/time/tick-common.c
> @@ -277,8 +277,7 @@ static bool tick_check_preferred(struct clock_event_device *curdev,
>          */
>         return !curdev ||
>                 newdev->rating > curdev->rating ||
> -              (!cpumask_equal(curdev->cpumask, newdev->cpumask) &&
> -               !tick_check_percpu(curdev, newdev, smp_processor_id()));
> +              !cpumask_equal(curdev->cpumask, newdev->cpumask);
>  }
>
>  /*
> --
> 2.7.4
>

Re: [PATCH 1/2] Revert "tick: Prefer a lower rating device only if it's CPU local device"

[Martin Blumenstingl]

On Mon, Jul 9, 2018 at 8:24 PM Kevin Hilman <khilman@baylibre.com> wrote:
>
> On Mon, Jul 9, 2018 at 8:45 AM Sudeep Holla <sudeep.holla@arm.com> wrote:
> >
> > This reverts commit 1332a90558013ae4242e3dd7934bdcdeafb06c0d.
> >
> > The original issue was not because of incorrect checking of cpumask for
> > both new and old tick device. It was incorrectly analysed was due to the
> > misunderstanding of the comment and misinterpretation of the return
> > value from tick_check_preferred. The main issue is with the clockevent
> > driver that sets the cpumask to cpu_all_mask instead of cpu_possible_mask.
> >
> > Cc: Thomas Gleixner <tglx@linutronix.de>
> > Signed-off-by: Sudeep Holla <sudeep.holla@arm.com>
>
> Tested-by: Kevin Hilman <khilman@baylibre.com>
Tested-by: Martin Blumenstingl <martin.blumenstingl@googlemail.com>

> And verified to fix a regression on the 32-bit ARM platform mesion8b-odroidc1.
I also tested it on Meson8b as well as Meson8m2


Regards
Martin

[tip:timers/urgent] Revert "tick: Prefer a lower rating device only if it's CPU local device"

[tip-bot for Sudeep Holla]

Commit-ID:  5b5ccbc2b041f98f26b984e013d303b7f9e6fb8e
Gitweb:     https://git.kernel.org/tip/5b5ccbc2b041f98f26b984e013d303b7f9e6fb8e
Author:     Sudeep Holla <sudeep.holla@arm.com>
AuthorDate: Mon, 9 Jul 2018 16:45:35 +0100
Committer:  Thomas Gleixner <tglx@linutronix.de>
CommitDate: Tue, 10 Jul 2018 22:12:47 +0200

Revert "tick: Prefer a lower rating device only if it's CPU local device"

This reverts commit 1332a90558013ae4242e3dd7934bdcdeafb06c0d.

The original issue was not because of incorrect checking of cpumask for
both new and old tick device. It was incorrectly analysed was due to the
misunderstanding of the comment and misinterpretation of the return value
from tick_check_preferred. The main issue is with the clockevent driver
that sets the cpumask to cpu_all_mask instead of cpu_possible_mask.

Signed-off-by: Sudeep Holla <sudeep.holla@arm.com>
Signed-off-by: Thomas Gleixner <tglx@linutronix.de>
Tested-by: Kevin Hilman <khilman@baylibre.com>
Tested-by: Martin Blumenstingl <martin.blumenstingl@googlemail.com>
Cc: linux-arm-kernel@lists.infradead.org
Cc: Marc Zyngier <marc.zyngier@arm.com>
Link: https://lkml.kernel.org/r/1531151136-18297-1-git-send-email-sudeep.holla@arm.com

---
 kernel/time/tick-common.c | 3 +--
 1 file changed, 1 insertion(+), 2 deletions(-)

diff --git a/kernel/time/tick-common.c b/kernel/time/tick-common.c
index b7005dd21ec1..14de3727b18e 100644
--- a/kernel/time/tick-common.c
+++ b/kernel/time/tick-common.c
@@ -277,8 +277,7 @@ static bool tick_check_preferred(struct clock_event_device *curdev,
 	 */
 	return !curdev ||
 		newdev->rating > curdev->rating ||
-	       (!cpumask_equal(curdev->cpumask, newdev->cpumask) &&
-	        !tick_check_percpu(curdev, newdev, smp_processor_id()));
+	       !cpumask_equal(curdev->cpumask, newdev->cpumask);
 }
 
 /*