From: Michael Ellerman <mpe@ellerman.id.au> Date: 2015-11-10 01:21:33
Hi John,
On Tue, 2015-11-03 at 11:21 -0600, John Allen wrote:
This patch fixes a bug where a kernel warning is triggered when performing
a memory hotplug on ppc64. This warning may also occur on any architecture
that has multiple sections per memory block.
So it looks like the only arches that enable this code at all are powerpc, sh
and x86 (via CONFIG_ARCH_MEMORY_PROBE). And it sounds like on x86 it's just
there for debugging, ACPI is meant to notify you of memory hotplug by the
sounds.
Do any of sh or x86 have "multiple sections per memory block" ?
If not then this bug would only apply to powerpc, which would be useful to
know.
And what is the actual warning? ie. what does the code look like. My line 210
of memory.c is a printk() not a WARN.
For change logs I think it's nice to trim the oops a bit. Others probably have
different opinions but I'd remove the printk timestamp, the GPRs and some of
the other regs and the instruction dump, so more like:
WARNING: at ../drivers/base/memory.c:210
CPU: 1 PID: 3090 Comm: systemd-udevd Tainted: G X 3.12.45-1-default #1
NIP [c0000000004fcff8] memory_block_action+0x258/0x2e0
LR [c0000000004fda84] memory_subsys_online+0x54/0x100
Call Trace:
[c0000004d7b93bb0] [c000000009071ce0] 0xc000000009071ce0 (unreliable)
[c0000004d7b93c40] [c0000000004fda84] memory_subsys_online+0x54/0x100
[c0000004d7b93c70] [c0000000004df784] device_online+0xb4/0x120
[c0000004d7b93cb0] [c0000000004fd738] store_mem_state+0x88/0x220
[c0000004d7b93cf0] [c0000000004db448] dev_attr_store+0x68/0xa0
[c0000004d7b93d30] [c00000000031f938] sysfs_write_file+0xf8/0x1d0
[c0000004d7b93d90] [c00000000027d29c] vfs_write+0xec/0x250
[c0000004d7b93de0] [c00000000027dfdc] SyS_write+0x6c/0xf0
[c0000004d7b93e30] [c00000000000a17c] syscall_exit+0x0/0x7c
Also looking at the trace, it's from 3.12.45-1, which is pretty old. Have you
also tested on mainline?
The warning is triggered because there is a udev rule that automatically
tries to online memory after it has been added. The udev rule varies from
distro to distro, but will generally look something like:
SUBSYSTEM=="memory", ACTION=="add", ATTR{state}=="offline", ATTR{state}="online"
On any architecture that uses memory_probe_store to reserve memory,
this can interrupt the memory reservation process. This patch modifies
memory_probe_store to take the hotplug sysfs lock to prevent the online
of added memory before the completion of the probe.
So presumably it's add_memory() that is causing a uevent to fire? I can't see
where that happens but I guess it's in there somewhere.
And that's a problem because we've only added one (or some) of the sections, so
the memory block is not fully populated, and then the add path hits the warning
because of that. Am I right?
quoted hunk
Signed-off-by: John Allen <redacted>
---
v2: Move call to unlock_device_hotplug under "out" label
This looks OK. What I don't know is what the locking rules in add_memory() are.
It takes the mem_hotplug lock, which is also a mutex. I suspect it's fine to
nest the mem_hotplug lock within the sysfs one, but it would be good if you can
try and confirm. Also if you run with lockdep enabled it should tell you if
you've got it wrong.
cheers
From: John Allen <hidden> Date: 2015-11-24 20:15:56
Hi Michael,
On 11/09/2015 07:21 PM, Michael Ellerman wrote:
Hi John,
On Tue, 2015-11-03 at 11:21 -0600, John Allen wrote:
quoted
This patch fixes a bug where a kernel warning is triggered when performing
a memory hotplug on ppc64. This warning may also occur on any architecture
that has multiple sections per memory block.
So it looks like the only arches that enable this code at all are powerpc, sh
and x86 (via CONFIG_ARCH_MEMORY_PROBE). And it sounds like on x86 it's just
there for debugging, ACPI is meant to notify you of memory hotplug by the
sounds.
Do any of sh or x86 have "multiple sections per memory block" ?
If not then this bug would only apply to powerpc, which would be useful to
know.
Sorry for the delayed response. It looks like your reply got buried in my
lkml folder instead of going straight to my inbox.
My understanding is that x86 may use multiple sections per block. I
think we would have to assume that any architecture that uses this code
could potentially hit this issue.
And what is the actual warning? ie. what does the code look like. My line 210
of memory.c is a printk() not a WARN.
In mainline, the warning is at line 200:
if (WARN_ON_ONCE(!pfn_valid(pfn)))
return false;
For change logs I think it's nice to trim the oops a bit. Others probably have
different opinions but I'd remove the printk timestamp, the GPRs and some of
the other regs and the instruction dump, so more like:
WARNING: at ../drivers/base/memory.c:210
CPU: 1 PID: 3090 Comm: systemd-udevd Tainted: G X 3.12.45-1-default #1
NIP [c0000000004fcff8] memory_block_action+0x258/0x2e0
LR [c0000000004fda84] memory_subsys_online+0x54/0x100
Call Trace:
[c0000004d7b93bb0] [c000000009071ce0] 0xc000000009071ce0 (unreliable)
[c0000004d7b93c40] [c0000000004fda84] memory_subsys_online+0x54/0x100
[c0000004d7b93c70] [c0000000004df784] device_online+0xb4/0x120
[c0000004d7b93cb0] [c0000000004fd738] store_mem_state+0x88/0x220
[c0000004d7b93cf0] [c0000000004db448] dev_attr_store+0x68/0xa0
[c0000004d7b93d30] [c00000000031f938] sysfs_write_file+0xf8/0x1d0
[c0000004d7b93d90] [c00000000027d29c] vfs_write+0xec/0x250
[c0000004d7b93de0] [c00000000027dfdc] SyS_write+0x6c/0xf0
[c0000004d7b93e30] [c00000000000a17c] syscall_exit+0x0/0x7c
Also looking at the trace, it's from 3.12.45-1, which is pretty old. Have you
also tested on mainline?
All testing and debugging was done on a mainline kernel. The traces with
the 3.12 kernel were just what was submitted in the initial bug report.
quoted
The warning is triggered because there is a udev rule that automatically
tries to online memory after it has been added. The udev rule varies from
distro to distro, but will generally look something like:
SUBSYSTEM=="memory", ACTION=="add", ATTR{state}=="offline", ATTR{state}="online"
On any architecture that uses memory_probe_store to reserve memory,
this can interrupt the memory reservation process. This patch modifies
memory_probe_store to take the hotplug sysfs lock to prevent the online
of added memory before the completion of the probe.
So presumably it's add_memory() that is causing a uevent to fire? I can't see
where that happens but I guess it's in there somewhere.
And that's a problem because we've only added one (or some) of the sections, so
the memory block is not fully populated, and then the add path hits the warning
because of that. Am I right?
Yes, that's right. While we are in the process of reserving the sections of the
block,the uevent gets triggered and calls store_mem_state(). This then calls
pages_correctly_reserved() which finds that the block has only been partially
reserved, triggering the warning.
quoted
Signed-off-by: John Allen <redacted>
---
v2: Move call to unlock_device_hotplug under "out" label
This looks OK. What I don't know is what the locking rules in add_memory() are.
It takes the mem_hotplug lock, which is also a mutex. I suspect it's fine to
nest the mem_hotplug lock within the sysfs one, but it would be good if you can
try and confirm. Also if you run with lockdep enabled it should tell you if
you've got it wrong.
My assumption is that nesting the locks in this way is alright. We can see this
same nesting structure used in store_mem_state(). I never ran into any locking
issues in my testing, but I can test with lockdep enabled if you think that's
necessary.
-John