Re: [PATCH v2 1/5] mm/hotplug: Embed vmem_altmap details in memory block

[David Hildenbrand <david@redhat.com>]

On 06.07.23 10:50, 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.

Can you elaborate why ppc64 needs that and x86-64 + aarch64 don't?

IOW, why can't ppc64 simply allocate the vmemmap from the start of the 
memblock (-> base_pfn) and use the stored number of vmemmap pages to 
calculate the end_pfn?

To rephrase: if the vmemmap is not at the beginning and doesn't cover 
full apgeblocks, memory onlining/offlining would be broken.

[...]

>   
> +/**
> + * struct vmem_altmap - pre-allocated storage for vmemmap_populate
> + * @base_pfn: base of the entire dev_pagemap mapping
> + * @reserve: pages mapped, but reserved for driver use (relative to @base)
> + * @free: free pages set aside in the mapping for memmap storage
> + * @align: pages reserved to meet allocation alignments
> + * @alloc: track pages consumed, private to vmemmap_populate()
> + */
> +struct vmem_altmap {
> +	unsigned long base_pfn;
> +	const unsigned long end_pfn;
> +	const unsigned long reserve;
> +	unsigned long free;
> +	unsigned long align;
> +	unsigned long alloc;
> +};

Instead of embedding that, what about conditionally allocating it and 
store a pointer to it in the "struct memory_block"?

In the general case as of today, we don't have an altmap.

> +
>   struct memory_block {
>   	unsigned long start_section_nr;
>   	unsigned long state;		/* serialized by the dev->lock */
> @@ -77,11 +94,7 @@ struct memory_block {
>   	 */
>   	struct zone *zone;
>   	struct device dev;
> -	/*
> -	 * Number of vmemmap pages. These pages
> -	 * lay at the beginning of the memory block.
> -	 */
> -	unsigned long nr_vmemmap_pages;
> +	struct vmem_altmap altmap;
>   	struct memory_group *group;	/* group (if any) for this block */
>   	struct list_head group_next;	/* next block inside memory group */
>   #if defined(CONFIG_MEMORY_FAILURE) && defined(CONFIG_MEMORY_HOTPLUG)
> @@ -147,7 +160,7 @@ static inline int hotplug_memory_notifier(notifier_fn_t fn, int pri)
>   extern int register_memory_notifier(struct notifier_block *nb);
>   extern void unregister_memory_notifier(struct notifier_block *nb);
>   int create_memory_block_devices(unsigned long start, unsigned long size,

[...]

>   static int check_cpu_on_node(int nid)
> @@ -2036,9 +2042,8 @@ EXPORT_SYMBOL(try_offline_node);
>   
>   static int __ref try_remove_memory(u64 start, u64 size)
>   {
> -	struct vmem_altmap mhp_altmap = {};
> +	int ret;
>   	struct vmem_altmap *altmap = NULL;
> -	unsigned long nr_vmemmap_pages;
>   	int rc = 0, nid = NUMA_NO_NODE;
>   
>   	BUG_ON(check_hotplug_memory_range(start, size));
> @@ -2060,24 +2065,16 @@ static int __ref try_remove_memory(u64 start, u64 size)
>   	 * We only support removing memory added with MHP_MEMMAP_ON_MEMORY in
>   	 * the same granularity it was added - a single memory block.
>   	 */
> +

^ unrealted change?

-- 
Cheers,

David / dhildenb