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(+)
diff --git a/mm/memory_hotplug.c b/mm/memory_hotplug.c
index 5eb4a4c7c21b..328878b6799d 100644
--- a/mm/memory_hotplug.c
+++ b/mm/memory_hotplug.c
@@ -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);
--
2.20.1
On 10.04.19 14:28, Oscar Salvador wrote:
> On Wed, Apr 10, 2019 at 12:14:55PM +0200, David Hildenbrand wrote:
>> 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 ...
--
Thanks,
David / dhildenb
On Wed, Apr 10, 2019 at 12:14:55PM +0200, David Hildenbrand wrote:
> 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.
>
> 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]>
--
Oscar Salvador
SUSE L3
On Wed, Apr 10, 2019 at 12:14:55PM +0200, David Hildenbrand wrote:
>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]>
You are right.
Reviewed-by: Wei Yang <[email protected]>
>---
> mm/memory_hotplug.c | 1 +
> 1 file changed, 1 insertion(+)
>
>diff --git a/mm/memory_hotplug.c b/mm/memory_hotplug.c
>index 5eb4a4c7c21b..328878b6799d 100644
>--- a/mm/memory_hotplug.c
>+++ b/mm/memory_hotplug.c
>@@ -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);
>--
>2.20.1
--
Wei Yang
Help you, Help me
On Wed 10-04-19 12:14:55, David Hildenbrand wrote:
> 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.
"
> 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]>
> ---
> mm/memory_hotplug.c | 1 +
> 1 file changed, 1 insertion(+)
>
> diff --git a/mm/memory_hotplug.c b/mm/memory_hotplug.c
> index 5eb4a4c7c21b..328878b6799d 100644
> --- a/mm/memory_hotplug.c
> +++ b/mm/memory_hotplug.c
> @@ -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);
> --
> 2.20.1
--
Michal Hocko
SUSE Labs
On 11.04.19 10:41, Michal Hocko wrote:
> On Wed 10-04-19 12:14:55, David Hildenbrand wrote:
>> 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.
> "
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().
"
>
>> 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]>
>
>> ---
>> mm/memory_hotplug.c | 1 +
>> 1 file changed, 1 insertion(+)
>>
>> diff --git a/mm/memory_hotplug.c b/mm/memory_hotplug.c
>> index 5eb4a4c7c21b..328878b6799d 100644
>> --- a/mm/memory_hotplug.c
>> +++ b/mm/memory_hotplug.c
>> @@ -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);
>> --
>> 2.20.1
>
--
Thanks,
David / dhildenb
On Thu 11-04-19 11:11:05, David Hildenbrand wrote:
> On 11.04.19 10:41, Michal Hocko wrote:
> > On Wed 10-04-19 12:14:55, David Hildenbrand wrote:
> >> 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.
> > "
>
> 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().
> "
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!
--
Michal Hocko
SUSE Labs
On 11.04.19 12:56, Michal Hocko wrote:
> On Thu 11-04-19 11:11:05, David Hildenbrand wrote:
>> On 11.04.19 10:41, Michal Hocko wrote:
>>> On Wed 10-04-19 12:14:55, David Hildenbrand wrote:
>>>> 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.
>>> "
>>
>> 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().
>> "
>
> 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!
--
Thanks,
David / dhildenb
On Thu 11-04-19 13:18:07, David Hildenbrand wrote:
> On 11.04.19 12:56, Michal Hocko wrote:
> > On Thu 11-04-19 11:11:05, David Hildenbrand wrote:
> >> On 11.04.19 10:41, Michal Hocko wrote:
> >>> On Wed 10-04-19 12:14:55, David Hildenbrand wrote:
> >>>> 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.
> >>> "
> >>
> >> 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().
> >> "
> >
> > 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.
> But as you said, only leaks of small amount of memory.
Yes, as long as there are no other side sysfs related effects.
--
Michal Hocko
SUSE Labs