From: Oleksandr <olekstysh@gmail.com>
To: Stefano Stabellini <sstabellini@kernel.org>
Cc: xen-devel@lists.xenproject.org, linux-kernel@vger.kernel.org,
Oleksandr Tyshchenko <oleksandr_tyshchenko@epam.com>,
Boris Ostrovsky <boris.ostrovsky@oracle.com>,
Juergen Gross <jgross@suse.com>, Julien Grall <julien@xen.org>
Subject: Re: [RFC PATCH 2/2] xen/grant-table: Use unpopulated DMAable pages instead of real RAM ones
Date: Wed, 15 Jun 2022 19:40:04 +0300 [thread overview]
Message-ID: <9f1e4568-1cfd-e967-e54c-735bad1ea211@gmail.com> (raw)
In-Reply-To: <alpine.DEB.2.22.394.2206141748150.1837490@ubuntu-linux-20-04-desktop>
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
prev parent reply other threads:[~2022-06-15 16:40 UTC|newest]
Thread overview: 16+ messages / expand[flat|nested] mbox.gz Atom feed top
2022-05-17 18:04 [RFC PATCH 0/2] Ability to allocate DMAable pages using unpopulated-alloc Oleksandr Tyshchenko
2022-05-17 18:04 ` [RFC PATCH 1/2] xen/unpopulated-alloc: Introduce helpers for DMA allocations Oleksandr Tyshchenko
2022-06-03 21:52 ` Stefano Stabellini
2022-06-08 18:12 ` Oleksandr
2022-06-11 0:12 ` Stefano Stabellini
2022-06-14 17:37 ` Oleksandr
2022-06-15 0:45 ` Stefano Stabellini
2022-06-15 16:56 ` Oleksandr
2022-06-16 8:49 ` Roger Pau Monné
2022-05-17 18:04 ` [RFC PATCH 2/2] xen/grant-table: Use unpopulated DMAable pages instead of real RAM ones Oleksandr Tyshchenko
2022-06-03 21:19 ` Stefano Stabellini
2022-06-09 18:39 ` Oleksandr
2022-06-10 23:55 ` Stefano Stabellini
2022-06-14 17:53 ` Oleksandr
2022-06-15 0:51 ` Stefano Stabellini
2022-06-15 16:40 ` Oleksandr [this message]
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=9f1e4568-1cfd-e967-e54c-735bad1ea211@gmail.com \
--to=olekstysh@gmail.com \
--cc=boris.ostrovsky@oracle.com \
--cc=jgross@suse.com \
--cc=julien@xen.org \
--cc=linux-kernel@vger.kernel.org \
--cc=oleksandr_tyshchenko@epam.com \
--cc=sstabellini@kernel.org \
--cc=xen-devel@lists.xenproject.org \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).