Re: [PATCH v2 3/7] mm, devm_memremap_pages: Fix shutdown handling

[Logan Gunthorpe]

On 22/05/18 11:10 PM, Dan Williams wrote:
> diff --git a/include/linux/memremap.h b/include/linux/memremap.h
> index 7b4899c06f49..b5e894133cf6 100644
> --- a/include/linux/memremap.h
> +++ b/include/linux/memremap.h
> @@ -106,6 +106,7 @@ typedef void (*dev_page_free_t)(struct page *page, void *data);
>   * @altmap: pre-allocated/reserved memory for vmemmap allocations
>   * @res: physical address range covered by @ref
>   * @ref: reference count that pins the devm_memremap_pages() mapping
> + * @kill: callback to transition @ref to the dead state
>   * @dev: host device of the mapping for debug
>   * @data: private data pointer for page_free()
>   * @type: memory type: see MEMORY_* in memory_hotplug.h
> @@ -117,13 +118,15 @@ struct dev_pagemap {
>  	bool altmap_valid;
>  	struct resource res;
>  	struct percpu_ref *ref;
> +	void (*kill)(struct percpu_ref *ref);
>  	struct device *dev;
>  	void *data;
>  	enum memory_type type;
>  };
>  
>  #ifdef CONFIG_ZONE_DEVICE
> -void *devm_memremap_pages(struct device *dev, struct dev_pagemap *pgmap);
> +void *devm_memremap_pages(struct device *dev, struct dev_pagemap *pgmap,
> +		void (*kill)(struct percpu_ref *));


It seems redundant to me to have the kill pointer both passed in as an
argument and passed in as part of pgmap... Why not just expect the user
to set it in the *pgmap that's passed in just like we expect ref to be
set ahead of time?

Another thought (that may be too forward looking) is to pass the
dev_pagemap struct to the kill function instead of the reference. That
way, if some future user wants to do something extra on kill they can
use container_of() to get extra context to work with.

Thanks,

Logan