[PATCH 1/2] timer: check failure of timer_cpu_notify() before calling init_timer_stats()

[PATCH 1/2] timer: check failure of timer_cpu_notify() before calling init_timer_stats()

[Viresh Kumar]

timer_cpu_notify() should return NOTIFY_OK and nothing else. Anything else would
trigger a BUG_ON(). Return value of this routine is already checked correctly
but is done after issuing a call to init_timer_stats(). The right order would be
to check the error case first and then call init_timer_stats(). Lets do it.

Signed-off-by: Viresh Kumar <viresh.kumar@linaro.org>
---
 kernel/timer.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/kernel/timer.c b/kernel/timer.c
index f64a98c..e8e7839 100644
--- a/kernel/timer.c
+++ b/kernel/timer.c
@@ -1674,9 +1674,9 @@ void __init init_timers(void)
 
 	err = timer_cpu_notify(&timers_nb, (unsigned long)CPU_UP_PREPARE,
 			       (void *)(long)smp_processor_id());
-	init_timer_stats();
-
 	BUG_ON(err != NOTIFY_OK);
+
+	init_timer_stats();
 	register_cpu_notifier(&timers_nb);
 	open_softirq(TIMER_SOFTIRQ, run_timer_softirq);
 }
-- 
1.7.12.rc2.18.g61b472e

[PATCH 2/2] timer: Make sure TIMER_FLAG_MASK bits are free in allocated base

[Viresh Kumar]

Currently we are using two lowest bit of base for internal purpose and so they
both should be zero in the allocated address. The code was doing the right thing
before this patch came in:

commit c5f66e99b7cb091e3d51ae8e8156892e8feb7fa3
Author: Tejun Heo <tj@kernel.org>
Date:   Wed Aug 8 11:10:28 2012 -0700

    timer: Implement TIMER_IRQSAFE

Tejun probably forgot to update this piece of code which checks if the lowest
'n' bits are zero or not and so wasn't updated according to the new flag. Lets
use TIMER_FLAG_MASK in the calculations here, so that this code wouldn't require
a change later on with another flag in.

Signed-off-by: Viresh Kumar <viresh.kumar@linaro.org>
---
 kernel/timer.c | 5 ++---
 1 file changed, 2 insertions(+), 3 deletions(-)

diff --git a/kernel/timer.c b/kernel/timer.c
index e8e7839..d52a8ff 100644
--- a/kernel/timer.c
+++ b/kernel/timer.c
@@ -1548,9 +1548,8 @@ static int init_timers_cpu(int cpu)
 			if (!base)
 				return -ENOMEM;
 
-			/* Make sure that tvec_base is 2 byte aligned */
-			if (tbase_get_deferrable(base)) {
-				WARN_ON(1);
+			/* Make sure tvec_base has TIMER_FLAG_MASK bits free */
+			if (WARN_ON(base != tbase_get_base(base))) {
 				kfree(base);
 				return -ENOMEM;
 			}
-- 
1.7.12.rc2.18.g61b472e

Re: [PATCH 2/2] timer: Make sure TIMER_FLAG_MASK bits are free in allocated base

[Thomas Gleixner]

On Fri, 28 Feb 2014, Viresh Kumar wrote:

> Currently we are using two lowest bit of base for internal purpose and so they
> both should be zero in the allocated address. The code was doing the right thing
> before this patch came in:
> 
> commit c5f66e99b7cb091e3d51ae8e8156892e8feb7fa3
> Author: Tejun Heo <tj@kernel.org>
> Date:   Wed Aug 8 11:10:28 2012 -0700
> 
>     timer: Implement TIMER_IRQSAFE
> 
> Tejun probably forgot to update this piece of code which checks if the lowest
> 'n' bits are zero or not and so wasn't updated according to the new flag. Lets
> use TIMER_FLAG_MASK in the calculations here, so that this code wouldn't require
> a change later on with another flag in.

Are you planning to introduce more flag horror? Don't go there. The
timer_list code is about to be rewritten completely and I'm not going
to add new features to the existing code base.

> Signed-off-by: Viresh Kumar <viresh.kumar@linaro.org>
> ---
>  kernel/timer.c | 5 ++---
>  1 file changed, 2 insertions(+), 3 deletions(-)
> 
> diff --git a/kernel/timer.c b/kernel/timer.c
> index e8e7839..d52a8ff 100644
> --- a/kernel/timer.c
> +++ b/kernel/timer.c
> @@ -1548,9 +1548,8 @@ static int init_timers_cpu(int cpu)
>  			if (!base)
>  				return -ENOMEM;
>  
> -			/* Make sure that tvec_base is 2 byte aligned */
> -			if (tbase_get_deferrable(base)) {
> -				WARN_ON(1);
> +			/* Make sure tvec_base has TIMER_FLAG_MASK bits free */
> +			if (WARN_ON(base != tbase_get_base(base))) {
>  				kfree(base);
>  				return -ENOMEM;
>  			}
> -- 
> 1.7.12.rc2.18.g61b472e
> 
> 

Re: [PATCH 2/2] timer: Make sure TIMER_FLAG_MASK bits are free in allocated base

[Viresh Kumar]

> On 28-Feb-2014, at 4:22 pm, Thomas Gleixner <tglx@linutronix.de> wrote:
> 
>> On Fri, 28 Feb 2014, Viresh Kumar wrote:
>> 
>> Currently we are using two lowest bit of base for internal purpose and so they
>> both should be zero in the allocated address. The code was doing the right thing
>> before this patch came in:
>> 
>> commit c5f66e99b7cb091e3d51ae8e8156892e8feb7fa3
>> Author: Tejun Heo <tj@kernel.org>
>> Date:   Wed Aug 8 11:10:28 2012 -0700
>> 
>>    timer: Implement TIMER_IRQSAFE
>> 
>> Tejun probably forgot to update this piece of code which checks if the lowest
>> 'n' bits are zero or not and so wasn't updated according to the new flag. Lets
>> use TIMER_FLAG_MASK in the calculations here, so that this code wouldn't require
>> a change later on with another flag in.
> 
> Are you planning to introduce more flag horror? Don't go there. The
> timer_list code is about to be rewritten completely and I'm not going
> to add new features to the existing code base.

Not at all. I was just trying to understand this framework and found this
Issue.

[tip:timers/core] timer: Check failure of timer_cpu_notify() before calling init_timer_stats()

[tip-bot for Viresh Kumar]

Commit-ID:  c24a4a369419c360c323865b91198878275c1481
Gitweb:     http://git.kernel.org/tip/c24a4a369419c360c323865b91198878275c1481
Author:     Viresh Kumar <viresh.kumar@linaro.org>
AuthorDate: Fri, 28 Feb 2014 14:15:21 +0530
Committer:  Thomas Gleixner <tglx@linutronix.de>
CommitDate: Tue, 4 Mar 2014 12:30:29 +0100

timer: Check failure of timer_cpu_notify() before calling init_timer_stats()

timer_cpu_notify() should return NOTIFY_OK and nothing else. Anything else would
trigger a BUG_ON(). Return value of this routine is already checked correctly
but is done after issuing a call to init_timer_stats(). The right order would be
to check the error case first and then call init_timer_stats(). Lets do it.

Signed-off-by: Viresh Kumar <viresh.kumar@linaro.org>
Cc: linaro-kernel@lists.linaro.org
Cc: fweisbec@gmail.com
Cc: tj@kernel.org
Cc: peterz@infradead.org
Link: http://lkml.kernel.org/r/c439f5b6bbc2047e1662f4d523350531425bcf9d.1393576981.git.viresh.kumar@linaro.org
Signed-off-by: Thomas Gleixner <tglx@linutronix.de>
---
 kernel/timer.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/kernel/timer.c b/kernel/timer.c
index a71bdfd..31824ef 100644
--- a/kernel/timer.c
+++ b/kernel/timer.c
@@ -1681,9 +1681,9 @@ void __init init_timers(void)
 
 	err = timer_cpu_notify(&timers_nb, (unsigned long)CPU_UP_PREPARE,
 			       (void *)(long)smp_processor_id());
-	init_timer_stats();
-
 	BUG_ON(err != NOTIFY_OK);
+
+	init_timer_stats();
 	register_cpu_notifier(&timers_nb);
 	open_softirq(TIMER_SOFTIRQ, run_timer_softirq);
 }

[tip:timers/core] timer: Make sure TIMER_FLAG_MASK bits are free in allocated base

[tip-bot for Viresh Kumar]

Commit-ID:  38edbb0b913d73713c23dcc742669f7e78b52aa7
Gitweb:     http://git.kernel.org/tip/38edbb0b913d73713c23dcc742669f7e78b52aa7
Author:     Viresh Kumar <viresh.kumar@linaro.org>
AuthorDate: Fri, 28 Feb 2014 14:15:22 +0530
Committer:  Thomas Gleixner <tglx@linutronix.de>
CommitDate: Tue, 4 Mar 2014 12:30:29 +0100

timer: Make sure TIMER_FLAG_MASK bits are free in allocated base

Currently we are using two lowest bit of base for internal purpose and
so they both should be zero in the allocated address. The code was
doing the right thing before this patch came in: commit c5f66e99b
(timer: Implement TIMER_IRQSAFE)

Tejun probably forgot to update this piece of code which checks if the
lowest 'n' bits are zero or not and so wasn't updated according to the
new flag. Lets use TIMER_FLAG_MASK in the calculations here.

[ tglx: Massaged changelog ]

Signed-off-by: Viresh Kumar <viresh.kumar@linaro.org>
Cc: linaro-kernel@lists.linaro.org
Cc: fweisbec@gmail.com
Cc: tj@kernel.org
Cc: peterz@infradead.org
Link: http://lkml.kernel.org/r/9144e10d7e854a0aa8a673332adec356d81a923c.1393576981.git.viresh.kumar@linaro.org
Signed-off-by: Thomas Gleixner <tglx@linutronix.de>
---
 kernel/timer.c | 5 ++---
 1 file changed, 2 insertions(+), 3 deletions(-)

diff --git a/kernel/timer.c b/kernel/timer.c
index 31824ef..949d74e 100644
--- a/kernel/timer.c
+++ b/kernel/timer.c
@@ -1555,9 +1555,8 @@ static int init_timers_cpu(int cpu)
 			if (!base)
 				return -ENOMEM;
 
-			/* Make sure that tvec_base is 2 byte aligned */
-			if (tbase_get_deferrable(base)) {
-				WARN_ON(1);
+			/* Make sure tvec_base has TIMER_FLAG_MASK bits free */
+			if (WARN_ON(base != tbase_get_base(base))) {
 				kfree(base);
 				return -ENOMEM;
 			}

Re: [PATCH 2/2] timer: Make sure TIMER_FLAG_MASK bits are free in allocated base

[Viresh Kumar]

Hi Thomas,

On 28 February 2014 18:52, Thomas Gleixner <tglx@linutronix.de> wrote:
> On Fri, 28 Feb 2014, Viresh Kumar wrote:
>
>> Currently we are using two lowest bit of base for internal purpose and so they
>> both should be zero in the allocated address. The code was doing the right thing
>> before this patch came in:
>>
>> commit c5f66e99b7cb091e3d51ae8e8156892e8feb7fa3
>> Author: Tejun Heo <tj@kernel.org>
>> Date:   Wed Aug 8 11:10:28 2012 -0700
>>
>>     timer: Implement TIMER_IRQSAFE
>>
>> Tejun probably forgot to update this piece of code which checks if the lowest
>> 'n' bits are zero or not and so wasn't updated according to the new flag. Lets
>> use TIMER_FLAG_MASK in the calculations here, so that this code wouldn't require
>> a change later on with another flag in.
>
> Are you planning to introduce more flag horror? Don't go there. The
> timer_list code is about to be rewritten completely and I'm not going
> to add new features to the existing code base.

Do you already have stuff prepared that can be shared on that? I am asking
because I am working on some CPU isolation stuff for Networking domain and
it looks like I need to add another of these flags :( .. I know its
just not acceptable
and so wanted your thoughts on how can I get things fixed.

Peter asked me to implement something like cpuset.quiesce to move away all
timers/workqueues/etc from a cpuset. It was proposed here:

https://lkml.org/lkml/2014/1/15/186

Now, I was looking to migrate away the timers first but I obviously
shouldn't migrate
the pinned timers. One way out to identify PINNED timers is to mark them PINNED
with the flag bits, which you wouldn't allow. Can you give some other idea with
which I can get this solved.

--
viresh

Re: [PATCH 2/2] timer: Make sure TIMER_FLAG_MASK bits are free in allocated base

[Viresh Kumar]

On 11 March 2014 15:56, Viresh Kumar <viresh.kumar@linaro.org> wrote:
> Hi Thomas,
>
> On 28 February 2014 18:52, Thomas Gleixner <tglx@linutronix.de> wrote:
>> On Fri, 28 Feb 2014, Viresh Kumar wrote:
>>
>>> Currently we are using two lowest bit of base for internal purpose and so they
>>> both should be zero in the allocated address. The code was doing the right thing
>>> before this patch came in:
>>>
>>> commit c5f66e99b7cb091e3d51ae8e8156892e8feb7fa3
>>> Author: Tejun Heo <tj@kernel.org>
>>> Date:   Wed Aug 8 11:10:28 2012 -0700
>>>
>>>     timer: Implement TIMER_IRQSAFE
>>>
>>> Tejun probably forgot to update this piece of code which checks if the lowest
>>> 'n' bits are zero or not and so wasn't updated according to the new flag. Lets
>>> use TIMER_FLAG_MASK in the calculations here, so that this code wouldn't require
>>> a change later on with another flag in.
>>
>> Are you planning to introduce more flag horror? Don't go there. The
>> timer_list code is about to be rewritten completely and I'm not going
>> to add new features to the existing code base.
>
> Do you already have stuff prepared that can be shared on that? I am asking
> because I am working on some CPU isolation stuff for Networking domain and
> it looks like I need to add another of these flags :( .. I know its
> just not acceptable
> and so wanted your thoughts on how can I get things fixed.
>
> Peter asked me to implement something like cpuset.quiesce to move away all
> timers/workqueues/etc from a cpuset. It was proposed here:
>
> https://lkml.org/lkml/2014/1/15/186
>
> Now, I was looking to migrate away the timers first but I obviously
> shouldn't migrate
> the pinned timers. One way out to identify PINNED timers is to mark them PINNED
> with the flag bits, which you wouldn't allow. Can you give some other idea with
> which I can get this solved.

Ping!!

Re: [PATCH 2/2] timer: Make sure TIMER_FLAG_MASK bits are free in allocated base

[Thomas Gleixner]

On Tue, 18 Mar 2014, Viresh Kumar wrote:
> On 11 March 2014 15:56, Viresh Kumar <viresh.kumar@linaro.org> wrote:
> >
> > Now, I was looking to migrate away the timers first but I
> > obviously shouldn't migrate the pinned timers. One way out to
> > identify PINNED timers is to mark them PINNED with the flag bits,
> > which you wouldn't allow. Can you give some other idea with which
> > I can get this solved.

Sigh, I really hoped to find some time to finish the rework, but then
you get to debug bugs, solve the problem and are rewarded with a full
inbox. Lather, rinse, repeat...

Go ahead and make the timer base aligned with more room for some bits
to tweak. The new stuff will have a flags field where stuff like
pinned is stored.

Thanks,

	tglx