Messages in this thread | | | Subject | Re: [RFC PATCH 2/2] xen/grant-table: Use unpopulated DMAable pages instead of real RAM ones | From | Oleksandr <> | Date | Wed, 15 Jun 2022 19:40:04 +0300 |
| |
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@epam.com> >>>>>> >>>>>> 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@epam.com> >>>>>> --- >>>>>> 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
| |