* [PATCH v2 1/2] iommu/dma: Respect __GFP_DMA and __GFP_DMA32 in incoming GFP flags
@ 2017-07-04 13:55 Tomasz Figa
2017-07-04 13:55 ` [PATCH v2 2/2] iommu/dma: Use __GFP_NOWARN only for high-order allocations Tomasz Figa
2017-07-26 9:15 ` [PATCH v2 1/2] iommu/dma: Respect __GFP_DMA and __GFP_DMA32 in incoming GFP flags Joerg Roedel
0 siblings, 2 replies; 6+ messages in thread
From: Tomasz Figa @ 2017-07-04 13:55 UTC (permalink / raw)
To: iommu; +Cc: linux-kernel, Joerg Roedel, Robin Murphy, Tomasz Figa
Current implementation of __iommu_dma_alloc_pages() keeps adding
__GFP_HIGHMEM to GFP flags regardless of whether other zone flags are
already included in the incoming flags. If __GFP_DMA or __GFP_DMA32 is
set at the same time as __GFP_HIGHMEM, the allocation fails due to
invalid zone flag combination.
Fix this by checking for __GFP_DMA and __GFP_DMA32 in incoming GFP flags
and adding __GFP_HIGHMEM only if they are not present.
Signed-off-by: Tomasz Figa <tfiga@chromium.org>
---
drivers/iommu/dma-iommu.c | 10 ++++++++--
1 file changed, 8 insertions(+), 2 deletions(-)
Changes from v1:
- Update the comment as per Robin's suggestion.
diff --git a/drivers/iommu/dma-iommu.c b/drivers/iommu/dma-iommu.c
index 9d1cebe7f6cb..bf23989b5158 100644
--- a/drivers/iommu/dma-iommu.c
+++ b/drivers/iommu/dma-iommu.c
@@ -445,8 +445,14 @@ static struct page **__iommu_dma_alloc_pages(unsigned int count,
if (!pages)
return NULL;
- /* IOMMU can map any pages, so himem can also be used here */
- gfp |= __GFP_NOWARN | __GFP_HIGHMEM;
+ /*
+ * Unless we have some addressing limitation implied by GFP_DMA flags,
+ * assume the IOMMU can map all of RAM and we can allocate anywhere.
+ */
+ if (!(gfp & (__GFP_DMA | __GFP_DMA32)))
+ gfp |= __GFP_HIGHMEM;
+
+ gfp |= __GFP_NOWARN;
while (count) {
struct page *page = NULL;
--
2.13.2.725.g09c95d1e9-goog
^ permalink raw reply related [flat|nested] 6+ messages in thread
* [PATCH v2 2/2] iommu/dma: Use __GFP_NOWARN only for high-order allocations
2017-07-04 13:55 [PATCH v2 1/2] iommu/dma: Respect __GFP_DMA and __GFP_DMA32 in incoming GFP flags Tomasz Figa
@ 2017-07-04 13:55 ` Tomasz Figa
2017-07-26 9:24 ` Joerg Roedel
2017-07-26 9:15 ` [PATCH v2 1/2] iommu/dma: Respect __GFP_DMA and __GFP_DMA32 in incoming GFP flags Joerg Roedel
1 sibling, 1 reply; 6+ messages in thread
From: Tomasz Figa @ 2017-07-04 13:55 UTC (permalink / raw)
To: iommu; +Cc: linux-kernel, Joerg Roedel, Robin Murphy, Tomasz Figa
Memory allocation routines are expected to report allocation errors to
kernel log. However, current implementation of __iommu_dma_alloc_pages()
adds __GFP_NOWARN for all calls to alloc_pages(), which completely
disables any logging.
Fix it by adding __GFP_NOWARN only to high order allocation attempts,
which are not critical.
Signed-off-by: Tomasz Figa <tfiga@chromium.org>
Reviewed-by: Robin Murphy <robin.murphy@arm.com>
---
drivers/iommu/dma-iommu.c | 5 ++---
1 file changed, 2 insertions(+), 3 deletions(-)
Changes from v1:
- Fix typo in subject.
- Add Robin's Reviewed-by.
diff --git a/drivers/iommu/dma-iommu.c b/drivers/iommu/dma-iommu.c
index bf23989b5158..6ed8c8f941d8 100644
--- a/drivers/iommu/dma-iommu.c
+++ b/drivers/iommu/dma-iommu.c
@@ -433,6 +433,7 @@ static struct page **__iommu_dma_alloc_pages(unsigned int count,
{
struct page **pages;
unsigned int i = 0, array_size = count * sizeof(*pages);
+ const gfp_t high_order_gfp = __GFP_NOWARN | __GFP_NORETRY;
order_mask &= (2U << MAX_ORDER) - 1;
if (!order_mask)
@@ -452,8 +453,6 @@ static struct page **__iommu_dma_alloc_pages(unsigned int count,
if (!(gfp & (__GFP_DMA | __GFP_DMA32)))
gfp |= __GFP_HIGHMEM;
- gfp |= __GFP_NOWARN;
-
while (count) {
struct page *page = NULL;
unsigned int order_size;
@@ -469,7 +468,7 @@ static struct page **__iommu_dma_alloc_pages(unsigned int count,
order_size = 1U << order;
page = alloc_pages((order_mask - order_size) ?
- gfp | __GFP_NORETRY : gfp, order);
+ gfp | high_order_gfp : gfp, order);
if (!page)
continue;
if (!order)
--
2.13.2.725.g09c95d1e9-goog
^ permalink raw reply related [flat|nested] 6+ messages in thread
* Re: [PATCH v2 1/2] iommu/dma: Respect __GFP_DMA and __GFP_DMA32 in incoming GFP flags
2017-07-04 13:55 [PATCH v2 1/2] iommu/dma: Respect __GFP_DMA and __GFP_DMA32 in incoming GFP flags Tomasz Figa
2017-07-04 13:55 ` [PATCH v2 2/2] iommu/dma: Use __GFP_NOWARN only for high-order allocations Tomasz Figa
@ 2017-07-26 9:15 ` Joerg Roedel
2017-07-26 9:26 ` Tomasz Figa
1 sibling, 1 reply; 6+ messages in thread
From: Joerg Roedel @ 2017-07-26 9:15 UTC (permalink / raw)
To: Tomasz Figa; +Cc: iommu, linux-kernel, Robin Murphy
On Tue, Jul 04, 2017 at 10:55:55PM +0900, Tomasz Figa wrote:
> Current implementation of __iommu_dma_alloc_pages() keeps adding
> __GFP_HIGHMEM to GFP flags regardless of whether other zone flags are
> already included in the incoming flags. If __GFP_DMA or __GFP_DMA32 is
> set at the same time as __GFP_HIGHMEM, the allocation fails due to
> invalid zone flag combination.
>
> Fix this by checking for __GFP_DMA and __GFP_DMA32 in incoming GFP flags
> and adding __GFP_HIGHMEM only if they are not present.
>
> Signed-off-by: Tomasz Figa <tfiga@chromium.org>
Isn't it better to mask out __GFP_DMA and __GFP_DMA32 in the allocation
flags and only take them into account for iova allocation?
When the IOMMU re-maps the DMA to this memory it doesn't matter where it
is in system memory.
Joerg
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH v2 2/2] iommu/dma: Use __GFP_NOWARN only for high-order allocations
2017-07-04 13:55 ` [PATCH v2 2/2] iommu/dma: Use __GFP_NOWARN only for high-order allocations Tomasz Figa
@ 2017-07-26 9:24 ` Joerg Roedel
2017-07-26 9:29 ` Tomasz Figa
0 siblings, 1 reply; 6+ messages in thread
From: Joerg Roedel @ 2017-07-26 9:24 UTC (permalink / raw)
To: Tomasz Figa; +Cc: iommu, linux-kernel, Robin Murphy
On Tue, Jul 04, 2017 at 10:55:56PM +0900, Tomasz Figa wrote:
> diff --git a/drivers/iommu/dma-iommu.c b/drivers/iommu/dma-iommu.c
> index bf23989b5158..6ed8c8f941d8 100644
> --- a/drivers/iommu/dma-iommu.c
> +++ b/drivers/iommu/dma-iommu.c
> @@ -433,6 +433,7 @@ static struct page **__iommu_dma_alloc_pages(unsigned int count,
> {
> struct page **pages;
> unsigned int i = 0, array_size = count * sizeof(*pages);
> + const gfp_t high_order_gfp = __GFP_NOWARN | __GFP_NORETRY;
>
> order_mask &= (2U << MAX_ORDER) - 1;
> if (!order_mask)
> @@ -452,8 +453,6 @@ static struct page **__iommu_dma_alloc_pages(unsigned int count,
> if (!(gfp & (__GFP_DMA | __GFP_DMA32)))
> gfp |= __GFP_HIGHMEM;
>
> - gfp |= __GFP_NOWARN;
> -
Okay, so a warning should be there if allocation fails, independent of
what the allocation order is. So either this function prints a message
in case of allocation failure or we remove __GFP_NOWARN for all
allocation attempts.
Adding __GFP_NOWARN only makes sense when there is another fall-back in
case allocation fails anyway, which is not the case here.
> while (count) {
> struct page *page = NULL;
> unsigned int order_size;
> @@ -469,7 +468,7 @@ static struct page **__iommu_dma_alloc_pages(unsigned int count,
>
> order_size = 1U << order;
> page = alloc_pages((order_mask - order_size) ?
> - gfp | __GFP_NORETRY : gfp, order);
> + gfp | high_order_gfp : gfp, order);
Why does it specify __GFP_NORETRY at all? The alloc-routines in the
DMA-API don't need to be atomic.
Joerg
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH v2 1/2] iommu/dma: Respect __GFP_DMA and __GFP_DMA32 in incoming GFP flags
2017-07-26 9:15 ` [PATCH v2 1/2] iommu/dma: Respect __GFP_DMA and __GFP_DMA32 in incoming GFP flags Joerg Roedel
@ 2017-07-26 9:26 ` Tomasz Figa
0 siblings, 0 replies; 6+ messages in thread
From: Tomasz Figa @ 2017-07-26 9:26 UTC (permalink / raw)
To: Joerg Roedel; +Cc: open list:IOMMU DRIVERS, linux-kernel, Robin Murphy
Hi Joerg,
On Wed, Jul 26, 2017 at 6:15 PM, Joerg Roedel <joro@8bytes.org> wrote:
> On Tue, Jul 04, 2017 at 10:55:55PM +0900, Tomasz Figa wrote:
>> Current implementation of __iommu_dma_alloc_pages() keeps adding
>> __GFP_HIGHMEM to GFP flags regardless of whether other zone flags are
>> already included in the incoming flags. If __GFP_DMA or __GFP_DMA32 is
>> set at the same time as __GFP_HIGHMEM, the allocation fails due to
>> invalid zone flag combination.
>>
>> Fix this by checking for __GFP_DMA and __GFP_DMA32 in incoming GFP flags
>> and adding __GFP_HIGHMEM only if they are not present.
>>
>> Signed-off-by: Tomasz Figa <tfiga@chromium.org>
>
> Isn't it better to mask out __GFP_DMA and __GFP_DMA32 in the allocation
> flags and only take them into account for iova allocation?
>
> When the IOMMU re-maps the DMA to this memory it doesn't matter where it
> is in system memory.
It's a platform specific knowledge and I'd say the generic helper is
not where it should be decided. Please see my reply to Robin for v1
[1].
[1] https://patchwork.kernel.org/patch/9810921/
Best regards.
Tomasz
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH v2 2/2] iommu/dma: Use __GFP_NOWARN only for high-order allocations
2017-07-26 9:24 ` Joerg Roedel
@ 2017-07-26 9:29 ` Tomasz Figa
0 siblings, 0 replies; 6+ messages in thread
From: Tomasz Figa @ 2017-07-26 9:29 UTC (permalink / raw)
To: Joerg Roedel; +Cc: open list:IOMMU DRIVERS, linux-kernel, Robin Murphy
Hi Joerg,
On Wed, Jul 26, 2017 at 6:24 PM, Joerg Roedel <joro@8bytes.org> wrote:
> On Tue, Jul 04, 2017 at 10:55:56PM +0900, Tomasz Figa wrote:
>> diff --git a/drivers/iommu/dma-iommu.c b/drivers/iommu/dma-iommu.c
>> index bf23989b5158..6ed8c8f941d8 100644
>> --- a/drivers/iommu/dma-iommu.c
>> +++ b/drivers/iommu/dma-iommu.c
>> @@ -433,6 +433,7 @@ static struct page **__iommu_dma_alloc_pages(unsigned int count,
>> {
>> struct page **pages;
>> unsigned int i = 0, array_size = count * sizeof(*pages);
>> + const gfp_t high_order_gfp = __GFP_NOWARN | __GFP_NORETRY;
>>
>> order_mask &= (2U << MAX_ORDER) - 1;
>> if (!order_mask)
>> @@ -452,8 +453,6 @@ static struct page **__iommu_dma_alloc_pages(unsigned int count,
>> if (!(gfp & (__GFP_DMA | __GFP_DMA32)))
>> gfp |= __GFP_HIGHMEM;
>>
>> - gfp |= __GFP_NOWARN;
>> -
>
> Okay, so a warning should be there if allocation fails, independent of
> what the allocation order is.
The allocation fails only if the order drops to the lowest possible
fallback order.
> So either this function prints a message
> in case of allocation failure or we remove __GFP_NOWARN for all
> allocation attempts.
>
> Adding __GFP_NOWARN only makes sense when there is another fall-back in
> case allocation fails anyway, which is not the case here.
There is fall back here. The loop tries allocating with higher order
and then falls back to a lower order if it fails and so on, until the
lowest acceptable order is reached.
>
>> while (count) {
>> struct page *page = NULL;
>> unsigned int order_size;
>> @@ -469,7 +468,7 @@ static struct page **__iommu_dma_alloc_pages(unsigned int count,
>>
>> order_size = 1U << order;
>> page = alloc_pages((order_mask - order_size) ?
>> - gfp | __GFP_NORETRY : gfp, order);
>> + gfp | high_order_gfp : gfp, order);
>
> Why does it specify __GFP_NORETRY at all? The alloc-routines in the
> DMA-API don't need to be atomic.
This doesn't have anything to do with being atomic. __GFP_NORETRY here
is to avoid the page allocator retrying indefinitely and actually
triggering OOM for high order allocation, if we can safely fall back
to lower order.
Best regards,
Tomasz
^ permalink raw reply [flat|nested] 6+ messages in thread
end of thread, other threads:[~2017-07-26 9:30 UTC | newest]
Thread overview: 6+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2017-07-04 13:55 [PATCH v2 1/2] iommu/dma: Respect __GFP_DMA and __GFP_DMA32 in incoming GFP flags Tomasz Figa
2017-07-04 13:55 ` [PATCH v2 2/2] iommu/dma: Use __GFP_NOWARN only for high-order allocations Tomasz Figa
2017-07-26 9:24 ` Joerg Roedel
2017-07-26 9:29 ` Tomasz Figa
2017-07-26 9:15 ` [PATCH v2 1/2] iommu/dma: Respect __GFP_DMA and __GFP_DMA32 in incoming GFP flags Joerg Roedel
2017-07-26 9:26 ` Tomasz Figa
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).