mm/memory_hotplug: Drop memory device reference after find_memory_block()

PATCH Superseded David Hildenbrand 4 participants 9 messages Apr 10, 2019 – Apr 11, 2019

Subsystems (2)

Memory management Memory hot(un)plug

lore

Share & tools

lore

David Hildenbrand

author

2019-04-10 10:15 UTC #0 lore raw

PATCH 1/1 mm/memory_hotplug: Drop memory device reference after find_memory_block()

While current node handling is probably terribly broken for memory block devices that span several nodes (only possible when added during boot, and something like that should be blocked completely), properly put the device reference we obtained via find_memory_block() to get the nid.

Fixes d0dc12e86b31 ("mm/memory_hotplug: optimize memory hotplug")

Cc Andrew Morton <[email protected]>

Cc Oscar Salvador <[email protected]>

Cc Michal Hocko <[email protected]>

Cc David Hildenbrand <[email protected]>

Cc Pavel Tatashin <[email protected]>

Cc Wei Yang <[email protected]>

Cc Qian Cai <[email protected]>

Cc Arun KS <[email protected]>

Cc Mathieu Malaterre <[email protected]>

Signed-off-by David Hildenbrand <[email protected]>

---
 mm/memory_hotplug.c | 1 +
 1 file changed, 1 insertion(+)

mm/memory_hotplug.c +1 -0

index 5eb4a4c7c21b..328878b6799d 100644
@@ -854,6 +854,7 @@ int __ref online_pages(unsigned long pfn, unsigned long nr_pages, int online_typ
 	 */
 	mem = find_memory_block(__pfn_to_section(pfn));
 	nid = mem->nid;
+	put_device(&mem->dev);
 
 	/* associate pfn range with the zone */
 	zone = move_pfn_range(online_type, nid, pfn, nr_pages);

Signature · 1 line

2.20.1

Oscar Salvador

maintainer Reviewed-by on v1 1/1

2019-04-10 12:28 UTC #1 lore raw

Quoted 4 lines · David Hildenbrand

While current node handling is probably terribly broken for memory block devices that span several nodes (only possible when added during boot, and something like that should be blocked completely), properly put the device reference we obtained via find_memory_block() to get the nid.

We even have nodes sharing sections, so tricky to "fix".
But I agree that the way memblocks are being handled now sucks big time.

Quoted 12 lines

Fixes d0dc12e86b31 ("mm/memory_hotplug: optimize memory hotplug")
Cc Andrew Morton <[email protected]>
Cc Oscar Salvador <[email protected]>
Cc Michal Hocko <[email protected]>
Cc David Hildenbrand <[email protected]>
Cc Pavel Tatashin <[email protected]>
Cc Wei Yang <[email protected]>
Cc Qian Cai <[email protected]>
Cc Arun KS <[email protected]>
Cc Mathieu Malaterre <[email protected]>
Signed-off-by David Hildenbrand <[email protected]>

Well spotted David ;-)

Reviewed-by Oscar Salvador <[email protected]>

Signature · 2 lines

Oscar Salvador
SUSE L3

David Hildenbrand

author

2019-04-10 12:48 UTC #2 lore raw

Quoted 9 lines · Oscar Salvador

Quoted 4 lines · David Hildenbrand
While current node handling is probably terribly broken for memory block devices that span several nodes (only possible when added during boot, and something like that should be blocked completely), properly put the device reference we obtained via find_memory_block() to get the nid.
We even have nodes sharing sections, so tricky to "fix".
But I agree that the way memblocks are being handled now sucks big time.

I'm planning to eventually tackle this via memblocks directly, using "nid" to indicate if mixed sections are contained. So we don't have to scan all the pages ...

Signature · 3 lines

Thanks,

David / dhildenb

Wei Yang

Reviewed-by on v1 1/1

2019-04-10 22:24 UTC #3 lore raw

In reply to David Hildenbrand

Quoted 5 lines · David Hildenbrand

While current node handling is probably terribly broken for memory block devices that span several nodes (only possible when added during boot, and something like that should be blocked completely), properly put the device reference we obtained via find_memory_block() to get the nid.

Quoted 11 lines

Fixes d0dc12e86b31 ("mm/memory_hotplug: optimize memory hotplug")
Cc Andrew Morton <[email protected]>
Cc Oscar Salvador <[email protected]>
Cc Michal Hocko <[email protected]>
Cc David Hildenbrand <[email protected]>
Cc Pavel Tatashin <[email protected]>
Cc Wei Yang <[email protected]>
Cc Qian Cai <[email protected]>
Cc Arun KS <[email protected]>
Cc Mathieu Malaterre <[email protected]>
Signed-off-by David Hildenbrand <[email protected]>

You are right.

Reviewed-by Wei Yang <[email protected]>

Quoted 18 lines

---
mm/memory_hotplug.c | 1 +

1 file changed, 1 insertion(+)

mm/memory_hotplug.c +1 -0

index 5eb4a4c7c21b..328878b6799d 100644
@@ -854,6 +854,7 @@ int __ref online_pages(unsigned long pfn, unsigned long nr_pages, int online_typ
 	 */
 	mem = find_memory_block(__pfn_to_section(pfn));
 	nid = mem->nid;
+	put_device(&mem->dev);
 
 	/* associate pfn range with the zone */
 	zone = move_pfn_range(online_type, nid, pfn, nr_pages);

Signature · 1 line

2.20.1

Signature · 2 lines

Wei Yang
Help you, Help me

Michal Hocko

Acked-by on v1 1/1

2019-04-11 08:41 UTC #4 lore raw

In reply to David Hildenbrand

Quoted 4 lines · David Hildenbrand

While current node handling is probably terribly broken for memory block devices that span several nodes (only possible when added during boot, and something like that should be blocked completely), properly put the device reference we obtained via find_memory_block() to get the nid.

The changelog could see some improvements I believe. (Half) stating broken status of multinode memblock is not really useful without a wider context so I would simply remove it. More to the point, it would be much better to actually describe the actual problem and the user visible effect.

"
d0dc12e86b31 ("mm/memory_hotplug: optimize memory hotplug") has started using find_memory_block to get a nodeid for the beginnig of the onlined pfn range. The commit has missed that the memblock contains a reference counted object and a missing put_device will leak the kobject behind which ADD THE USER VISIBLE EFFECT HERE.
"

Quoted 11 lines

Fixes d0dc12e86b31 ("mm/memory_hotplug: optimize memory hotplug")
Cc Andrew Morton <[email protected]>
Cc Oscar Salvador <[email protected]>
Cc Michal Hocko <[email protected]>
Cc David Hildenbrand <[email protected]>
Cc Pavel Tatashin <[email protected]>
Cc Wei Yang <[email protected]>
Cc Qian Cai <[email protected]>
Cc Arun KS <[email protected]>
Cc Mathieu Malaterre <[email protected]>
Signed-off-by David Hildenbrand <[email protected]>

Other than that

Acked-by Michal Hocko <[email protected]>

Quoted 18 lines

---
 mm/memory_hotplug.c | 1 +
 1 file changed, 1 insertion(+)

mm/memory_hotplug.c +1 -0

index 5eb4a4c7c21b..328878b6799d 100644
@@ -854,6 +854,7 @@ int __ref online_pages(unsigned long pfn, unsigned long nr_pages, int online_typ
 	 */
 	mem = find_memory_block(__pfn_to_section(pfn));
 	nid = mem->nid;
+	put_device(&mem->dev);
 
 	/* associate pfn range with the zone */
 	zone = move_pfn_range(online_type, nid, pfn, nr_pages);

Signature · 1 line

2.20.1

Signature · 2 lines

Michal Hocko
SUSE Labs

David Hildenbrand

author

2019-04-11 09:11 UTC #5 lore raw

Quoted 12 lines · Michal Hocko

Quoted 4 lines · David Hildenbrand
While current node handling is probably terribly broken for memory block devices that span several nodes (only possible when added during boot, and something like that should be blocked completely), properly put the device reference we obtained via find_memory_block() to get the nid.
The changelog could see some improvements I believe. (Half) stating broken status of multinode memblock is not really useful without a wider context so I would simply remove it. More to the point, it would be much better to actually describe the actual problem and the user visible effect.

Quoted 7 lines

"
d0dc12e86b31 ("mm/memory_hotplug: optimize memory hotplug") has started using find_memory_block to get a nodeid for the beginnig of the onlined pfn range. The commit has missed that the memblock contains a reference counted object and a missing put_device will leak the kobject behind which ADD THE USER VISIBLE EFFECT HERE.
"

I don't think mentioning the commit a second time is really needed.

"
Right now we are using find_memory_block() to get the node id for the pfn range to online. We are missing to drop a reference to the memory block device. While the device still gets unregistered via device_unregister(), resulting in no user visible problem, the device is never released via device_release(), resulting in a memory leak. Fix that by properly using a put_device().
"

Quoted 35 lines

Quoted 12 lines
Fixes d0dc12e86b31 ("mm/memory_hotplug: optimize memory hotplug")
Cc Andrew Morton <[email protected]>
Cc Oscar Salvador <[email protected]>
Cc Michal Hocko <[email protected]>
Cc David Hildenbrand <[email protected]>
Cc Pavel Tatashin <[email protected]>
Cc Wei Yang <[email protected]>
Cc Qian Cai <[email protected]>
Cc Arun KS <[email protected]>
Cc Mathieu Malaterre <[email protected]>
Signed-off-by David Hildenbrand <[email protected]>
Other than that
Acked-by Michal Hocko <[email protected]>
Quoted 18 lines
---
 mm/memory_hotplug.c | 1 +
 1 file changed, 1 insertion(+)
mm/memory_hotplug.c +1 -0
index 5eb4a4c7c21b..328878b6799d 100644
@@ -854,6 +854,7 @@ int __ref online_pages(unsigned long pfn, unsigned long nr_pages, int online_typ
 	 */
 	mem = find_memory_block(__pfn_to_section(pfn));
 	nid = mem->nid;
+	put_device(&mem->dev);
 
 	/* associate pfn range with the zone */
 	zone = move_pfn_range(online_type, nid, pfn, nr_pages);
Signature · 1 line
2.20.1

Signature · 3 lines

Thanks,

David / dhildenb

Michal Hocko

2019-04-11 10:56 UTC #6 lore raw

Quoted 23 lines · David Hildenbrand

Quoted 12 lines · Michal Hocko
Quoted 4 lines · David Hildenbrand
While current node handling is probably terribly broken for memory block devices that span several nodes (only possible when added during boot, and something like that should be blocked completely), properly put the device reference we obtained via find_memory_block() to get the nid.
The changelog could see some improvements I believe. (Half) stating broken status of multinode memblock is not really useful without a wider context so I would simply remove it. More to the point, it would be much better to actually describe the actual problem and the user visible effect.
Quoted 7 lines
"
d0dc12e86b31 ("mm/memory_hotplug: optimize memory hotplug") has started using find_memory_block to get a nodeid for the beginnig of the onlined pfn range. The commit has missed that the memblock contains a reference counted object and a missing put_device will leak the kobject behind which ADD THE USER VISIBLE EFFECT HERE.
"
I don't think mentioning the commit a second time is really needed.

Quoted 8 lines

"
Right now we are using find_memory_block() to get the node id for the pfn range to online. We are missing to drop a reference to the memory block device. While the device still gets unregistered via device_unregister(), resulting in no user visible problem, the device is never released via device_release(), resulting in a memory leak. Fix that by properly using a put_device().
"

OK, sounds good to me. I was not sure about all the sysfs machinery and the kobj dependencies but if there are no sysfs files leaking and crashing upon a later access then a leak of a small amount of memory that is not user controlable then this is not super urgent.

Thanks!

Signature · 2 lines

Michal Hocko
SUSE Labs

David Hildenbrand

author

2019-04-11 11:18 UTC #7 lore raw

Quoted 33 lines · Michal Hocko

Quoted 23 lines · David Hildenbrand
Quoted 12 lines · Michal Hocko
Quoted 4 lines · David Hildenbrand
While current node handling is probably terribly broken for memory block devices that span several nodes (only possible when added during boot, and something like that should be blocked completely), properly put the device reference we obtained via find_memory_block() to get the nid.
The changelog could see some improvements I believe. (Half) stating broken status of multinode memblock is not really useful without a wider context so I would simply remove it. More to the point, it would be much better to actually describe the actual problem and the user visible effect.
Quoted 7 lines
"
d0dc12e86b31 ("mm/memory_hotplug: optimize memory hotplug") has started using find_memory_block to get a nodeid for the beginnig of the onlined pfn range. The commit has missed that the memblock contains a reference counted object and a missing put_device will leak the kobject behind which ADD THE USER VISIBLE EFFECT HERE.
"
I don't think mentioning the commit a second time is really needed.
Quoted 8 lines
"
Right now we are using find_memory_block() to get the node id for the pfn range to online. We are missing to drop a reference to the memory block device. While the device still gets unregistered via device_unregister(), resulting in no user visible problem, the device is never released via device_release(), resulting in a memory leak. Fix that by properly using a put_device().
"

Quoted 6 lines

OK, sounds good to me. I was not sure about all the sysfs machinery and the kobj dependencies but if there are no sysfs files leaking and crashing upon a later access then a leak of a small amount of memory that is not user controlable then this is not super urgent.
Thanks!

I think it can be triggered by onlining/offlining memory in a loop. But as you said, only leaks of small amount of memory.

Thanks!

Signature · 3 lines

Thanks,

David / dhildenb

Michal Hocko

2019-04-11 11:32 UTC #8 lore raw

Quoted 34 lines · David Hildenbrand

Quoted 33 lines · Michal Hocko
Quoted 31 lines · David Hildenbrand
Quoted 12 lines · Michal Hocko
Quoted 4 lines · David Hildenbrand
While current node handling is probably terribly broken for memory block devices that span several nodes (only possible when added during boot, and something like that should be blocked completely), properly put the device reference we obtained via find_memory_block() to get the nid.
The changelog could see some improvements I believe. (Half) stating broken status of multinode memblock is not really useful without a wider context so I would simply remove it. More to the point, it would be much better to actually describe the actual problem and the user visible effect.
Quoted 7 lines
"
d0dc12e86b31 ("mm/memory_hotplug: optimize memory hotplug") has started using find_memory_block to get a nodeid for the beginnig of the onlined pfn range. The commit has missed that the memblock contains a reference counted object and a missing put_device will leak the kobject behind which ADD THE USER VISIBLE EFFECT HERE.
"
I don't think mentioning the commit a second time is really needed.
"
Right now we are using find_memory_block() to get the node id for the pfn range to online. We are missing to drop a reference to the memory block device. While the device still gets unregistered via device_unregister(), resulting in no user visible problem, the device is never released via device_release(), resulting in a memory leak. Fix that by properly using a put_device().
"

Quoted 8 lines

Quoted 6 lines
OK, sounds good to me. I was not sure about all the sysfs machinery and the kobj dependencies but if there are no sysfs files leaking and crashing upon a later access then a leak of a small amount of memory that is not user controlable then this is not super urgent.
Thanks!
I think it can be triggered by onlining/offlining memory in a loop.

which is a privileged operation so the impact is limited.

Quoted 1 line

But as you said, only leaks of small amount of memory.

Yes, as long as there are no other side sysfs related effects.

Signature · 2 lines

Michal Hocko
SUSE Labs