On Wed, 8 Jun 2022, Oleksandr wrote:
2. Drop the "page_list" entirely and use "dma_pool" for all (contiguous andThe second option (use "dma_pool" for all) looks great, thank you for
non-contiguous) allocations. After all, all pages are initially contiguous in
fill_list() as they are built from the resource. This changes behavior for all
users of xen_alloc_unpopulated_pages()
Below the diff for unpopulated-alloc.c. The patch is also available at:
https://github.com/otyshchenko1/linux/commit/7be569f113a4acbdc4bcb9b20cb3995b3151387a
diff --git a/drivers/xen/unpopulated-alloc.c b/drivers/xen/unpopulated-alloc.c
index a39f2d3..ab5c7bd 100644
--- a/drivers/xen/unpopulated-alloc.c
+++ b/drivers/xen/unpopulated-alloc.c
@@ -1,5 +1,7 @@
// SPDX-License-Identifier: GPL-2.0
+#include <linux/dma-mapping.h>
#include <linux/errno.h>
+#include <linux/genalloc.h>
#include <linux/gfp.h>
#include <linux/kernel.h>
#include <linux/mm.h>
@@ -13,8 +15,8 @@
#include <xen/xen.h>
static DEFINE_MUTEX(list_lock);
-static struct page *page_list;
-static unsigned int list_count;
+
+static struct gen_pool *dma_pool;
static struct resource *target_resource;
@@ -36,7 +38,7 @@ static int fill_list(unsigned int nr_pages)
struct dev_pagemap *pgmap;
struct resource *res, *tmp_res = NULL;
void *vaddr;
- unsigned int i, alloc_pages = round_up(nr_pages, PAGES_PER_SECTION);
+ unsigned int alloc_pages = round_up(nr_pages, PAGES_PER_SECTION);
struct range mhp_range;
int ret;
@@ -106,6 +108,7 @@ static int fill_list(unsigned int nr_pages)
* conflict with any devices.
*/
if (!xen_feature(XENFEAT_auto_translated_physmap)) {
+ unsigned int i;
xen_pfn_t pfn = PFN_DOWN(res->start);
for (i = 0; i < alloc_pages; i++) {
@@ -125,16 +128,17 @@ static int fill_list(unsigned int nr_pages)
goto err_memremap;
}
- for (i = 0; i < alloc_pages; i++) {
- struct page *pg = virt_to_page(vaddr + PAGE_SIZE * i);
-
- pg->zone_device_data = page_list;
- page_list = pg;
- list_count++;
+ ret = gen_pool_add_virt(dma_pool, (unsigned long)vaddr, res->start,
+ alloc_pages * PAGE_SIZE, NUMA_NO_NODE);
+ if (ret) {
+ pr_err("Cannot add memory range to the pool\n");
+ goto err_pool;
}
return 0;
+err_pool:
+ memunmap_pages(pgmap);
err_memremap:
kfree(pgmap);
err_pgmap:
@@ -149,51 +153,49 @@ static int fill_list(unsigned int nr_pages)
return ret;
}
-/**
- * xen_alloc_unpopulated_pages - alloc unpopulated pages
- * @nr_pages: Number of pages
- * @pages: pages returned
- * @return 0 on success, error otherwise
- */
-int xen_alloc_unpopulated_pages(unsigned int nr_pages, struct page **pages)
+static int alloc_unpopulated_pages(unsigned int nr_pages, struct page
**pages,
+ bool contiguous)
{
unsigned int i;
int ret = 0;
+ void *vaddr;
+ bool filled = false;
/*
* Fallback to default behavior if we do not have any suitable
resource
* to allocate required region from and as the result we won't be able
to
* construct pages.
*/
- if (!target_resource)
+ if (!target_resource) {
+ if (contiguous)
+ return -ENODEV;
+
return xen_alloc_ballooned_pages(nr_pages, pages);
+ }
mutex_lock(&list_lock);
- if (list_count < nr_pages) {
- ret = fill_list(nr_pages - list_count);
+
+ while (!(vaddr = (void *)gen_pool_alloc(dma_pool, nr_pages *
PAGE_SIZE))) {
+ if (filled)
+ ret = -ENOMEM;
+ else {
+ ret = fill_list(nr_pages);
+ filled = true;
+ }
if (ret)
goto out;
}
for (i = 0; i < nr_pages; i++) {
- struct page *pg = page_list;
-
- BUG_ON(!pg);
- page_list = pg->zone_device_data;
- list_count--;
- pages[i] = pg;
+ pages[i] = virt_to_page(vaddr + PAGE_SIZE * i);
#ifdef CONFIG_XEN_HAVE_PVMMU
if (!xen_feature(XENFEAT_auto_translated_physmap)) {
- ret = xen_alloc_p2m_entry(page_to_pfn(pg));
+ ret = xen_alloc_p2m_entry(page_to_pfn(pages[i]));
if (ret < 0) {
- unsigned int j;
-
- for (j = 0; j <= i; j++) {
- pages[j]->zone_device_data = page_list;
- page_list = pages[j];
- list_count++;
- }
+ /* XXX Do we need to zeroed pages[i]? */
+ gen_pool_free(dma_pool, (unsigned long)vaddr,
+ nr_pages * PAGE_SIZE);
goto out;
}
}
@@ -204,32 +206,89 @@ int xen_alloc_unpopulated_pages(unsigned int nr_pages,
struct page **pages)
mutex_unlock(&list_lock);
return ret;
}
-EXPORT_SYMBOL(xen_alloc_unpopulated_pages);
-/**
- * xen_free_unpopulated_pages - return unpopulated pages
- * @nr_pages: Number of pages
- * @pages: pages to return
- */
-void xen_free_unpopulated_pages(unsigned int nr_pages, struct page **pages)
+static void free_unpopulated_pages(unsigned int nr_pages, struct page
**pages,
+ bool contiguous)
{
- unsigned int i;
-
if (!target_resource) {
+ if (contiguous)
+ return;
+
xen_free_ballooned_pages(nr_pages, pages);
return;
}
mutex_lock(&list_lock);
- for (i = 0; i < nr_pages; i++) {
- pages[i]->zone_device_data = page_list;
- page_list = pages[i];
- list_count++;
+
+ /* XXX Do we need to check the range (gen_pool_has_addr)? */
+ if (contiguous)
+ gen_pool_free(dma_pool, (unsigned long)page_to_virt(pages[0]),
+ nr_pages * PAGE_SIZE);
+ else {
+ unsigned int i;
+
+ for (i = 0; i < nr_pages; i++)
+ gen_pool_free(dma_pool, (unsigned
long)page_to_virt(pages[i]),
+ PAGE_SIZE);
}
+
mutex_unlock(&list_lock);
}
+
+/**
+ * xen_alloc_unpopulated_pages - alloc unpopulated pages
+ * @nr_pages: Number of pages
+ * @pages: pages returned
+ * @return 0 on success, error otherwise
+ */
+int xen_alloc_unpopulated_pages(unsigned int nr_pages, struct page **pages)
+{
+ return alloc_unpopulated_pages(nr_pages, pages, false);
+}
+EXPORT_SYMBOL(xen_alloc_unpopulated_pages);
+
+/**
+ * xen_free_unpopulated_pages - return unpopulated pages
+ * @nr_pages: Number of pages
+ * @pages: pages to return
+ */
+void xen_free_unpopulated_pages(unsigned int nr_pages, struct page **pages)
+{
+ free_unpopulated_pages(nr_pages, pages, false);
+}
EXPORT_SYMBOL(xen_free_unpopulated_pages);
+/**
+ * xen_alloc_unpopulated_dma_pages - alloc unpopulated DMAable pages
+ * @dev: valid struct device pointer
+ * @nr_pages: Number of pages
+ * @pages: pages returned
+ * @return 0 on success, error otherwise
+ */
+int xen_alloc_unpopulated_dma_pages(struct device *dev, unsigned int
nr_pages,
+ struct page **pages)
+{
+ /* XXX Handle devices which support 64-bit DMA address only for now */
+ if (dma_get_mask(dev) != DMA_BIT_MASK(64))
+ return -EINVAL;
+
+ return alloc_unpopulated_pages(nr_pages, pages, true);
+}
+EXPORT_SYMBOL(xen_alloc_unpopulated_dma_pages);
+
+/**
+ * xen_free_unpopulated_dma_pages - return unpopulated DMAable pages
+ * @dev: valid struct device pointer
+ * @nr_pages: Number of pages
+ * @pages: pages to return
+ */
+void xen_free_unpopulated_dma_pages(struct device *dev, unsigned int
nr_pages,
+ struct page **pages)
+{
+ free_unpopulated_pages(nr_pages, pages, true);
+}
+EXPORT_SYMBOL(xen_free_unpopulated_dma_pages);
+
static int __init unpopulated_init(void)
{
int ret;
@@ -237,9 +296,19 @@ static int __init unpopulated_init(void)
if (!xen_domain())
return -ENODEV;
+ dma_pool = gen_pool_create(PAGE_SHIFT, NUMA_NO_NODE);
+ if (!dma_pool) {
+ pr_err("xen:unpopulated: Cannot create DMA pool\n");
+ return -ENOMEM;
+ }
+
+ gen_pool_set_algo(dma_pool, gen_pool_best_fit, NULL);
+
ret = arch_xen_unpopulated_init(&target_resource);
if (ret) {
pr_err("xen:unpopulated: Cannot initialize target
resource\n");
+ gen_pool_destroy(dma_pool);
+ dma_pool = NULL;
target_resource = NULL;
}
[snip]
I think, regarding on the approach we would likely need to do some renaming
for fill_list, page_list, list_lock, etc.
Both options work in my Arm64 based environment, not sure regarding x86.
Or do we have another option here?
I would be happy to go any route. What do you think?
looking into it!