Re: [PATCH v5 3/3] xen: add helpers to allocate unpopulated memory

From: "Jürgen Groß" <jgross@suse.com>
To: "Roger Pau Monné" <roger.pau@citrix.com>, linux-kernel@vger.kernel.org
Cc: Oleksandr Andrushchenko <oleksandr_andrushchenko@epam.com>,
	David Airlie <airlied@linux.ie>, Daniel Vetter <daniel@ffwll.ch>,
	Boris Ostrovsky <boris.ostrovsky@oracle.com>,
	Stefano Stabellini <sstabellini@kernel.org>,
	Dan Carpenter <dan.carpenter@oracle.com>, Wei Liu <wl@xen.org>,
	Yan Yankovskyi <yyankovskyi@gmail.com>,
	dri-devel@lists.freedesktop.org, xen-devel@lists.xenproject.org,
	linux-mm@kvack.org, David Hildenbrand <david@redhat.com>,
	Michal Hocko <mhocko@kernel.org>,
	Dan Williams <dan.j.williams@intel.com>
Subject: Re: [PATCH v5 3/3] xen: add helpers to allocate unpopulated memory
Date: Tue, 1 Sep 2020 16:54:40 +0200	[thread overview]
Message-ID: <4b24c8c3-fb0f-23ad-09c5-81fcfdb6a2b5@suse.com> (raw)
In-Reply-To: <20200901144539.GI753@Air-de-Roger>

On 01.09.20 16:45, Roger Pau Monné wrote:
> On Tue, Sep 01, 2020 at 10:33:26AM +0200, Roger Pau Monne wrote:
>> +static int fill_list(unsigned int nr_pages)
>> +{
>> +	struct dev_pagemap *pgmap;
>> +	void *vaddr;
>> +	unsigned int i, alloc_pages = round_up(nr_pages, PAGES_PER_SECTION);
>> +	int nid, ret;
>> +
>> +	pgmap = kzalloc(sizeof(*pgmap), GFP_KERNEL);
>> +	if (!pgmap)
>> +		return -ENOMEM;
>> +
>> +	pgmap->type = MEMORY_DEVICE_GENERIC;
>> +	pgmap->res.name = "Xen scratch";
>> +	pgmap->res.flags = IORESOURCE_MEM | IORESOURCE_BUSY;
>> +
>> +	ret = allocate_resource(&iomem_resource, &pgmap->res,
>> +				alloc_pages * PAGE_SIZE, 0, -1,
>> +				PAGES_PER_SECTION * PAGE_SIZE, NULL, NULL);
>> +	if (ret < 0) {
>> +		pr_err("Cannot allocate new IOMEM resource\n");
>> +		kfree(pgmap);
>> +		return ret;
>> +	}
>> +
>> +	nid = memory_add_physaddr_to_nid(pgmap->res.start);
> 
> I think this is not needed ...
> 
>> +
>> +#ifdef CONFIG_XEN_HAVE_PVMMU
>> +        /*
>> +         * memremap will build page tables for the new memory so
>> +         * the p2m must contain invalid entries so the correct
>> +         * non-present PTEs will be written.
>> +         *
>> +         * If a failure occurs, the original (identity) p2m entries
>> +         * are not restored since this region is now known not to
>> +         * conflict with any devices.
>> +         */
>> +	if (!xen_feature(XENFEAT_auto_translated_physmap)) {
>> +		xen_pfn_t pfn = PFN_DOWN(pgmap->res.start);
>> +
>> +		for (i = 0; i < alloc_pages; i++) {
>> +			if (!set_phys_to_machine(pfn + i, INVALID_P2M_ENTRY)) {
>> +				pr_warn("set_phys_to_machine() failed, no memory added\n");
>> +				release_resource(&pgmap->res);
>> +				kfree(pgmap);
>> +				return -ENOMEM;
>> +			}
>> +                }
>> +	}
>> +#endif
>> +
>> +	vaddr = memremap_pages(pgmap, nid);
> 
> ... and NUMA_NO_NODE should be used here instead, as this memory is just
> fictitious space to map foreign memory, and shouldn't be related to
> any NUMA node.
> 
> The following chunk should be folded in, or I can resend.

I can fold it in.

Juergen