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

[Dan Williams]

On Wed, May 23, 2018 at 8:47 AM, Logan Gunthorpe <logang@xxxxxxxxxxxx> wrote:
>
>
> 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?

I did this for grep-ability. Now you can grep for all
devm_memremap_pages and see the associated teardown actions,
everything else in pgmap is data. I'm not opposed to just requiring it
to be passed in with the pgmap, but I thought removing a step for
someone trying to grep through the code flow was worth it. Yes, not
the strongest argument, so if folks feel it adds too much clutter we
can switch it.

> 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.

We can cross that bridge if it comes to it, but as it stands being
able to get the container of the reference count seems to be enough
for all users.