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 <[email protected]>
Cc: Nick Piggin <[email protected]>
Cc: Tejun Heo <[email protected]>
Cc: Rusty Russell <[email protected]>
Cc: Christoph Lameter <[email protected]>
---
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
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
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 <[email protected]>
Cc: Nick Piggin <[email protected]>
Cc: Tejun Heo <[email protected]>
Cc: Rusty Russell <[email protected]>
Cc: Christoph Lameter <[email protected]>
---
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
On Mon, Nov 2, 2009 at 4:49 PM, Jiri Slaby <[email protected]> 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 <[email protected]>
> Cc: Nick Piggin <[email protected]>
> Cc: Tejun Heo <[email protected]>
> Cc: Rusty Russell <[email protected]>
> Cc: Christoph Lameter <[email protected]>
Tested-by: Dave Young <[email protected]>
> ---
> 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 [email protected]
> More majordomo info at http://vger.kernel.org/majordomo-info.html
> Please read the FAQ at http://www.tux.org/lkml/
>
--
Regards
dave
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 <[email protected]>
> Cc: Nick Piggin <[email protected]>
> Cc: Tejun Heo <[email protected]>
> Cc: Rusty Russell <[email protected]>
> Cc: Christoph Lameter <[email protected]>
> ---
> 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.
2009년 11월 02일 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 <[email protected]>
> Cc: Nick Piggin <[email protected]>
> Cc: Tejun Heo <[email protected]>
> Cc: Rusty Russell <[email protected]>
> Cc: Christoph Lameter <[email protected]>
Acked-by: Tejun Heo <[email protected]>
Thanks.
--
tejun
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.
Reviewed-by: Christoph Lameter <[email protected]>
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.