[PATCH 1/2] arm64: Workaround renaming of __GFP_WAIT

[PATCH 1/2] arm64: Workaround renaming of __GFP_WAIT

[Robin Murphy]

The imminent renaming of __GFP_WAIT in the mm tree conflicts with its
use in the new IOMMU DMA ops; introduce a temporary local version of
its replacement to smooth over the transition.

This patch should be reverted at 4.4-rc1.

CC: Mel Gorman <mgorman@techsingularity.net>
CC: Andrew Morton <akpm@linux-foundation.org>
Reported-by: Sudeep Holla <sudeep.holla@arm.com>
Signed-off-by: Robin Murphy <robin.murphy@arm.com>
---

Sudeep points out that there are pending changes in -next touching arm64
which I hadn't spotted, which end up breaking the build when merged with
my changes in the IOMMU tree. Catalin, would you mind acking these fixes
so that Joerg can carry them? We should be able to send the revert through
arm64 once the dust has settled.

Thanks,
Robin.

---
 arch/arm64/mm/dma-mapping.c | 4 ++++
 1 file changed, 4 insertions(+)

diff --git a/arch/arm64/mm/dma-mapping.c b/arch/arm64/mm/dma-mapping.c
index 6320361..66444df 100644
--- a/arch/arm64/mm/dma-mapping.c
+++ b/arch/arm64/mm/dma-mapping.c
@@ -546,6 +546,10 @@ static void flush_page(struct device *dev, const void *virt, phys_addr_t phys)
 	__dma_flush_range(virt, virt + PAGE_SIZE);
 }
 
+#ifdef __GFP_WAIT
+#define gfpflags_allow_blocking(gfp) ((gfp) & __GFP_WAIT)
+#endif
+
 static void *__iommu_alloc_attrs(struct device *dev, size_t size,
 				 dma_addr_t *handle, gfp_t gfp,
 				 struct dma_attrs *attrs)
-- 
1.9.1

[PATCH 2/2] arm64: Use gfpflags_allow_blocking()

[Robin Murphy]

__GFP_WAIT is going away to live its life under a new identity; convert
__iommu_alloc_attrs() to the new helper function instead.

CC: Mel Gorman <mgorman@techsingularity.net>
CC: Andrew Morton <akpm@linux-foundation.org>
Reported-by: Sudeep Holla <sudeep.holla@arm.com>
Signed-off-by: Robin Murphy <robin.murphy@arm.com>
---
 arch/arm64/mm/dma-mapping.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/arch/arm64/mm/dma-mapping.c b/arch/arm64/mm/dma-mapping.c
index 66444df..40cd4af 100644
--- a/arch/arm64/mm/dma-mapping.c
+++ b/arch/arm64/mm/dma-mapping.c
@@ -566,7 +566,7 @@ static void *__iommu_alloc_attrs(struct device *dev, size_t size,
 	 */
 	gfp |= __GFP_ZERO;
 
-	if (gfp & __GFP_WAIT) {
+	if (gfpflags_allow_blocking(gfp)) {
 		struct page **pages;
 		pgprot_t prot = __get_dma_pgprot(attrs, PAGE_KERNEL, coherent);
 
-- 
1.9.1

[PATCH 2/2] arm64: Use gfpflags_allow_blocking()

[Catalin Marinas]

On Fri, Oct 16, 2015 at 04:33:42PM +0100, Robin Murphy wrote:
> __GFP_WAIT is going away to live its life under a new identity; convert
> __iommu_alloc_attrs() to the new helper function instead.
> 
> CC: Mel Gorman <mgorman@techsingularity.net>
> CC: Andrew Morton <akpm@linux-foundation.org>
> Reported-by: Sudeep Holla <sudeep.holla@arm.com>
> Signed-off-by: Robin Murphy <robin.murphy@arm.com>

Acked-by: Catalin Marinas <catalin.marinas@arm.com>

[PATCH 2/2] arm64: Use gfpflags_allow_blocking()

[Andrew Morton]

On Fri, 16 Oct 2015 16:33:42 +0100 Robin Murphy <robin.murphy@arm.com> wrote:

> __GFP_WAIT is going away to live its life under a new identity; convert
> __iommu_alloc_attrs() to the new helper function instead.
> 
> ...
>
> --- a/arch/arm64/mm/dma-mapping.c
> +++ b/arch/arm64/mm/dma-mapping.c
> @@ -566,7 +566,7 @@ static void *__iommu_alloc_attrs(struct device *dev, size_t size,
>  	 */
>  	gfp |= __GFP_ZERO;
>  
> -	if (gfp & __GFP_WAIT) {
> +	if (gfpflags_allow_blocking(gfp)) {
>  		struct page **pages;
>  		pgprot_t prot = __get_dma_pgprot(attrs, PAGE_KERNEL, coherent);

Seems unnecessarily elaborate.  What's wrong with

--- a/arch/arm64/mm/dma-mapping.c~mm-page_alloc-rename-__gfp_wait-to-__gfp_reclaim-arm-fix
+++ a/arch/arm64/mm/dma-mapping.c
@@ -562,7 +562,7 @@ static void *__iommu_alloc_attrs(struct
 	 */
 	gfp |= __GFP_ZERO;
 
-	if (gfp & __GFP_WAIT) {
+	if (gfp & __GFP_RECLAIM) {
 		struct page **pages;
 		pgprot_t prot = __get_dma_pgprot(attrs, PAGE_KERNEL, coherent);
 

?

[PATCH 2/2] arm64: Use gfpflags_allow_blocking()

[Robin Murphy]

Hi Andrew,

On 16/10/15 21:59, Andrew Morton wrote:
> On Fri, 16 Oct 2015 16:33:42 +0100 Robin Murphy <robin.murphy@arm.com> wrote:
>
>> __GFP_WAIT is going away to live its life under a new identity; convert
>> __iommu_alloc_attrs() to the new helper function instead.
>>
>> ...
>>
>> --- a/arch/arm64/mm/dma-mapping.c
>> +++ b/arch/arm64/mm/dma-mapping.c
>> @@ -566,7 +566,7 @@ static void *__iommu_alloc_attrs(struct device *dev, size_t size,
>>   	 */
>>   	gfp |= __GFP_ZERO;
>>
>> -	if (gfp & __GFP_WAIT) {
>> +	if (gfpflags_allow_blocking(gfp)) {
>>   		struct page **pages;
>>   		pgprot_t prot = __get_dma_pgprot(attrs, PAGE_KERNEL, coherent);
>
> Seems unnecessarily elaborate.  What's wrong with
>
> --- a/arch/arm64/mm/dma-mapping.c~mm-page_alloc-rename-__gfp_wait-to-__gfp_reclaim-arm-fix
> +++ a/arch/arm64/mm/dma-mapping.c
> @@ -562,7 +562,7 @@ static void *__iommu_alloc_attrs(struct
>   	 */
>   	gfp |= __GFP_ZERO;
>
> -	if (gfp & __GFP_WAIT) {
> +	if (gfp & __GFP_RECLAIM) {
>   		struct page **pages;
>   		pgprot_t prot = __get_dma_pgprot(attrs, PAGE_KERNEL, coherent);
>
>
> ?

Well, in that case the charge of "unnecessarily elaborate" should have 
been directed at the original patch, and the 53 other locations where 
(flags & __GFP_WAIT) was changed as per the commit message:

   "Callers that are checking if they are non-blocking should use the
    helper gfpflags_allow_blocking() where possible."

More importantly, it's also now apparently inconsistent with all the 
other dma_alloc_coherent() implementations, which thanks to the helper 
function are testing against __GFP_DIRECT_RECLAIM instead. As I have no 
clear understanding of what the difference between the two flags is, and 
how they relate to whether it's safe to call map_vm_area() or not (which 
is what principally matters here), I'm very uncomfortable with a change 
introducing that inconsistency.

If instead of my two patches you'd prefer to carry a fix through -mm and 
coordinate with Joerg and Linus to ensure everything gets merged in the 
right order, that's fine by me, but either way the change needs to 
guarantee the same behaviour as all the other instances in arch/arm and 
arch/arm64 which return a remapped buffer (and I have to say personally 
I much prefer having the rather inscrutable flag logic hidden behind a 
clearly-named helper function).

Thanks,
Robin.

[PATCH 2/2] arm64: Use gfpflags_allow_blocking()

[Mel Gorman]

On Mon, Oct 19, 2015 at 01:43:13PM +0100, Robin Murphy wrote:
> Hi Andrew,
> 
> On 16/10/15 21:59, Andrew Morton wrote:
> >On Fri, 16 Oct 2015 16:33:42 +0100 Robin Murphy <robin.murphy@arm.com> wrote:
> >
> >>__GFP_WAIT is going away to live its life under a new identity; convert
> >>__iommu_alloc_attrs() to the new helper function instead.
> >>
> >>...
> >>
> >>--- a/arch/arm64/mm/dma-mapping.c
> >>+++ b/arch/arm64/mm/dma-mapping.c
> >>@@ -566,7 +566,7 @@ static void *__iommu_alloc_attrs(struct device *dev, size_t size,
> >>  	 */
> >>  	gfp |= __GFP_ZERO;
> >>
> >>-	if (gfp & __GFP_WAIT) {
> >>+	if (gfpflags_allow_blocking(gfp)) {
> >>  		struct page **pages;
> >>  		pgprot_t prot = __get_dma_pgprot(attrs, PAGE_KERNEL, coherent);
> >
> >Seems unnecessarily elaborate.  What's wrong with
> >
> >--- a/arch/arm64/mm/dma-mapping.c~mm-page_alloc-rename-__gfp_wait-to-__gfp_reclaim-arm-fix
> >+++ a/arch/arm64/mm/dma-mapping.c
> >@@ -562,7 +562,7 @@ static void *__iommu_alloc_attrs(struct
> >  	 */
> >  	gfp |= __GFP_ZERO;
> >
> >-	if (gfp & __GFP_WAIT) {
> >+	if (gfp & __GFP_RECLAIM) {
> >  		struct page **pages;
> >  		pgprot_t prot = __get_dma_pgprot(attrs, PAGE_KERNEL, coherent);
> >
> >
> >?
> 
> Well, in that case the charge of "unnecessarily elaborate" should have been
> directed at the original patch, and the 53 other locations where (flags &
> __GFP_WAIT) was changed as per the commit message:
> 
>   "Callers that are checking if they are non-blocking should use the
>    helper gfpflags_allow_blocking() where possible."
> 

The use of gfpflags_allows_blocking() like you originally had is actually
preferred by me. __GFP_RECLAIM can return true when the caller only allows
kswapd to wake which has nothing to do with blocking (currently). The
helper was added to avoid this type of confusion.

-- 
Mel Gorman
SUSE Labs

[PATCH 1/2] arm64: Workaround renaming of __GFP_WAIT

[Catalin Marinas]

On Fri, Oct 16, 2015 at 04:33:41PM +0100, Robin Murphy wrote:
> The imminent renaming of __GFP_WAIT in the mm tree conflicts with its
> use in the new IOMMU DMA ops; introduce a temporary local version of
> its replacement to smooth over the transition.
> 
> This patch should be reverted at 4.4-rc1.
> 
> CC: Mel Gorman <mgorman@techsingularity.net>
> CC: Andrew Morton <akpm@linux-foundation.org>
> Reported-by: Sudeep Holla <sudeep.holla@arm.com>
> Signed-off-by: Robin Murphy <robin.murphy@arm.com>

Acked-by: Catalin Marinas <catalin.marinas@arm.com>

[PATCH 1/2] arm64: Workaround renaming of __GFP_WAIT

[Joerg Roedel]

Hi Robin,

On Fri, Oct 16, 2015 at 04:33:41PM +0100, Robin Murphy wrote:
> The imminent renaming of __GFP_WAIT in the mm tree conflicts with its
> use in the new IOMMU DMA ops; introduce a temporary local version of
> its replacement to smooth over the transition.
> 
> This patch should be reverted at 4.4-rc1.

I lost track here, are these two patches required in the iommu tree for
the next merge window?


	Joerg

[PATCH 1/2] arm64: Workaround renaming of __GFP_WAIT

[Robin Murphy]

Hi Joerg,

On 28/10/15 00:53, Joerg Roedel wrote:
> Hi Robin,
>
> On Fri, Oct 16, 2015 at 04:33:41PM +0100, Robin Murphy wrote:
>> The imminent renaming of __GFP_WAIT in the mm tree conflicts with its
>> use in the new IOMMU DMA ops; introduce a temporary local version of
>> its replacement to smooth over the transition.
>>
>> This patch should be reverted at 4.4-rc1.
>
> I lost track here, are these two patches required in the iommu tree for
> the next merge window?

Andrew has the equivalent of patch 2 as a fixup on top of Mel's series, 
which Stephen is currently carrying as a merge resolution in -next. I'm 
assuming this will go into the final merge as well, so as long as Linus 
takes the IOMMU tree first we should be OK as-is.

Robin.

>
>
> 	Joerg
>