Is per_cpu_ptr_to_phys broken?

PATCH In review Petr Tesarik 3 participants 6 messages Dec 13, 2011 – Dec 16, 2011

Subsystems (2)

Memory management Per-CPU memory allocator

lore

Share & tools

lore

Petr Tesarik

author

2011-12-13 23:34 UTC #0 lore raw

Hi folks,

while trying to understand a weird kdump failure, I found out that the secondary kernel doesn't get the correct NT_PRSTATUS notes from the primary kernel. Further research reveals that the notes are correctly generated, corresponding elfcorehdr program headers are created by kexec, but the physical address is wrong.

The trouble is that the crash_notes per-cpu variable is not page-aligned:

crash_notes = 0xc08e8ed4
PER-CPU OFFSET VALUES:
CPU 0: 3711f000
CPU 1: 37129000
CPU 2: 37133000
CPU 3: 3713d000

So, the per-cpu addresses are:
crash_notes on CPU 0: f7a07ed4 => phys 36b57ed4
crash_notes on CPU 1: f7a11ed4 => phys 36b4ded4
crash_notes on CPU 2: f7a1bed4 => phys 36b43ed4
crash_notes on CPU 3: f7a25ed4 => phys 36b39ed4

However, /sys/devices/system/cpu/cpu*/crash_notes says:
/sys/devices/system/cpu/cpu0/crash_notes: 36b57000 /sys/devices/system/cpu/cpu1/crash_notes: 36b4d000 /sys/devices/system/cpu/cpu2/crash_notes: 36b43000 /sys/devices/system/cpu/cpu3/crash_notes: 36b39000

As you can see, all values are rounded down to a page boundary. Consequently, this is where kexec sets up the NOTE segments, and thus where the secondary kernel is looking for them. However, when the first kernel crashes, it saves the notes to the unaligned addresses, where they are not found.

The value in the crash_notes sysfs attribute are computed as follows:

        addr = per_cpu_ptr_to_phys(per_cpu_ptr(crash_notes, cpunum));

Note that the per-cpu addresses lie between VMALLOC_START (0xf79fe000 on this machine) and VMALLOC_END (0xff1fe000).

Now, the per_cpu_ptr_to_phys() function aligns all vmalloc addresses to a page boundary. This was probably right when Vivek Goyal introduced that function (commit 3b034b0d084221596bf35c8d893e1d4d5477b9cc), because per-cpu addresses were only allocated by vmalloc if booted with percpu_alloc=page, but this is no longer the case, because per-cpu variables are now always allocated that way AFAICS.

So, shouldn't we add the offset within the page inside per_cpu_ptr_to_phys?

Signed-off-by Petr Tesarik <[email protected]>

mm/percpu.c +4 -2

index 3bb810a..4c13334 100644
@@ -998,6 +998,7 @@ phys_addr_t per_cpu_ptr_to_phys(void *addr)
 	bool in_first_chunk = false;
 	unsigned long first_low, first_high;
 	unsigned int cpu;
+	phys_addr_t page_addr;
 
 	/*
 	 * The following test on unit_low/high isn't strictly

@@ -1023,9 +1024,10 @@ phys_addr_t per_cpu_ptr_to_phys(void *addr)
 		if (!is_vmalloc_addr(addr))
 			return __pa(addr);
 		else
-			return page_to_phys(vmalloc_to_page(addr));
+			page_addr = page_to_phys(vmalloc_to_page(addr));
 	} else
-		return page_to_phys(pcpu_addr_to_page(addr));
+		page_addr = page_to_phys(pcpu_addr_to_page(addr));
+	return page_addr + ((unsigned long)addr & ~PAGE_MASK);
 }
 
 /**

--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to [email protected]. For more info on Linux MM,
see: http://www.linux-mm.org/ .
Fight unfair telecom internet charges in Canada: sign http://stopthemeter.ca/
Don't email: <a href=mailto:"[email protected]"> [email protected] </a>

Cong Wang

2011-12-14 16:17 UTC #1 lore raw

Quoted 2 lines · Petr Tesarik

Hi folks,

...

Quoted 10 lines

Now, the per_cpu_ptr_to_phys() function aligns all vmalloc addresses to a page boundary. This was probably right when Vivek Goyal introduced that function (commit 3b034b0d084221596bf35c8d893e1d4d5477b9cc), because per-cpu addresses were only allocated by vmalloc if booted with percpu_alloc=page, but this is no longer the case, because per-cpu variables are now always allocated that way AFAICS.
So, shouldn't we add the offset within the page inside per_cpu_ptr_to_phys?

Hi,

Tejun already fixed this, see:

commit a855b84c3d8c73220d4d3cd392a7bee7c83de70e
percpu: fix chunk range calculation
author Tejun Heo <[email protected]>

Petr Tesarik

author

2011-12-15 10:39 UTC #2 lore raw

Dne St 14. prosince 2011 17:17:15 Cong Wang napsal(a):

Quoted 15 lines · Petr Tesarik

Quoted 1 line
Hi folks,
...
Quoted 9 lines
Now, the per_cpu_ptr_to_phys() function aligns all vmalloc addresses to a page boundary. This was probably right when Vivek Goyal introduced that function (commit 3b034b0d084221596bf35c8d893e1d4d5477b9cc), because per-cpu addresses were only allocated by vmalloc if booted with percpu_alloc=page, but this is no longer the case, because per-cpu variables are now always allocated that way AFAICS.
So, shouldn't we add the offset within the page inside per_cpu_ptr_to_phys?

Quoted 7 lines

Hi,
Tejun already fixed this, see:
commit a855b84c3d8c73220d4d3cd392a7bee7c83de70e
percpu: fix chunk range calculation
author Tejun Heo <[email protected]>

Thanks for looking, but AFAICS this was a different issue. Maybe I'm missing something, but even with Tejun's fix, the first chunk gets allocated by vmalloc, and pcpu objects may not be page-aligned (as is the case with crash notes, which are only aligned to a word boundary).

In particular, the x86 architecture defines NEED_PER_CPU_PAGE_FIRST_CHUNK, so the first chunk gets allocated in pcpu_page_first_chunk():

	vm.flags = VM_ALLOC;
	vm.size = num_possible_cpus() * ai->unit_size;
	vm_area_register_early(&vm, PAGE_SIZE);

This allocates a vmalloc address which is then used to set up the first chunk:

	rc = pcpu_setup_first_chunk(ai, vm.addr);

Later on, crash notes get allocated with:

	crash_notes = alloc_percpu(note_buf_t);

which translates to

	__alloc_percpu(sizeof(note_buf_t), __alignof__(note_buf_t))

Alignment of note_buf_t is 4 bytes (it is an array of u32), so the resulting address may not be page-aligned. However, show_crash_notes() contains:

	addr = per_cpu_ptr_to_phys(per_cpu_ptr(crash_notes, cpunum));
	rc = sprintf(buf, "%Lx\n", addr);

Now, per_cpu_ptr() gives the correct virtual address, but per_cpu_ptr_to_phys() gets the result wrong, regardless whether it thinks that the address is in the first chunk or not:

	if (in_first_chunk) {
		if (!is_vmalloc_addr(addr))
			return __pa(addr);
		else
			return page_to_phys(vmalloc_to_page(addr));
	} else
		return page_to_phys(pcpu_addr_to_page(addr));

For anything except a non-vmalloc address, this will always round the result down to a page boundary. I thought this was obvious...

Petr Tesarik

Tejun Heo

maintainer

2011-12-15 18:40 UTC #3 lore raw

Hello,

Quoted 3 lines

Now, per_cpu_ptr() gives the correct virtual address, but per_cpu_ptr_to_phys() gets the result wrong, regardless whether it thinks that the address is in the first chunk or not:

Yeah, that's me forgetting "+ offset_in_page()" after vmalloc page translation, which incidentally was also discovered by surovegin last night. It has been broken forever, by which I mean longer than six months. I wonder why this is coming up only now. Anyways, please send me a patch, I'll be push it mainline & to stable.

Thanks.

Signature · 8 lines

tejun

--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to [email protected].  For more info on Linux MM,
see: http://www.linux-mm.org/ .
Fight unfair telecom internet charges in Canada: sign http://stopthemeter.ca/
Don't email: <a href=mailto:"[email protected]"> [email protected] </a>

Petr Tesarik

author

2011-12-16 08:04 UTC #4 lore raw

PATCH 1/1 Do not round per_cpu_ptr_to_phys to page boundary

The phys_addr_t per_cpu_ptr_to_phys() function ignores the offset within a page, whenever not using a simple translation using __pa().

Without this patch /sys/devices/system/cpu/cpu*/crash_notes shows incorrect values, which breaks kdump. Other things may also be broken.

Signed-off-by Petr Tesarik <[email protected]>

mm/percpu.c +4 -2

index 3bb810a..1a1b5ac 100644
@@ -998,6 +998,7 @@ phys_addr_t per_cpu_ptr_to_phys(void *addr)
 	bool in_first_chunk = false;
 	unsigned long first_low, first_high;
 	unsigned int cpu;
+	phys_addr_t page_addr;
 
 	/*
 	 * The following test on unit_low/high isn't strictly

@@ -1023,9 +1024,10 @@ phys_addr_t per_cpu_ptr_to_phys(void *addr)
 		if (!is_vmalloc_addr(addr))
 			return __pa(addr);
 		else
-			return page_to_phys(vmalloc_to_page(addr));
+			page_addr = page_to_phys(vmalloc_to_page(addr));
 	} else
-		return page_to_phys(pcpu_addr_to_page(addr));
+		page_addr = page_to_phys(pcpu_addr_to_page(addr));
+	return page_addr + offset_in_page(addr);
 }
 
 /**

Petr Tesarik

author

2011-12-16 08:40 UTC #5 lore raw

Re: [PATCH] Do not round per_cpu_ptr_to_phys to page boundary

Ignore my patch. I can see, a patch has already been accepted.

I only replied too fast (before I read the rest of the thread).

Petr

Dne Pá 16. prosince 2011 09:04:23 Petr Tesarik napsal(a):

Quoted 35 lines

The phys_addr_t per_cpu_ptr_to_phys() function ignores the offset within a page, whenever not using a simple translation using __pa().

Without this patch /sys/devices/system/cpu/cpu*/crash_notes shows incorrect values, which breaks kdump. Other things may also be broken.

Signed-off-by Petr Tesarik <[email protected]>

mm/percpu.c +4 -3

index 3bb810a..1a1b5ac 100644
@@ -998,6 +998,7 @@ phys_addr_t per_cpu_ptr_to_phys(void *addr)
 	bool in_first_chunk = false;
 	unsigned long first_low, first_high;
 	unsigned int cpu;
+	phys_addr_t page_addr;
 
 	/*
 	 * The following test on unit_low/high isn't strictly

@@ -1023,9 +1024,10 @@ phys_addr_t per_cpu_ptr_to_phys(void *addr)
 		if (!is_vmalloc_addr(addr))
 			return __pa(addr);
 		else
-			return page_to_phys(vmalloc_to_page(addr));
+			page_addr = page_to_phys(vmalloc_to_page(addr));
 	} else
-		return page_to_phys(pcpu_addr_to_page(addr));
+		page_addr = page_to_phys(pcpu_addr_to_page(addr));
+	return page_addr + offset_in_page(addr);
 }
 
 /**
 
--

Quoted 6 lines

To unsubscribe, send a message with 'unsubscribe linux-mm' in the body to [email protected]. For more info on Linux MM,
see: http://www.linux-mm.org/ .
Fight unfair telecom internet charges in Canada: sign
http://stopthemeter.ca/ Don't email: <a href=mailto:"[email protected]"> [email protected] </a>