[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




 


Rackspace

Lists.xenproject.org is hosted with RackSpace, monitoring our
servers 24x7x365 and backed by RackSpace's Fanatical Support®.