Re: [PATCH v6 6/7] mm/memory_hotplug: Embed vmem_altmap details in memory block

[Michal Hocko <mhocko@suse.com>]

On Thu 27-07-23 15:02:12, Aneesh Kumar K V wrote:
> On 7/27/23 2:55 PM, Michal Hocko wrote:
> > On Thu 27-07-23 13:32:31, Aneesh Kumar K.V wrote:
> >> With memmap on memory, some architecture needs more details w.r.t altmap
> >> such as base_pfn, end_pfn, etc to unmap vmemmap memory. Instead of
> >> computing them again when we remove a memory block, embed vmem_altmap
> >> details in struct memory_block if we are using memmap on memory block
> >> feature.
> >>
> >> No functional change in this patch
> >>
> >> Signed-off-by: Aneesh Kumar K.V <aneesh.kumar@linux.ibm.com>
> >> ---
> >>  drivers/base/memory.c  | 25 +++++++++++-------
> >>  include/linux/memory.h |  8 ++----
> >>  mm/memory_hotplug.c    | 58 +++++++++++++++++++++++++++---------------
> >>  3 files changed, 55 insertions(+), 36 deletions(-)
> >>
> >> diff --git a/drivers/base/memory.c b/drivers/base/memory.c
> >> index b456ac213610..57ed61212277 100644
> >> --- a/drivers/base/memory.c
> >> +++ b/drivers/base/memory.c
> >> @@ -106,6 +106,7 @@ static void memory_block_release(struct device *dev)
> >>  {
> >>  	struct memory_block *mem = to_memory_block(dev);
> >>  
> >> +	WARN_ON(mem->altmap);
> > 
> > What is this supposed to catch? A comment would be handy so that we know
> > what to look at should it ever trigger.
> > 
> 
> I did add a comment where we clear the altmap in try_remove_memory(). I will also add
> more details here.
> 
> +			 * Mark altmap NULL so that we can add a debug
> +			 * check on memblock free.
>  			 */
> 
> WARN_ON is an indication of memory leak because if we have mem->altmap != NULL
> then the allocated altmap is not freed . It also indicate that memblock got freed
> without going through the try_remove_memory(). 

I think it would be better to be explicit here (who should free up but
hasn't).

-- 
Michal Hocko
SUSE Labs