* [PATCH v2 1/2] iommu/dma: Respect __GFP_DMA and __GFP_DMA32 in incoming GFP flags @ 2017-07-04 13:55 ` Tomasz Figa 0 siblings, 0 replies; 10+ messages in thread From: Tomasz Figa @ 2017-07-04 13:55 UTC (permalink / raw) To: iommu-cunTk1MwBs9QetFLy7KEm3xJsTq8ys+cHZ5vskTnxNA Cc: linux-kernel-u79uwXL29TY76Z2rM5mHXA 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-F7+t8E8rja9g9hUCZPvPmw@public.gmane.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] 10+ messages in thread
* [PATCH v2 1/2] iommu/dma: Respect __GFP_DMA and __GFP_DMA32 in incoming GFP flags @ 2017-07-04 13:55 ` Tomasz Figa 0 siblings, 0 replies; 10+ 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] 10+ messages in thread
[parent not found: <20170704135556.21704-1-tfiga-F7+t8E8rja9g9hUCZPvPmw@public.gmane.org>]
* [PATCH v2 2/2] iommu/dma: Use __GFP_NOWARN only for high-order allocations 2017-07-04 13:55 ` Tomasz Figa @ 2017-07-04 13:55 ` Tomasz Figa -1 siblings, 0 replies; 10+ messages in thread From: Tomasz Figa @ 2017-07-04 13:55 UTC (permalink / raw) To: iommu-cunTk1MwBs9QetFLy7KEm3xJsTq8ys+cHZ5vskTnxNA Cc: linux-kernel-u79uwXL29TY76Z2rM5mHXA 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-F7+t8E8rja9g9hUCZPvPmw@public.gmane.org> Reviewed-by: Robin Murphy <robin.murphy-5wv7dgnIgG8@public.gmane.org> --- 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] 10+ messages in thread
* [PATCH v2 2/2] iommu/dma: Use __GFP_NOWARN only for high-order allocations @ 2017-07-04 13:55 ` Tomasz Figa 0 siblings, 0 replies; 10+ 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] 10+ messages in thread
* Re: [PATCH v2 2/2] iommu/dma: Use __GFP_NOWARN only for high-order allocations 2017-07-04 13:55 ` Tomasz Figa (?) @ 2017-07-26 9:24 ` Joerg Roedel [not found] ` <20170726092411.GD15833-zLv9SwRftAIdnm+yROfE0A@public.gmane.org> -1 siblings, 1 reply; 10+ 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] 10+ messages in thread
[parent not found: <20170726092411.GD15833-zLv9SwRftAIdnm+yROfE0A@public.gmane.org>]
* 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; 10+ messages in thread From: Tomasz Figa @ 2017-07-26 9:29 UTC (permalink / raw) To: Joerg Roedel Cc: open list:IOMMU DRIVERS, linux-kernel-u79uwXL29TY76Z2rM5mHXA@public.gmane.org Hi Joerg, On Wed, Jul 26, 2017 at 6:24 PM, Joerg Roedel <joro-zLv9SwRftAIdnm+yROfE0A@public.gmane.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] 10+ messages in thread
* Re: [PATCH v2 2/2] iommu/dma: Use __GFP_NOWARN only for high-order allocations @ 2017-07-26 9:29 ` Tomasz Figa 0 siblings, 0 replies; 10+ messages in thread From: Tomasz Figa @ 2017-07-26 9:29 UTC (permalink / raw) To: Joerg Roedel Cc: open list:IOMMU DRIVERS, linux-kernel@vger.kernel.org, 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] 10+ 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 ` Tomasz Figa (?) (?) @ 2017-07-26 9:15 ` Joerg Roedel [not found] ` <20170726091545.GC15833-zLv9SwRftAIdnm+yROfE0A@public.gmane.org> -1 siblings, 1 reply; 10+ 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] 10+ messages in thread
[parent not found: <20170726091545.GC15833-zLv9SwRftAIdnm+yROfE0A@public.gmane.org>]
* 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; 10+ messages in thread From: Tomasz Figa @ 2017-07-26 9:26 UTC (permalink / raw) To: Joerg Roedel Cc: open list:IOMMU DRIVERS, linux-kernel-u79uwXL29TY76Z2rM5mHXA@public.gmane.org Hi Joerg, On Wed, Jul 26, 2017 at 6:15 PM, Joerg Roedel <joro-zLv9SwRftAIdnm+yROfE0A@public.gmane.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-F7+t8E8rja9g9hUCZPvPmw@public.gmane.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] 10+ messages in thread
* Re: [PATCH v2 1/2] iommu/dma: Respect __GFP_DMA and __GFP_DMA32 in incoming GFP flags @ 2017-07-26 9:26 ` Tomasz Figa 0 siblings, 0 replies; 10+ messages in thread From: Tomasz Figa @ 2017-07-26 9:26 UTC (permalink / raw) To: Joerg Roedel Cc: open list:IOMMU DRIVERS, linux-kernel@vger.kernel.org, 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] 10+ messages in thread
end of thread, other threads:[~2017-07-26 9:30 UTC | newest]
Thread overview: 10+ 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 ` Tomasz Figa
[not found] ` <20170704135556.21704-1-tfiga-F7+t8E8rja9g9hUCZPvPmw@public.gmane.org>
2017-07-04 13:55 ` [PATCH v2 2/2] iommu/dma: Use __GFP_NOWARN only for high-order allocations Tomasz Figa
2017-07-04 13:55 ` Tomasz Figa
2017-07-26 9:24 ` Joerg Roedel
[not found] ` <20170726092411.GD15833-zLv9SwRftAIdnm+yROfE0A@public.gmane.org>
2017-07-26 9:29 ` Tomasz Figa
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
[not found] ` <20170726091545.GC15833-zLv9SwRftAIdnm+yROfE0A@public.gmane.org>
2017-07-26 9:26 ` Tomasz Figa
2017-07-26 9:26 ` Tomasz Figa
This is an external index of several public inboxes, see mirroring instructions on how to clone and mirror all data and code used by this external index.