[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: [RFC PATCH 2/2] xen/grant-table: Use unpopulated DMAable pages instead of real RAM ones
On 15.06.22 03:51, Stefano Stabellini wrote:
Hello Stefano
On Tue, 14 Jun 2022, Oleksandr wrote:
On 11.06.22 02:55, Stefano Stabellini wrote:
Hello Stefano
On Thu, 9 Jun 2022, Oleksandr wrote:
On 04.06.22 00:19, Stefano Stabellini wrote:
Hello Stefano
Thank you for having a look and sorry for the late response.
On Tue, 17 May 2022, Oleksandr Tyshchenko wrote:
From: Oleksandr Tyshchenko <oleksandr_tyshchenko@xxxxxxxx>
Depends on CONFIG_XEN_UNPOPULATED_ALLOC. If enabled then unpopulated
DMAable (contiguous) pages will be allocated for grant mapping into
instead of ballooning out real RAM pages.
TODO: Fallback to real RAM pages if xen_alloc_unpopulated_dma_pages()
fails.
Signed-off-by: Oleksandr Tyshchenko <oleksandr_tyshchenko@xxxxxxxx>
---
drivers/xen/grant-table.c | 27 +++++++++++++++++++++++++++
1 file changed, 27 insertions(+)
diff --git a/drivers/xen/grant-table.c b/drivers/xen/grant-table.c
index 8ccccac..2bb4392 100644
--- a/drivers/xen/grant-table.c
+++ b/drivers/xen/grant-table.c
@@ -864,6 +864,25 @@ EXPORT_SYMBOL_GPL(gnttab_free_pages);
*/
int gnttab_dma_alloc_pages(struct gnttab_dma_alloc_args *args)
{
+#ifdef CONFIG_XEN_UNPOPULATED_ALLOC
+ int ret;
This is an alternative implementation of the same function.
Currently, yes.
If we are
going to use #ifdef, then I would #ifdef the entire function, rather
than just the body. Otherwise within the function body we can use
IS_ENABLED.
Good point. Note, there is one missing thing in current patch which is
described in TODO.
"Fallback to real RAM pages if xen_alloc_unpopulated_dma_pages() fails."
So I
will likely use IS_ENABLED within the function body.
If CONFIG_XEN_UNPOPULATED_ALLOC is enabled then gnttab_dma_alloc_pages()
will
try to call xen_alloc_unpopulated_dma_pages() the first and if fails then
fallback to allocate RAM pages and balloon them out.
One moment is not entirely clear to me. If we use fallback in
gnttab_dma_alloc_pages() then we must use fallback in
gnttab_dma_free_pages()
as well, we cannot use xen_free_unpopulated_dma_pages() for real RAM
pages.
The question is how to pass this information to the
gnttab_dma_free_pages()?
The first idea which comes to mind is to add a flag to struct
gnttab_dma_alloc_args...
You can check if the page is within the mhp_range range or part of
iomem_resource? If not, you can free it as a normal page.
If we do this, then the fallback is better implemented in
unpopulated-alloc.c because that is the one that is aware about
page addresses.
I got your idea and agree this can work technically. Or if we finally decide
to use the second option (use "dma_pool" for all) in the first patch
"[RFC PATCH 1/2] xen/unpopulated-alloc: Introduce helpers for DMA allocations"
then we will likely be able to check whether a page in question
is within a "dma_pool" using gen_pool_has_addr().
I am still wondering, can we avoid the fallback implementation in
unpopulated-alloc.c.
Because for that purpose we will need to pull more code into
unpopulated-alloc.c (to be more precise, almost everything which
gnttab_dma_free_pages() already has except gnttab_pages_clear_private()) and
pass more arguments to xen_free_unpopulated_dma_pages(). Also I might mistake,
but having a fallback split between grant-table.c (to allocate RAM pages in
gnttab_dma_alloc_pages()) and unpopulated-alloc.c (to free RAM pages in
xen_free_unpopulated_dma_pages()) would look a bit weird.
I see two possible options for the fallback implementation in grant-table.c:
1. (less preferable) by introducing new flag in struct gnttab_dma_alloc_args
2. (more preferable) by guessing unpopulated (non real RAM) page using
is_zone_device_page(), etc
For example, with the second option the resulting code will look quite simple
(only build tested):
diff --git a/drivers/xen/grant-table.c b/drivers/xen/grant-table.c
index 738029d..3bda71f 100644
--- a/drivers/xen/grant-table.c
+++ b/drivers/xen/grant-table.c
@@ -1047,6 +1047,23 @@ int gnttab_dma_alloc_pages(struct gnttab_dma_alloc_args
*args)
size_t size;
int i, ret;
+ if (IS_ENABLED(CONFIG_XEN_UNPOPULATED_ALLOC)) {
+ ret = xen_alloc_unpopulated_dma_pages(args->dev,
args->nr_pages,
+ args->pages);
+ if (ret < 0)
+ goto fallback;
+
+ ret = gnttab_pages_set_private(args->nr_pages, args->pages);
+ if (ret < 0)
+ goto fail;
+
+ args->vaddr = page_to_virt(args->pages[0]);
+ args->dev_bus_addr = page_to_phys(args->pages[0]);
+
+ return ret;
+ }
+
+fallback:
size = args->nr_pages << PAGE_SHIFT;
if (args->coherent)
args->vaddr = dma_alloc_coherent(args->dev, size,
@@ -1103,6 +1120,12 @@ int gnttab_dma_free_pages(struct gnttab_dma_alloc_args
*args)
gnttab_pages_clear_private(args->nr_pages, args->pages);
+ if (IS_ENABLED(CONFIG_XEN_UNPOPULATED_ALLOC) &&
+ is_zone_device_page(args->pages[0])) {
+ xen_free_unpopulated_dma_pages(args->dev, args->nr_pages,
args->pages);
+ return 0;
+ }
+
for (i = 0; i < args->nr_pages; i++)
args->frames[i] = page_to_xen_pfn(args->pages[i]);
What do you think?
I have another idea. Why don't we introduce a function implemented in
drivers/xen/unpopulated-alloc.c called is_xen_unpopulated_page() and
call it from here? is_xen_unpopulated_page can be implemented by using
gen_pool_has_addr.
I like the idea, will do
--
Regards,
Oleksandr Tyshchenko
|
