Re: [HMM v13 02/18] mm/ZONE_DEVICE/unaddressable: add support for un-addressable device memory

[Jerome Glisse]

On Mon, Nov 21, 2016 at 01:36:57PM +0530, Anshuman Khandual wrote:
> On 11/18/2016 11:48 PM, Jérôme Glisse wrote:
> > This add support for un-addressable device memory. Such memory is hotpluged
> > only so we can have struct page but should never be map. This patch add code
> 
> struct pages inside the system RAM range unlike the vmem_altmap scheme
> where the struct pages can be inside the device memory itself. This
> possibility does not arise for un addressable device memory. May be we
> will have to block the paths where vmem_altmap is requested along with
> un addressable device memory.

I did not think checking for that explicitly was necessary, sounded like shooting
yourself in the foot and that it would be obvious :)

[...]

> > diff --git a/include/linux/memremap.h b/include/linux/memremap.h
> > index 9341619..fe61dca 100644
> > --- a/include/linux/memremap.h
> > +++ b/include/linux/memremap.h
> > @@ -41,22 +41,34 @@ static inline struct vmem_altmap *to_vmem_altmap(unsigned long memmap_start)
> >   * @res: physical address range covered by @ref
> >   * @ref: reference count that pins the devm_memremap_pages() mapping
> >   * @dev: host device of the mapping for debug
> > + * @flags: memory flags (look for MEMORY_FLAGS_NONE in memory_hotplug.h)
> 
> ^^^^^^^^^^^^^ device memory flags instead ?

Well maybe it will be use for something else than device memory in the future
but yes for now it is only device memory so i can rename it.

> >   */
> >  struct dev_pagemap {
> >  	struct vmem_altmap *altmap;
> >  	const struct resource *res;
> >  	struct percpu_ref *ref;
> >  	struct device *dev;
> > +	int flags;
> >  };
> >  
> >  #ifdef CONFIG_ZONE_DEVICE
> >  void *devm_memremap_pages(struct device *dev, struct resource *res,
> > -		struct percpu_ref *ref, struct vmem_altmap *altmap);
> > +			  struct percpu_ref *ref, struct vmem_altmap *altmap,
> > +			  struct dev_pagemap **ppgmap, int flags);
> >  struct dev_pagemap *find_dev_pagemap(resource_size_t phys);
> > +
> > +static inline bool is_addressable_page(const struct page *page)
> > +{
> > +	return ((page_zonenum(page) != ZONE_DEVICE) ||
> > +		!(page->pgmap->flags & MEMORY_UNADDRESSABLE));
> > +}
> >  #else
> >  static inline void *devm_memremap_pages(struct device *dev,
> > -		struct resource *res, struct percpu_ref *ref,
> > -		struct vmem_altmap *altmap)
> > +					struct resource *res,
> > +					struct percpu_ref *ref,
> > +					struct vmem_altmap *altmap,
> > +					struct dev_pagemap **ppgmap,
> > +					int flags)
> 
> 
> As I had mentioned before devm_memremap_pages() should be changed not
> to accept a valid altmap along with request for un-addressable memory.

If you fear such case yes sure.


[...]

> > diff --git a/kernel/memremap.c b/kernel/memremap.c
> > index 07665eb..438a73aa2 100644
> > --- a/kernel/memremap.c
> > +++ b/kernel/memremap.c
> > @@ -246,7 +246,7 @@ static void devm_memremap_pages_release(struct device *dev, void *data)
> >  	/* pages are dead and unused, undo the arch mapping */
> >  	align_start = res->start & ~(SECTION_SIZE - 1);
> >  	align_size = ALIGN(resource_size(res), SECTION_SIZE);
> > -	arch_remove_memory(align_start, align_size, MEMORY_DEVICE);
> > +	arch_remove_memory(align_start, align_size, pgmap->flags);
> >  	untrack_pfn(NULL, PHYS_PFN(align_start), align_size);
> >  	pgmap_radix_release(res);
> >  	dev_WARN_ONCE(dev, pgmap->altmap && pgmap->altmap->alloc,
> > @@ -270,6 +270,8 @@ struct dev_pagemap *find_dev_pagemap(resource_size_t phys)
> >   * @res: "host memory" address range
> >   * @ref: a live per-cpu reference count
> >   * @altmap: optional descriptor for allocating the memmap from @res
> > + * @ppgmap: pointer set to new page dev_pagemap on success
> > + * @flags: flag for memory (look for MEMORY_FLAGS_NONE in memory_hotplug.h)
> >   *
> >   * Notes:
> >   * 1/ @ref must be 'live' on entry and 'dead' before devm_memunmap_pages() time
> > @@ -280,7 +282,8 @@ struct dev_pagemap *find_dev_pagemap(resource_size_t phys)
> >   *    this is not enforced.
> >   */
> >  void *devm_memremap_pages(struct device *dev, struct resource *res,
> > -		struct percpu_ref *ref, struct vmem_altmap *altmap)
> > +			  struct percpu_ref *ref, struct vmem_altmap *altmap,
> > +			  struct dev_pagemap **ppgmap, int flags)
> >  {
> >  	resource_size_t key, align_start, align_size, align_end;
> >  	pgprot_t pgprot = PAGE_KERNEL;
> > @@ -322,6 +325,7 @@ void *devm_memremap_pages(struct device *dev, struct resource *res,
> >  	}
> >  	pgmap->ref = ref;
> >  	pgmap->res = &page_map->res;
> > +	pgmap->flags = flags | MEMORY_DEVICE;
> 
> So the caller of devm_memremap_pages() should not have give out MEMORY_DEVICE
> in the flag it passed on to this function ? Hmm, else we should just check
> that the flags contains all appropriate bits before proceeding.

Here i was just trying to be on the safe side, yes caller should already have set
the flag but this function is only use for device memory so it did not seem like
it would hurt to be extra safe. I can add a BUG_ON() but it seems people have mix
feeling about BUG_ON()

Cheers,
Jérôme