On Fri, Jul 28, 2017 at 01:10:03PM +0200, Michal Hocko wrote: > I haven't seen a newer version posted but the same comment applies on > your hmm-v25-4.9 git version from > git://people.freedesktop.org/~glisse/linux > > On Wed 28-06-17 14:00:41, Jérôme Glisse wrote: > > This introduce a simple struct and associated helpers for device driver > > to use when hotpluging un-addressable device memory as ZONE_DEVICE. It > > will find a unuse physical address range and trigger memory hotplug for > > it which allocates and initialize struct page for the device memory. > > Please document the hotplug semantic some more please (who is in charge, > what is the lifetime, userspace API to add/remove this memory if any > etc...). > > I can see you call add_pages. Please document why arch_add_memory (like > devm_memremap_pages) is not used. You also never seem to online the > range which is in line with nvdim usage and it is OK. But then I fail to > understand why you need
I added documentation in function and in commit message: https://cgit.freedesktop.org/~glisse/linux/commit/?h=hmm-next&id=33e236a64da84423c83db401fc62ea13877111f2 Not much to say i am affraid as everything is under control of the device driver (when hotplug/hotremove happens, memory management, userspace API, ...). > > [...] > > + mem_hotplug_begin(); > > + ret = add_pages(nid, align_start >> PAGE_SHIFT, > > + align_size >> PAGE_SHIFT, false); > > + if (ret) { > > + mem_hotplug_done(); > > + goto error_add_memory; > > + } > > + move_pfn_range_to_zone(&NODE_DATA(nid)->node_zones[ZONE_DEVICE], > > + align_start >> PAGE_SHIFT, > > + align_size >> PAGE_SHIFT); > > + mem_hotplug_done(); > > + > > + for (pfn = devmem->pfn_first; pfn < devmem->pfn_last; pfn++) { > > + struct page *page = pfn_to_page(pfn); > > + > > + /* > > + * ZONE_DEVICE pages union ->lru with a ->pgmap back > > + * pointer. It is a bug if a ZONE_DEVICE page is ever > > + * freed or placed on a driver-private list. Therefore, > > + * seed the storage with LIST_POISON* values. > > + */ > > + list_del(&page->lru); > > this? The page is not on any list yet - it hasn't been added to the page > allocator. Like comments says it was to init page->lru.next|prev with poison values it is not important so i remove it. Jérôme
