[PATCH 1/1] MM: slqb, fix per_cpu access

[PATCH 1/1] MM: slqb, fix per_cpu access

[Jiri Slaby]

We cannot use the same local variable name as the declared per_cpu
variable since commit "percpu: remove per_cpu__ prefix."

Otherwise we would see crashes like:
general protection fault: 0000 [#1] SMP
last sysfs file:
CPU 1
Modules linked in:
Pid: 1, comm: swapper Tainted: G        W  2.6.32-rc5-mm1_64 #860
RIP: 0010:[<ffffffff8142ff94>]  [<ffffffff8142ff94>] start_cpu_timer+0x2b/0x87
...

Signed-off-by: Jiri Slaby <jirislaby@gmail.com>
Cc: Nick Piggin <npiggin@suse.de>
Cc: Tejun Heo <tj@kernel.org>
Cc: Rusty Russell <rusty@rustcorp.com.au>
Cc: Christoph Lameter <cl@linux-foundation.org>
---
 mm/slqb.c |    8 ++++----
 1 files changed, 4 insertions(+), 4 deletions(-)

diff --git a/mm/slqb.c b/mm/slqb.c
index e745d9a..27f5025 100644
--- a/mm/slqb.c
+++ b/mm/slqb.c
@@ -2770,16 +2770,16 @@ static DEFINE_PER_CPU(struct delayed_work, cache_trim_work);
 
 static void __cpuinit start_cpu_timer(int cpu)
 {
-	struct delayed_work *cache_trim_work = &per_cpu(cache_trim_work, cpu);
+	struct delayed_work *_cache_trim_work = &per_cpu(cache_trim_work, cpu);
 
 	/*
 	 * When this gets called from do_initcalls via cpucache_init(),
 	 * init_workqueues() has already run, so keventd will be setup
 	 * at that time.
 	 */
-	if (keventd_up() && cache_trim_work->work.func == NULL) {
-		INIT_DELAYED_WORK(cache_trim_work, cache_trim_worker);
-		schedule_delayed_work_on(cpu, cache_trim_work,
+	if (keventd_up() && _cache_trim_work->work.func == NULL) {
+		INIT_DELAYED_WORK(_cache_trim_work, cache_trim_worker);
+		schedule_delayed_work_on(cpu, _cache_trim_work,
 					__round_jiffies_relative(HZ, cpu));
 	}
 }
-- 
1.6.4.2

--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to majordomo@kvack.org.  For more info on Linux MM,
see: http://www.linux-mm.org/ .
Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>

Re: [PATCH 1/1] MM: slqb, fix per_cpu access

[Tejun Heo]

Hello,

Jiri Slaby wrote:
> @@ -2770,16 +2770,16 @@ static DEFINE_PER_CPU(struct delayed_work, cache_trim_work);

How about renaming cache_trim_work to slqb_cache_trim_work?  Another
percpu name requirement is global uniqueness (for s390 and alpha
support), so prefixing perpcu variables with subsystem name usually
resolves situations like this better.

Thanks.

-- 
tejun

--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to majordomo@kvack.org.  For more info on Linux MM,
see: http://www.linux-mm.org/ .
Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>

[PATCH 1/1] MM: slqb, fix per_cpu access

[Jiri Slaby]

We cannot use the same local variable name as the declared per_cpu
variable since commit "percpu: remove per_cpu__ prefix."

Otherwise we would see crashes like:
general protection fault: 0000 [#1] SMP
last sysfs file:
CPU 1
Modules linked in:
Pid: 1, comm: swapper Tainted: G        W  2.6.32-rc5-mm1_64 #860
RIP: 0010:[<ffffffff8142ff94>]  [<ffffffff8142ff94>] start_cpu_timer+0x2b/0x87
...

Use slqb_ prefix for the global variable so that we don't collide
even with the rest of the kernel (s390 and alpha need this).

Signed-off-by: Jiri Slaby <jirislaby@gmail.com>
Cc: Nick Piggin <npiggin@suse.de>
Cc: Tejun Heo <tj@kernel.org>
Cc: Rusty Russell <rusty@rustcorp.com.au>
Cc: Christoph Lameter <cl@linux-foundation.org>
---
 mm/slqb.c |   10 ++++++----
 1 files changed, 6 insertions(+), 4 deletions(-)

diff --git a/mm/slqb.c b/mm/slqb.c
index e745d9a..e4bb53f 100644
--- a/mm/slqb.c
+++ b/mm/slqb.c
@@ -2766,11 +2766,12 @@ out:
 	schedule_delayed_work(work, round_jiffies_relative(3*HZ));
 }
 
-static DEFINE_PER_CPU(struct delayed_work, cache_trim_work);
+static DEFINE_PER_CPU(struct delayed_work, slqb_cache_trim_work);
 
 static void __cpuinit start_cpu_timer(int cpu)
 {
-	struct delayed_work *cache_trim_work = &per_cpu(cache_trim_work, cpu);
+	struct delayed_work *cache_trim_work = &per_cpu(slqb_cache_trim_work,
+			cpu);
 
 	/*
 	 * When this gets called from do_initcalls via cpucache_init(),
@@ -3136,8 +3137,9 @@ static int __cpuinit slab_cpuup_callback(struct notifier_block *nfb,
 
 	case CPU_DOWN_PREPARE:
 	case CPU_DOWN_PREPARE_FROZEN:
-		cancel_rearming_delayed_work(&per_cpu(cache_trim_work, cpu));
-		per_cpu(cache_trim_work, cpu).work.func = NULL;
+		cancel_rearming_delayed_work(&per_cpu(slqb_cache_trim_work,
+					cpu));
+		per_cpu(slqb_cache_trim_work, cpu).work.func = NULL;
 		break;
 
 	case CPU_UP_CANCELED:
-- 
1.6.4.2

--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to majordomo@kvack.org.  For more info on Linux MM,
see: http://www.linux-mm.org/ .
Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>

Re: [PATCH 1/1] MM: slqb, fix per_cpu access

[Dave Young]

On Mon, Nov 2, 2009 at 4:49 PM, Jiri Slaby <jirislaby@gmail.com> wrote:
> We cannot use the same local variable name as the declared per_cpu
> variable since commit "percpu: remove per_cpu__ prefix."
>
> Otherwise we would see crashes like:
> general protection fault: 0000 [#1] SMP
> last sysfs file:
> CPU 1
> Modules linked in:
> Pid: 1, comm: swapper Tainted: G        W  2.6.32-rc5-mm1_64 #860
> RIP: 0010:[<ffffffff8142ff94>]  [<ffffffff8142ff94>] start_cpu_timer+0x2b/0x87
> ...
>
> Use slqb_ prefix for the global variable so that we don't collide
> even with the rest of the kernel (s390 and alpha need this).
>
> Signed-off-by: Jiri Slaby <jirislaby@gmail.com>
> Cc: Nick Piggin <npiggin@suse.de>
> Cc: Tejun Heo <tj@kernel.org>
> Cc: Rusty Russell <rusty@rustcorp.com.au>
> Cc: Christoph Lameter <cl@linux-foundation.org>

Tested-by: Dave Young <hidave.darkstar@gmail.com>

> ---
>  mm/slqb.c |   10 ++++++----
>  1 files changed, 6 insertions(+), 4 deletions(-)
>
> diff --git a/mm/slqb.c b/mm/slqb.c
> index e745d9a..e4bb53f 100644
> --- a/mm/slqb.c
> +++ b/mm/slqb.c
> @@ -2766,11 +2766,12 @@ out:
>        schedule_delayed_work(work, round_jiffies_relative(3*HZ));
>  }
>
> -static DEFINE_PER_CPU(struct delayed_work, cache_trim_work);
> +static DEFINE_PER_CPU(struct delayed_work, slqb_cache_trim_work);
>
>  static void __cpuinit start_cpu_timer(int cpu)
>  {
> -       struct delayed_work *cache_trim_work = &per_cpu(cache_trim_work, cpu);
> +       struct delayed_work *cache_trim_work = &per_cpu(slqb_cache_trim_work,
> +                       cpu);
>
>        /*
>         * When this gets called from do_initcalls via cpucache_init(),
> @@ -3136,8 +3137,9 @@ static int __cpuinit slab_cpuup_callback(struct notifier_block *nfb,
>
>        case CPU_DOWN_PREPARE:
>        case CPU_DOWN_PREPARE_FROZEN:
> -               cancel_rearming_delayed_work(&per_cpu(cache_trim_work, cpu));
> -               per_cpu(cache_trim_work, cpu).work.func = NULL;
> +               cancel_rearming_delayed_work(&per_cpu(slqb_cache_trim_work,
> +                                       cpu));
> +               per_cpu(slqb_cache_trim_work, cpu).work.func = NULL;
>                break;
>
>        case CPU_UP_CANCELED:
> --
> 1.6.4.2
>
> --
> To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
> Please read the FAQ at  http://www.tux.org/lkml/
>



-- 
Regards
dave

--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to majordomo@kvack.org.  For more info on Linux MM,
see: http://www.linux-mm.org/ .
Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>

Re: [PATCH 1/1] MM: slqb, fix per_cpu access

[Rusty Russell]

On Mon, 2 Nov 2009 08:42:58 am Jiri Slaby wrote:
> We cannot use the same local variable name as the declared per_cpu
> variable since commit "percpu: remove per_cpu__ prefix."
> 
> Otherwise we would see crashes like:
> general protection fault: 0000 [#1] SMP
> last sysfs file:
> CPU 1
> Modules linked in:
> Pid: 1, comm: swapper Tainted: G        W  2.6.32-rc5-mm1_64 #860
> RIP: 0010:[<ffffffff8142ff94>]  [<ffffffff8142ff94>] start_cpu_timer+0x2b/0x87
> ...
> 
> Signed-off-by: Jiri Slaby <jirislaby@gmail.com>
> Cc: Nick Piggin <npiggin@suse.de>
> Cc: Tejun Heo <tj@kernel.org>
> Cc: Rusty Russell <rusty@rustcorp.com.au>
> Cc: Christoph Lameter <cl@linux-foundation.org>
> ---
>  mm/slqb.c |    8 ++++----
>  1 files changed, 4 insertions(+), 4 deletions(-)
> 
> diff --git a/mm/slqb.c b/mm/slqb.c
> index e745d9a..27f5025 100644
> --- a/mm/slqb.c
> +++ b/mm/slqb.c
> @@ -2770,16 +2770,16 @@ static DEFINE_PER_CPU(struct delayed_work, cache_trim_work);
>  
>  static void __cpuinit start_cpu_timer(int cpu)
>  {
> -	struct delayed_work *cache_trim_work = &per_cpu(cache_trim_work, cpu);
> +	struct delayed_work *_cache_trim_work = &per_cpu(cache_trim_work, cpu);
>  
>  	/*
>  	 * When this gets called from do_initcalls via cpucache_init(),
>  	 * init_workqueues() has already run, so keventd will be setup
>  	 * at that time.
>  	 */
> -	if (keventd_up() && cache_trim_work->work.func == NULL) {
> -		INIT_DELAYED_WORK(cache_trim_work, cache_trim_worker);
> -		schedule_delayed_work_on(cpu, cache_trim_work,
> +	if (keventd_up() && _cache_trim_work->work.func == NULL) {
> +		INIT_DELAYED_WORK(_cache_trim_work, cache_trim_worker);
> +		schedule_delayed_work_on(cpu, _cache_trim_work,
>  					__round_jiffies_relative(HZ, cpu));

How about calling the local var "trim"?

This actually makes the code more readable, IMHO.

Thanks,
Rusty.

--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to majordomo@kvack.org.  For more info on Linux MM,
see: http://www.linux-mm.org/ .
Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>

Re: [PATCH 1/1] MM: slqb, fix per_cpu access

[Tejun Heo]

2009e?? 11i?? 02i? 1/4  17:49, Jiri Slaby wrote:
> We cannot use the same local variable name as the declared per_cpu
> variable since commit "percpu: remove per_cpu__ prefix."
> 
> Otherwise we would see crashes like:
> general protection fault: 0000 [#1] SMP
> last sysfs file:
> CPU 1
> Modules linked in:
> Pid: 1, comm: swapper Tainted: G        W  2.6.32-rc5-mm1_64 #860
> RIP: 0010:[<ffffffff8142ff94>]  [<ffffffff8142ff94>] start_cpu_timer+0x2b/0x87
> ...
> 
> Use slqb_ prefix for the global variable so that we don't collide
> even with the rest of the kernel (s390 and alpha need this).
> 
> Signed-off-by: Jiri Slaby <jirislaby@gmail.com>
> Cc: Nick Piggin <npiggin@suse.de>
> Cc: Tejun Heo <tj@kernel.org>
> Cc: Rusty Russell <rusty@rustcorp.com.au>
> Cc: Christoph Lameter <cl@linux-foundation.org>

Acked-by: Tejun Heo <tj@kernel.org>

Thanks.

-- 
tejun

--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to majordomo@kvack.org.  For more info on Linux MM,
see: http://www.linux-mm.org/ .
Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>

Re: [PATCH 1/1] MM: slqb, fix per_cpu access

[Jiri Slaby]

On 11/02/2009 02:23 PM, Rusty Russell wrote:
>> --- a/mm/slqb.c
>> +++ b/mm/slqb.c
>> @@ -2770,16 +2770,16 @@ static DEFINE_PER_CPU(struct delayed_work, cache_trim_work);
>>  
>>  static void __cpuinit start_cpu_timer(int cpu)
>>  {
>> -	struct delayed_work *cache_trim_work = &per_cpu(cache_trim_work, cpu);
>> +	struct delayed_work *_cache_trim_work = &per_cpu(cache_trim_work, cpu);
>>  
>>  	/*
>>  	 * When this gets called from do_initcalls via cpucache_init(),
>>  	 * init_workqueues() has already run, so keventd will be setup
>>  	 * at that time.
>>  	 */
>> -	if (keventd_up() && cache_trim_work->work.func == NULL) {
>> -		INIT_DELAYED_WORK(cache_trim_work, cache_trim_worker);
>> -		schedule_delayed_work_on(cpu, cache_trim_work,
>> +	if (keventd_up() && _cache_trim_work->work.func == NULL) {
>> +		INIT_DELAYED_WORK(_cache_trim_work, cache_trim_worker);
>> +		schedule_delayed_work_on(cpu, _cache_trim_work,
>>  					__round_jiffies_relative(HZ, cpu));
> 
> How about calling the local var "trim"?
> 
> This actually makes the code more readable, IMHO.

Please ignore this version of the patch. After this I sent a new one
which changes the global var name.

So the local variable is untouched there. If you want me to perform the
cleanup, let me know. In any case I'd make it trim_work instead of trim
which makes more sense to me.

--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to majordomo@kvack.org.  For more info on Linux MM,
see: http://www.linux-mm.org/ .
Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>

Re: [PATCH 1/1] MM: slqb, fix per_cpu access

[Christoph Lameter]

Reviewed-by: Christoph Lameter <cl@linux-foundation.org>


--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to majordomo@kvack.org.  For more info on Linux MM,
see: http://www.linux-mm.org/ .
Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>

Re: [PATCH 1/1] MM: slqb, fix per_cpu access

[Rusty Russell]

On Tue, 3 Nov 2009 02:01:41 am Jiri Slaby wrote:
> >> -	struct delayed_work *cache_trim_work = &per_cpu(cache_trim_work, cpu);
> >> +	struct delayed_work *_cache_trim_work = &per_cpu(cache_trim_work, cpu);
> >>  
> >>  	/*
> >>  	 * When this gets called from do_initcalls via cpucache_init(),
> >>  	 * init_workqueues() has already run, so keventd will be setup
> >>  	 * at that time.
> >>  	 */
> >> -	if (keventd_up() && cache_trim_work->work.func == NULL) {
> >> -		INIT_DELAYED_WORK(cache_trim_work, cache_trim_worker);
> >> -		schedule_delayed_work_on(cpu, cache_trim_work,
> >> +	if (keventd_up() && _cache_trim_work->work.func == NULL) {
> >> +		INIT_DELAYED_WORK(_cache_trim_work, cache_trim_worker);
> >> +		schedule_delayed_work_on(cpu, _cache_trim_work,
> >>  					__round_jiffies_relative(HZ, cpu));
> > 
> > How about calling the local var "trim"?
> > 
> > This actually makes the code more readable, IMHO.
> 
> Please ignore this version of the patch. After this I sent a new one
> which changes the global var name.

OK, sure.  It's not worth changing unless you were doing a rename anyway.

> So the local variable is untouched there. If you want me to perform the
> cleanup, let me know. In any case I'd make it trim_work instead of trim
> which makes more sense to me.

This is getting pedantic and marginal, but the word "work" already appears
everywhere this var is used.  Either "XXX->work", or "INIT_DELAYED_WORK(XXX"
or "scheduled_delayed_work_on(cpu, XXX".

That's why I think the word "work" in unnecessary.

Hope that clarifies why I preferred "trim".
Rusty.

--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to majordomo@kvack.org.  For more info on Linux MM,
see: http://www.linux-mm.org/ .
Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>