Re: Question on ___GFP_NOLOCKDEP - Was: Re: [PATCH 1/1] Remove hardcoding of ___GFP_xxx bitmasks

[Vlastimil Babka]

On 04/27/2017 03:35 PM, Michal Hocko wrote:
> On Thu 27-04-17 15:16:47, Igor Stoppa wrote:
>> On 26/04/17 18:29, Igor Stoppa wrote:
>>
>>> On 26/04/17 17:47, Michal Hocko wrote:
>>
>> [...]
>>
 Also the current mm tree has ___GFP_NOLOCKDEP which is not addressed
 here so I suspect you have based your change on the Linus tree.
>>
>>> I used your tree from kernel.org
>>
>> I found it, I was using master, instead of auto-latest (is it correct?)
> 
> yes
> 
>> But now I see something that I do not understand (apologies if I'm
>> asking something obvious).
>>
>> First there is:
>>
>> [...]
>> #define ___GFP_WRITE 0x80u
>> #define ___GFP_KSWAPD_RECLAIM0x100u
>> #ifdef CONFIG_LOCKDEP
>> #define ___GFP_NOLOCKDEP 0x400u
>> #else
>> #define ___GFP_NOLOCKDEP 0
>> #endif
>>
>> Then:
>>
>> /* Room for N __GFP_FOO bits */
>> #define __GFP_BITS_SHIFT (25 + IS_ENABLED(CONFIG_LOCKDEP))
>>
>>
>>
>> Shouldn't it be either:
>> ___GFP_NOLOCKDEP 0x200u
> 
> Yes it should. At the time when this patch was written this value was
> used. Later I've removed __GFP_OTHER by 41b6167e8f74 ("mm: get rid of
> __GFP_OTHER_NODE") and forgot to refresh this one. Thanks for noticing
> this.
> 
> Andrew, could you fold the following in please?
> ---
> From 8dc9c917af215f659bb990fa48ae7b4753027c19 Mon Sep 17 00:00:00 2001
> From: Michal Hocko 
> Date: Thu, 27 Apr 2017 15:28:10 +0200
> Subject: [PATCH] lockdep-allow-to-disable-reclaim-lockup-detection-fix
> 
> Igor Stoppa has noticed that __GFP_NOLOCKDEP can use a lower bit. At the
> time lockdep-allow-to-disable-reclaim-lockup-detection was written we
> still had __GFP_OTHER_NODE but I have removed it in 41b6167e8f74 ("mm:
> get rid of __GFP_OTHER_NODE") and forgot to lower the bit value.
> 
> Signed-off-by: Michal Hocko 

Ping, I have noticed (at least in the mmotm-2017-05-08-16-30 git tag)
there's still 0x400u ?

> ---
>  include/linux/gfp.h | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/include/linux/gfp.h b/include/linux/gfp.h
> index 2b1a44f5bdb6..a89d37e8b387 100644
> --- a/include/linux/gfp.h
> +++ b/include/linux/gfp.h
> @@ -41,7 +41,7 @@ struct vm_area_struct;
>  #define ___GFP_WRITE 0x80u
>  #define ___GFP_KSWAPD_RECLAIM0x100u
>  #ifdef CONFIG_LOCKDEP
> -#define ___GFP_NOLOCKDEP 0x400u
> +#define ___GFP_NOLOCKDEP 0x200u
>  #else
>  #define ___GFP_NOLOCKDEP 0
>  #endif
> 

Re: Question on ___GFP_NOLOCKDEP - Was: Re: [PATCH 1/1] Remove hardcoding of ___GFP_xxx bitmasks

[Vlastimil Babka]

On 04/27/2017 03:35 PM, Michal Hocko wrote:
> On Thu 27-04-17 15:16:47, Igor Stoppa wrote:
>> On 26/04/17 18:29, Igor Stoppa wrote:
>>
>>> On 26/04/17 17:47, Michal Hocko wrote:
>>
>> [...]
>>
 Also the current mm tree has ___GFP_NOLOCKDEP which is not addressed
 here so I suspect you have based your change on the Linus tree.
>>
>>> I used your tree from kernel.org
>>
>> I found it, I was using master, instead of auto-latest (is it correct?)
> 
> yes
> 
>> But now I see something that I do not understand (apologies if I'm
>> asking something obvious).
>>
>> First there is:
>>
>> [...]
>> #define ___GFP_WRITE 0x80u
>> #define ___GFP_KSWAPD_RECLAIM0x100u
>> #ifdef CONFIG_LOCKDEP
>> #define ___GFP_NOLOCKDEP 0x400u
>> #else
>> #define ___GFP_NOLOCKDEP 0
>> #endif
>>
>> Then:
>>
>> /* Room for N __GFP_FOO bits */
>> #define __GFP_BITS_SHIFT (25 + IS_ENABLED(CONFIG_LOCKDEP))
>>
>>
>>
>> Shouldn't it be either:
>> ___GFP_NOLOCKDEP 0x200u
> 
> Yes it should. At the time when this patch was written this value was
> used. Later I've removed __GFP_OTHER by 41b6167e8f74 ("mm: get rid of
> __GFP_OTHER_NODE") and forgot to refresh this one. Thanks for noticing
> this.
> 
> Andrew, could you fold the following in please?
> ---
> From 8dc9c917af215f659bb990fa48ae7b4753027c19 Mon Sep 17 00:00:00 2001
> From: Michal Hocko 
> Date: Thu, 27 Apr 2017 15:28:10 +0200
> Subject: [PATCH] lockdep-allow-to-disable-reclaim-lockup-detection-fix
> 
> Igor Stoppa has noticed that __GFP_NOLOCKDEP can use a lower bit. At the
> time lockdep-allow-to-disable-reclaim-lockup-detection was written we
> still had __GFP_OTHER_NODE but I have removed it in 41b6167e8f74 ("mm:
> get rid of __GFP_OTHER_NODE") and forgot to lower the bit value.
> 
> Signed-off-by: Michal Hocko 

Ping, I have noticed (at least in the mmotm-2017-05-08-16-30 git tag)
there's still 0x400u ?

> ---
>  include/linux/gfp.h | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/include/linux/gfp.h b/include/linux/gfp.h
> index 2b1a44f5bdb6..a89d37e8b387 100644
> --- a/include/linux/gfp.h
> +++ b/include/linux/gfp.h
> @@ -41,7 +41,7 @@ struct vm_area_struct;
>  #define ___GFP_WRITE 0x80u
>  #define ___GFP_KSWAPD_RECLAIM0x100u
>  #ifdef CONFIG_LOCKDEP
> -#define ___GFP_NOLOCKDEP 0x400u
> +#define ___GFP_NOLOCKDEP 0x200u
>  #else
>  #define ___GFP_NOLOCKDEP 0
>  #endif
> 

Re: [PATCH 1/1] Remove hardcoding of ___GFP_xxx bitmasks

[Igor Stoppa]

On 28/04/17 10:43, Igor Stoppa wrote:

[...]

> I'm writing an alternative different proposal, let's call it last attempt.
> 
> Should be ready in a few minutes.

Here: http://marc.info/?l=linux-mm=149336675129967=2

--
thanks, igor

Re: [PATCH 1/1] Remove hardcoding of ___GFP_xxx bitmasks

[Igor Stoppa]

On 28/04/17 10:43, Igor Stoppa wrote:

[...]

> I'm writing an alternative different proposal, let's call it last attempt.
> 
> Should be ready in a few minutes.

Here: http://marc.info/?l=linux-mm=149336675129967=2

--
thanks, igor

Re: [PATCH 1/1] Remove hardcoding of ___GFP_xxx bitmasks

[Igor Stoppa]

On 28/04/17 10:40, Michal Hocko wrote:

> Do not add a new zone, really. What you seem to be looking for is an
> allocator on top of the page/memblock allocator which does write
> protection on top. I understand that you would like to avoid object
> management duplication but I am not really sure how much you can re-use
> what slab allocators do already, anyway. I will respond to the original
> thread to not mix things together.

I'm writing an alternative different proposal, let's call it last attempt.

Should be ready in a few minutes.

thanks, igor

Re: [PATCH 1/1] Remove hardcoding of ___GFP_xxx bitmasks

[Igor Stoppa]

On 28/04/17 10:40, Michal Hocko wrote:

> Do not add a new zone, really. What you seem to be looking for is an
> allocator on top of the page/memblock allocator which does write
> protection on top. I understand that you would like to avoid object
> management duplication but I am not really sure how much you can re-use
> what slab allocators do already, anyway. I will respond to the original
> thread to not mix things together.

I'm writing an alternative different proposal, let's call it last attempt.

Should be ready in a few minutes.

thanks, igor

Re: [PATCH 1/1] Remove hardcoding of ___GFP_xxx bitmasks

[Michal Hocko]

On Thu 27-04-17 17:06:05, Igor Stoppa wrote:
> 
> 
> On 27/04/17 16:41, Michal Hocko wrote:
> > On Wed 26-04-17 18:29:08, Igor Stoppa wrote:
> > [...]
> >> If you prefer to have this patch only as part of the larger patchset,
> >> I'm also fine with it.
> > 
> > I agree that the situation is not ideal. If a larger set of changes
> > would benefit from this change then it would clearly add arguments...
> 
> Ok, then I'll send it out as part of the larger RFC set.
> 
> 
> >> Also, if you could reply to [1], that would be greatly appreciated.
> > 
> > I will try to get to it but from a quick glance, yet-another-zone will
> > hit a lot of opposition...
> 
> The most basic questions, that I hope can be answered with Yes/No =) are:
> 
> - should a new zone be added after DMA32?
> 
> - should I try hard to keep the mask fitting a 32bit word - at least for
> hose who do not use the new zone - or is it ok to just stretch it to 64
> bits?

Do not add a new zone, really. What you seem to be looking for is an
allocator on top of the page/memblock allocator which does write
protection on top. I understand that you would like to avoid object
management duplication but I am not really sure how much you can re-use
what slab allocators do already, anyway. I will respond to the original
thread to not mix things together.
-- 
Michal Hocko
SUSE Labs

Re: [PATCH 1/1] Remove hardcoding of ___GFP_xxx bitmasks

[Michal Hocko]

On Thu 27-04-17 17:06:05, Igor Stoppa wrote:
> 
> 
> On 27/04/17 16:41, Michal Hocko wrote:
> > On Wed 26-04-17 18:29:08, Igor Stoppa wrote:
> > [...]
> >> If you prefer to have this patch only as part of the larger patchset,
> >> I'm also fine with it.
> > 
> > I agree that the situation is not ideal. If a larger set of changes
> > would benefit from this change then it would clearly add arguments...
> 
> Ok, then I'll send it out as part of the larger RFC set.
> 
> 
> >> Also, if you could reply to [1], that would be greatly appreciated.
> > 
> > I will try to get to it but from a quick glance, yet-another-zone will
> > hit a lot of opposition...
> 
> The most basic questions, that I hope can be answered with Yes/No =) are:
> 
> - should a new zone be added after DMA32?
> 
> - should I try hard to keep the mask fitting a 32bit word - at least for
> hose who do not use the new zone - or is it ok to just stretch it to 64
> bits?

Do not add a new zone, really. What you seem to be looking for is an
allocator on top of the page/memblock allocator which does write
protection on top. I understand that you would like to avoid object
management duplication but I am not really sure how much you can re-use
what slab allocators do already, anyway. I will respond to the original
thread to not mix things together.
-- 
Michal Hocko
SUSE Labs

Re: [PATCH 1/1] Remove hardcoding of ___GFP_xxx bitmasks

[Igor Stoppa]

On 27/04/17 16:41, Michal Hocko wrote:
> On Wed 26-04-17 18:29:08, Igor Stoppa wrote:
> [...]
>> If you prefer to have this patch only as part of the larger patchset,
>> I'm also fine with it.
> 
> I agree that the situation is not ideal. If a larger set of changes
> would benefit from this change then it would clearly add arguments...

Ok, then I'll send it out as part of the larger RFC set.


>> Also, if you could reply to [1], that would be greatly appreciated.
> 
> I will try to get to it but from a quick glance, yet-another-zone will
> hit a lot of opposition...

The most basic questions, that I hope can be answered with Yes/No =) are:

- should a new zone be added after DMA32?

- should I try hard to keep the mask fitting a 32bit word - at least for
hose who do not use the new zone - or is it ok to just stretch it to 64
bits?



If you could answer these, then I'll have a better idea of what I need
to do to.

TIA, igor

Re: [PATCH 1/1] Remove hardcoding of ___GFP_xxx bitmasks

[Igor Stoppa]

On 27/04/17 16:41, Michal Hocko wrote:
> On Wed 26-04-17 18:29:08, Igor Stoppa wrote:
> [...]
>> If you prefer to have this patch only as part of the larger patchset,
>> I'm also fine with it.
> 
> I agree that the situation is not ideal. If a larger set of changes
> would benefit from this change then it would clearly add arguments...

Ok, then I'll send it out as part of the larger RFC set.


>> Also, if you could reply to [1], that would be greatly appreciated.
> 
> I will try to get to it but from a quick glance, yet-another-zone will
> hit a lot of opposition...

The most basic questions, that I hope can be answered with Yes/No =) are:

- should a new zone be added after DMA32?

- should I try hard to keep the mask fitting a 32bit word - at least for
hose who do not use the new zone - or is it ok to just stretch it to 64
bits?



If you could answer these, then I'll have a better idea of what I need
to do to.

TIA, igor

Re: [PATCH 1/1] Remove hardcoding of ___GFP_xxx bitmasks

[Michal Hocko]

On Wed 26-04-17 18:29:08, Igor Stoppa wrote:
[...]
> If you prefer to have this patch only as part of the larger patchset,
> I'm also fine with it.

I agree that the situation is not ideal. If a larger set of changes
would benefit from this change then it would clearly add arguments...

> Also, if you could reply to [1], that would be greatly appreciated.

I will try to get to it but from a quick glance, yet-another-zone will
hit a lot of opposition...
-- 
Michal Hocko
SUSE Labs

Re: [PATCH 1/1] Remove hardcoding of ___GFP_xxx bitmasks

[Michal Hocko]

On Wed 26-04-17 18:29:08, Igor Stoppa wrote:
[...]
> If you prefer to have this patch only as part of the larger patchset,
> I'm also fine with it.

I agree that the situation is not ideal. If a larger set of changes
would benefit from this change then it would clearly add arguments...

> Also, if you could reply to [1], that would be greatly appreciated.

I will try to get to it but from a quick glance, yet-another-zone will
hit a lot of opposition...
-- 
Michal Hocko
SUSE Labs

Re: Question on ___GFP_NOLOCKDEP - Was: Re: [PATCH 1/1] Remove hardcoding of ___GFP_xxx bitmasks

[Michal Hocko]

On Thu 27-04-17 15:16:47, Igor Stoppa wrote:
> On 26/04/17 18:29, Igor Stoppa wrote:
> 
> > On 26/04/17 17:47, Michal Hocko wrote:
> 
> [...]
> 
> >> Also the current mm tree has ___GFP_NOLOCKDEP which is not addressed
> >> here so I suspect you have based your change on the Linus tree.
> 
> > I used your tree from kernel.org
> 
> I found it, I was using master, instead of auto-latest (is it correct?)

yes

> But now I see something that I do not understand (apologies if I'm
> asking something obvious).
> 
> First there is:
> 
> [...]
> #define ___GFP_WRITE  0x80u
> #define ___GFP_KSWAPD_RECLAIM 0x100u
> #ifdef CONFIG_LOCKDEP
> #define ___GFP_NOLOCKDEP  0x400u
> #else
> #define ___GFP_NOLOCKDEP  0
> #endif
> 
> Then:
> 
> /* Room for N __GFP_FOO bits */
> #define __GFP_BITS_SHIFT (25 + IS_ENABLED(CONFIG_LOCKDEP))
> 
> 
> 
> Shouldn't it be either:
> ___GFP_NOLOCKDEP  0x200u

Yes it should. At the time when this patch was written this value was
used. Later I've removed __GFP_OTHER by 41b6167e8f74 ("mm: get rid of
__GFP_OTHER_NODE") and forgot to refresh this one. Thanks for noticing
this.

Andrew, could you fold the following in please?
---
>From 8dc9c917af215f659bb990fa48ae7b4753027c19 Mon Sep 17 00:00:00 2001
From: Michal Hocko 
Date: Thu, 27 Apr 2017 15:28:10 +0200
Subject: [PATCH] lockdep-allow-to-disable-reclaim-lockup-detection-fix

Igor Stoppa has noticed that __GFP_NOLOCKDEP can use a lower bit. At the
time lockdep-allow-to-disable-reclaim-lockup-detection was written we
still had __GFP_OTHER_NODE but I have removed it in 41b6167e8f74 ("mm:
get rid of __GFP_OTHER_NODE") and forgot to lower the bit value.

Signed-off-by: Michal Hocko 
---
 include/linux/gfp.h | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/include/linux/gfp.h b/include/linux/gfp.h
index 2b1a44f5bdb6..a89d37e8b387 100644
--- a/include/linux/gfp.h
+++ b/include/linux/gfp.h
@@ -41,7 +41,7 @@ struct vm_area_struct;
 #define ___GFP_WRITE   0x80u
 #define ___GFP_KSWAPD_RECLAIM  0x100u
 #ifdef CONFIG_LOCKDEP
-#define ___GFP_NOLOCKDEP   0x400u
+#define ___GFP_NOLOCKDEP   0x200u
 #else
 #define ___GFP_NOLOCKDEP   0
 #endif
-- 
2.11.0


-- 
Michal Hocko
SUSE Labs

Re: Question on ___GFP_NOLOCKDEP - Was: Re: [PATCH 1/1] Remove hardcoding of ___GFP_xxx bitmasks

[Michal Hocko]

On Thu 27-04-17 15:16:47, Igor Stoppa wrote:
> On 26/04/17 18:29, Igor Stoppa wrote:
> 
> > On 26/04/17 17:47, Michal Hocko wrote:
> 
> [...]
> 
> >> Also the current mm tree has ___GFP_NOLOCKDEP which is not addressed
> >> here so I suspect you have based your change on the Linus tree.
> 
> > I used your tree from kernel.org
> 
> I found it, I was using master, instead of auto-latest (is it correct?)

yes

> But now I see something that I do not understand (apologies if I'm
> asking something obvious).
> 
> First there is:
> 
> [...]
> #define ___GFP_WRITE  0x80u
> #define ___GFP_KSWAPD_RECLAIM 0x100u
> #ifdef CONFIG_LOCKDEP
> #define ___GFP_NOLOCKDEP  0x400u
> #else
> #define ___GFP_NOLOCKDEP  0
> #endif
> 
> Then:
> 
> /* Room for N __GFP_FOO bits */
> #define __GFP_BITS_SHIFT (25 + IS_ENABLED(CONFIG_LOCKDEP))
> 
> 
> 
> Shouldn't it be either:
> ___GFP_NOLOCKDEP  0x200u

Yes it should. At the time when this patch was written this value was
used. Later I've removed __GFP_OTHER by 41b6167e8f74 ("mm: get rid of
__GFP_OTHER_NODE") and forgot to refresh this one. Thanks for noticing
this.

Andrew, could you fold the following in please?
---
>From 8dc9c917af215f659bb990fa48ae7b4753027c19 Mon Sep 17 00:00:00 2001
From: Michal Hocko 
Date: Thu, 27 Apr 2017 15:28:10 +0200
Subject: [PATCH] lockdep-allow-to-disable-reclaim-lockup-detection-fix

Igor Stoppa has noticed that __GFP_NOLOCKDEP can use a lower bit. At the
time lockdep-allow-to-disable-reclaim-lockup-detection was written we
still had __GFP_OTHER_NODE but I have removed it in 41b6167e8f74 ("mm:
get rid of __GFP_OTHER_NODE") and forgot to lower the bit value.

Signed-off-by: Michal Hocko 
---
 include/linux/gfp.h | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/include/linux/gfp.h b/include/linux/gfp.h
index 2b1a44f5bdb6..a89d37e8b387 100644
--- a/include/linux/gfp.h
+++ b/include/linux/gfp.h
@@ -41,7 +41,7 @@ struct vm_area_struct;
 #define ___GFP_WRITE   0x80u
 #define ___GFP_KSWAPD_RECLAIM  0x100u
 #ifdef CONFIG_LOCKDEP
-#define ___GFP_NOLOCKDEP   0x400u
+#define ___GFP_NOLOCKDEP   0x200u
 #else
 #define ___GFP_NOLOCKDEP   0
 #endif
-- 
2.11.0


-- 
Michal Hocko
SUSE Labs

Question on ___GFP_NOLOCKDEP - Was: Re: [PATCH 1/1] Remove hardcoding of ___GFP_xxx bitmasks

[Igor Stoppa]

On 26/04/17 18:29, Igor Stoppa wrote:

> On 26/04/17 17:47, Michal Hocko wrote:

[...]

>> Also the current mm tree has ___GFP_NOLOCKDEP which is not addressed
>> here so I suspect you have based your change on the Linus tree.

> I used your tree from kernel.org

I found it, I was using master, instead of auto-latest (is it correct?)
But now I see something that I do not understand (apologies if I'm
asking something obvious).

First there is:

[...]
#define ___GFP_WRITE0x80u
#define ___GFP_KSWAPD_RECLAIM   0x100u
#ifdef CONFIG_LOCKDEP
#define ___GFP_NOLOCKDEP0x400u
#else
#define ___GFP_NOLOCKDEP0
#endif

Then:

/* Room for N __GFP_FOO bits */
#define __GFP_BITS_SHIFT (25 + IS_ENABLED(CONFIG_LOCKDEP))



Shouldn't it be either:
___GFP_NOLOCKDEP0x200u

or:

#define __GFP_BITS_SHIFT (25 + 2 * IS_ENABLED(CONFIG_LOCKDEP))


thanks, igor

Question on ___GFP_NOLOCKDEP - Was: Re: [PATCH 1/1] Remove hardcoding of ___GFP_xxx bitmasks

[Igor Stoppa]

On 26/04/17 18:29, Igor Stoppa wrote:

> On 26/04/17 17:47, Michal Hocko wrote:

[...]

>> Also the current mm tree has ___GFP_NOLOCKDEP which is not addressed
>> here so I suspect you have based your change on the Linus tree.

> I used your tree from kernel.org

I found it, I was using master, instead of auto-latest (is it correct?)
But now I see something that I do not understand (apologies if I'm
asking something obvious).

First there is:

[...]
#define ___GFP_WRITE0x80u
#define ___GFP_KSWAPD_RECLAIM   0x100u
#ifdef CONFIG_LOCKDEP
#define ___GFP_NOLOCKDEP0x400u
#else
#define ___GFP_NOLOCKDEP0
#endif

Then:

/* Room for N __GFP_FOO bits */
#define __GFP_BITS_SHIFT (25 + IS_ENABLED(CONFIG_LOCKDEP))



Shouldn't it be either:
___GFP_NOLOCKDEP0x200u

or:

#define __GFP_BITS_SHIFT (25 + 2 * IS_ENABLED(CONFIG_LOCKDEP))


thanks, igor

Question on ___GFP_NOLOCKDEP - Was: Re: [PATCH 1/1] Remove hardcoding of ___GFP_xxx bitmasks

[Igor Stoppa]

On 26/04/17 18:29, Igor Stoppa wrote:

> On 26/04/17 17:47, Michal Hocko wrote:

[...]

>> Also the current mm tree has ___GFP_NOLOCKDEP which is not addressed
>> here so I suspect you have based your change on the Linus tree.

> I used your tree from kernel.org

I found it, I was using master, instead of auto-latest (is it correct?)
But now I see something that I do not understand (apologies if I'm
asking something obvious).

First there is:

[...]
#define ___GFP_WRITE0x80u
#define ___GFP_KSWAPD_RECLAIM   0x100u
#ifdef CONFIG_LOCKDEP
#define ___GFP_NOLOCKDEP0x400u
#else
#define ___GFP_NOLOCKDEP0
#endif

Then:

/* Room for N __GFP_FOO bits */
#define __GFP_BITS_SHIFT (25 + IS_ENABLED(CONFIG_LOCKDEP))



Shouldn't it be either:
___GFP_NOLOCKDEP0x200u

or:

#define __GFP_BITS_SHIFT (25 + 2 * IS_ENABLED(CONFIG_LOCKDEP))


thanks, igor

Question on ___GFP_NOLOCKDEP - Was: Re: [PATCH 1/1] Remove hardcoding of ___GFP_xxx bitmasks

[Igor Stoppa]

On 26/04/17 18:29, Igor Stoppa wrote:

> On 26/04/17 17:47, Michal Hocko wrote:

[...]

>> Also the current mm tree has ___GFP_NOLOCKDEP which is not addressed
>> here so I suspect you have based your change on the Linus tree.

> I used your tree from kernel.org

I found it, I was using master, instead of auto-latest (is it correct?)
But now I see something that I do not understand (apologies if I'm
asking something obvious).

First there is:

[...]
#define ___GFP_WRITE0x80u
#define ___GFP_KSWAPD_RECLAIM   0x100u
#ifdef CONFIG_LOCKDEP
#define ___GFP_NOLOCKDEP0x400u
#else
#define ___GFP_NOLOCKDEP0
#endif

Then:

/* Room for N __GFP_FOO bits */
#define __GFP_BITS_SHIFT (25 + IS_ENABLED(CONFIG_LOCKDEP))



Shouldn't it be either:
___GFP_NOLOCKDEP0x200u

or:

#define __GFP_BITS_SHIFT (25 + 2 * IS_ENABLED(CONFIG_LOCKDEP))


thanks, igor

Re: [PATCH 1/1] Remove hardcoding of ___GFP_xxx bitmasks

[Igor Stoppa]

On 26/04/17 17:47, Michal Hocko wrote:
> On Wed 26-04-17 16:35:49, Igor Stoppa wrote:
>> The bitmasks used for ___GFP_xxx can be defined in terms of an enum,
>> which doesn't require manual updates to its values.
> 
> GFP masks are rarely updated so why is this worth doing?

I have plans for that [1] - yeah, I should have not written only to ml -
but I thought there was sufficient value in this patch to be sent alone.

I got into this part of the code because (if I understood correctly)
there are no spare bits available from the 32bits mask that is currently
in use.

Adding a new zone, therefore, would cause the bumping to a 64bits type.
If the zone is not strictly needed, some people might prefer to have the
option to stick to 32 bits.

Which in turn would mean more #ifdefs.

Using the enum should provide the same flexibility with a more limited
number of #ifdefs in the code.

But I really thought that there is a value in the change per-se.
Regardless of what other patches might follow.


>> As bonus, __GFP_BITS_SHIFT is automatically kept consistent.
> 
> this alone doesn't sound like a huge win to me, to be honest. We already
> have ___GFP_$FOO and __GFP_FOO you are adding __GFP_$FOO_SHIFT. This is
> too much IMHO.

I do not like the #defines being floating and potentially inconsistent
with the rest, when they are supposed to represent all the individual
bits in a bitmask.
One might argue that an error will be detected fairly soon, but to me
using an enum to automatically manage the values and counter of items
seems preferable.

> Also the current mm tree has ___GFP_NOLOCKDEP which is not addressed
> here so I suspect you have based your change on the Linus tree.

I used your tree from kernel.org - I asked yesterday on #mm if it was a
good idea and was told that it should be ok, so I did it, but I can redo
the patch with mm.


If you prefer to have this patch only as part of the larger patchset,
I'm also fine with it.
Also, if you could reply to [1], that would be greatly appreciated.

Maybe I'm starting from some wrong assumption or there is a better way
to achieve what I want.


thanks, igor

[1] http://marc.info/?l=linux-mm=149276346722464=2

Re: [PATCH 1/1] Remove hardcoding of ___GFP_xxx bitmasks

[Igor Stoppa]

On 26/04/17 17:47, Michal Hocko wrote:
> On Wed 26-04-17 16:35:49, Igor Stoppa wrote:
>> The bitmasks used for ___GFP_xxx can be defined in terms of an enum,
>> which doesn't require manual updates to its values.
> 
> GFP masks are rarely updated so why is this worth doing?

I have plans for that [1] - yeah, I should have not written only to ml -
but I thought there was sufficient value in this patch to be sent alone.

I got into this part of the code because (if I understood correctly)
there are no spare bits available from the 32bits mask that is currently
in use.

Adding a new zone, therefore, would cause the bumping to a 64bits type.
If the zone is not strictly needed, some people might prefer to have the
option to stick to 32 bits.

Which in turn would mean more #ifdefs.

Using the enum should provide the same flexibility with a more limited
number of #ifdefs in the code.

But I really thought that there is a value in the change per-se.
Regardless of what other patches might follow.


>> As bonus, __GFP_BITS_SHIFT is automatically kept consistent.
> 
> this alone doesn't sound like a huge win to me, to be honest. We already
> have ___GFP_$FOO and __GFP_FOO you are adding __GFP_$FOO_SHIFT. This is
> too much IMHO.

I do not like the #defines being floating and potentially inconsistent
with the rest, when they are supposed to represent all the individual
bits in a bitmask.
One might argue that an error will be detected fairly soon, but to me
using an enum to automatically manage the values and counter of items
seems preferable.

> Also the current mm tree has ___GFP_NOLOCKDEP which is not addressed
> here so I suspect you have based your change on the Linus tree.

I used your tree from kernel.org - I asked yesterday on #mm if it was a
good idea and was told that it should be ok, so I did it, but I can redo
the patch with mm.


If you prefer to have this patch only as part of the larger patchset,
I'm also fine with it.
Also, if you could reply to [1], that would be greatly appreciated.

Maybe I'm starting from some wrong assumption or there is a better way
to achieve what I want.


thanks, igor

[1] http://marc.info/?l=linux-mm=149276346722464=2

Re: [PATCH 1/1] Remove hardcoding of ___GFP_xxx bitmasks

[Michal Hocko]

On Wed 26-04-17 16:35:49, Igor Stoppa wrote:
> The bitmasks used for ___GFP_xxx can be defined in terms of an enum,
> which doesn't require manual updates to its values.

GFP masks are rarely updated so why is this worth doing?

> As bonus, __GFP_BITS_SHIFT is automatically kept consistent.

this alone doesn't sound like a huge win to me, to be honest. We already
have ___GFP_$FOO and __GFP_FOO you are adding __GFP_$FOO_SHIFT. This is
too much IMHO.

Also the current mm tree has ___GFP_NOLOCKDEP which is not addressed
here so I suspect you have based your change on the Linus tree.

> Signed-off-by: Igor Stoppa 
> ---
>  include/linux/gfp.h | 82 
> +++--
>  1 file changed, 55 insertions(+), 27 deletions(-)
> 
> diff --git a/include/linux/gfp.h b/include/linux/gfp.h
> index 0fe0b62..2f894c5 100644
> --- a/include/linux/gfp.h
> +++ b/include/linux/gfp.h
> @@ -14,33 +14,62 @@ struct vm_area_struct;
>   * include/trace/events/mmflags.h and tools/perf/builtin-kmem.c
>   */
>  
> +enum gfp_bitmask_shift {
> + __GFP_DMA_SHIFT = 0,
> + __GFP_HIGHMEM_SHIFT,
> + __GFP_DMA32_SHIFT,
> + __GFP_MOVABLE_SHIFT,
> + __GFP_RECLAIMABLE_SHIFT,
> + __GFP_HIGH_SHIFT,
> + __GFP_IO_SHIFT,
> + __GFP_FS_SHIFT,
> + __GFP_COLD_SHIFT,
> + __GFP_NOWARN_SHIFT,
> + __GFP_REPEAT_SHIFT,
> + __GFP_NOFAIL_SHIFT,
> + __GFP_NORETRY_SHIFT,
> + __GFP_MEMALLOC_SHIFT,
> + __GFP_COMP_SHIFT,
> + __GFP_ZERO_SHIFT,
> + __GFP_NOMEMALLOC_SHIFT,
> + __GFP_HARDWALL_SHIFT,
> + __GFP_THISNODE_SHIFT,
> + __GFP_ATOMIC_SHIFT,
> + __GFP_ACCOUNT_SHIFT,
> + __GFP_NOTRACK_SHIFT,
> + __GFP_DIRECT_RECLAIM_SHIFT,
> + __GFP_WRITE_SHIFT,
> + __GFP_KSWAPD_RECLAIM_SHIFT,
> + __GFP_BITS_SHIFT
> +};
> +
> +
>  /* Plain integer GFP bitmasks. Do not use this directly. */
> -#define ___GFP_DMA   0x01u
> -#define ___GFP_HIGHMEM   0x02u
> -#define ___GFP_DMA32 0x04u
> -#define ___GFP_MOVABLE   0x08u
> -#define ___GFP_RECLAIMABLE   0x10u
> -#define ___GFP_HIGH  0x20u
> -#define ___GFP_IO0x40u
> -#define ___GFP_FS0x80u
> -#define ___GFP_COLD  0x100u
> -#define ___GFP_NOWARN0x200u
> -#define ___GFP_REPEAT0x400u
> -#define ___GFP_NOFAIL0x800u
> -#define ___GFP_NORETRY   0x1000u
> -#define ___GFP_MEMALLOC  0x2000u
> -#define ___GFP_COMP  0x4000u
> -#define ___GFP_ZERO  0x8000u
> -#define ___GFP_NOMEMALLOC0x1u
> -#define ___GFP_HARDWALL  0x2u
> -#define ___GFP_THISNODE  0x4u
> -#define ___GFP_ATOMIC0x8u
> -#define ___GFP_ACCOUNT   0x10u
> -#define ___GFP_NOTRACK   0x20u
> -#define ___GFP_DIRECT_RECLAIM0x40u
> -#define ___GFP_WRITE 0x80u
> -#define ___GFP_KSWAPD_RECLAIM0x100u
> -/* If the above are modified, __GFP_BITS_SHIFT may need updating */
> +#define ___GFP_DMA   (1u << __GFP_DMA_SHIFT)
> +#define ___GFP_HIGHMEM   (1u << __GFP_HIGHMEM_SHIFT)
> +#define ___GFP_DMA32 (1u << __GFP_DMA32_SHIFT)
> +#define ___GFP_MOVABLE   (1u << __GFP_MOVABLE_SHIFT)
> +#define ___GFP_RECLAIMABLE   (1u << __GFP_RECLAIMABLE_SHIFT)
> +#define ___GFP_HIGH  (1u << __GFP_HIGH_SHIFT)
> +#define ___GFP_IO(1u << __GFP_IO_SHIFT)
> +#define ___GFP_FS(1u << __GFP_FS_SHIFT)
> +#define ___GFP_COLD  (1u << __GFP_COLD_SHIFT)
> +#define ___GFP_NOWARN(1u << __GFP_NOWARN_SHIFT)
> +#define ___GFP_REPEAT(1u << __GFP_REPEAT_SHIFT)
> +#define ___GFP_NOFAIL(1u << __GFP_NOFAIL_SHIFT)
> +#define ___GFP_NORETRY   (1u << __GFP_NORETRY_SHIFT)
> +#define ___GFP_MEMALLOC  (1u << __GFP_MEMALLOC_SHIFT)
> +#define ___GFP_COMP  (1u << __GFP_COMP_SHIFT)
> +#define ___GFP_ZERO  (1u << __GFP_ZERO_SHIFT)
> +#define ___GFP_NOMEMALLOC(1u << __GFP_NOMEMALLOC_SHIFT)
> +#define ___GFP_HARDWALL  (1u << __GFP_HARDWALL_SHIFT)
> +#define ___GFP_THISNODE  (1u << __GFP_THISNODE_SHIFT)
> +#define ___GFP_ATOMIC(1u << __GFP_ATOMIC_SHIFT)
> +#define ___GFP_ACCOUNT   (1u << __GFP_ACCOUNT_SHIFT)
> +#define ___GFP_NOTRACK   (1u << __GFP_NOTRACK_SHIFT)
> +#define ___GFP_DIRECT_RECLAIM(1u << __GFP_DIRECT_RECLAIM_SHIFT)
> +#define ___GFP_WRITE (1u << __GFP_WRITE_SHIFT)
> +#define ___GFP_KSWAPD_RECLAIM(1u << __GFP_KSWAPD_RECLAIM_SHIFT)
>  
>  /*
>   * Physical address zone modifiers (see linux/mmzone.h - low four bits)
> @@ -180,7 +209,6 @@ struct vm_area_struct;
>  #define __GFP_NOTRACK_FALSE_POSITIVE (__GFP_NOTRACK)
>  
>  /* Room for N __GFP_FOO bits */
> -#define __GFP_BITS_SHIFT 25
>  #define __GFP_BITS_MASK ((__force gfp_t)((1 << 

Re: [PATCH 1/1] Remove hardcoding of ___GFP_xxx bitmasks

[Michal Hocko]

On Wed 26-04-17 16:35:49, Igor Stoppa wrote:
> The bitmasks used for ___GFP_xxx can be defined in terms of an enum,
> which doesn't require manual updates to its values.

GFP masks are rarely updated so why is this worth doing?

> As bonus, __GFP_BITS_SHIFT is automatically kept consistent.

this alone doesn't sound like a huge win to me, to be honest. We already
have ___GFP_$FOO and __GFP_FOO you are adding __GFP_$FOO_SHIFT. This is
too much IMHO.

Also the current mm tree has ___GFP_NOLOCKDEP which is not addressed
here so I suspect you have based your change on the Linus tree.

> Signed-off-by: Igor Stoppa 
> ---
>  include/linux/gfp.h | 82 
> +++--
>  1 file changed, 55 insertions(+), 27 deletions(-)
> 
> diff --git a/include/linux/gfp.h b/include/linux/gfp.h
> index 0fe0b62..2f894c5 100644
> --- a/include/linux/gfp.h
> +++ b/include/linux/gfp.h
> @@ -14,33 +14,62 @@ struct vm_area_struct;
>   * include/trace/events/mmflags.h and tools/perf/builtin-kmem.c
>   */
>  
> +enum gfp_bitmask_shift {
> + __GFP_DMA_SHIFT = 0,
> + __GFP_HIGHMEM_SHIFT,
> + __GFP_DMA32_SHIFT,
> + __GFP_MOVABLE_SHIFT,
> + __GFP_RECLAIMABLE_SHIFT,
> + __GFP_HIGH_SHIFT,
> + __GFP_IO_SHIFT,
> + __GFP_FS_SHIFT,
> + __GFP_COLD_SHIFT,
> + __GFP_NOWARN_SHIFT,
> + __GFP_REPEAT_SHIFT,
> + __GFP_NOFAIL_SHIFT,
> + __GFP_NORETRY_SHIFT,
> + __GFP_MEMALLOC_SHIFT,
> + __GFP_COMP_SHIFT,
> + __GFP_ZERO_SHIFT,
> + __GFP_NOMEMALLOC_SHIFT,
> + __GFP_HARDWALL_SHIFT,
> + __GFP_THISNODE_SHIFT,
> + __GFP_ATOMIC_SHIFT,
> + __GFP_ACCOUNT_SHIFT,
> + __GFP_NOTRACK_SHIFT,
> + __GFP_DIRECT_RECLAIM_SHIFT,
> + __GFP_WRITE_SHIFT,
> + __GFP_KSWAPD_RECLAIM_SHIFT,
> + __GFP_BITS_SHIFT
> +};
> +
> +
>  /* Plain integer GFP bitmasks. Do not use this directly. */
> -#define ___GFP_DMA   0x01u
> -#define ___GFP_HIGHMEM   0x02u
> -#define ___GFP_DMA32 0x04u
> -#define ___GFP_MOVABLE   0x08u
> -#define ___GFP_RECLAIMABLE   0x10u
> -#define ___GFP_HIGH  0x20u
> -#define ___GFP_IO0x40u
> -#define ___GFP_FS0x80u
> -#define ___GFP_COLD  0x100u
> -#define ___GFP_NOWARN0x200u
> -#define ___GFP_REPEAT0x400u
> -#define ___GFP_NOFAIL0x800u
> -#define ___GFP_NORETRY   0x1000u
> -#define ___GFP_MEMALLOC  0x2000u
> -#define ___GFP_COMP  0x4000u
> -#define ___GFP_ZERO  0x8000u
> -#define ___GFP_NOMEMALLOC0x1u
> -#define ___GFP_HARDWALL  0x2u
> -#define ___GFP_THISNODE  0x4u
> -#define ___GFP_ATOMIC0x8u
> -#define ___GFP_ACCOUNT   0x10u
> -#define ___GFP_NOTRACK   0x20u
> -#define ___GFP_DIRECT_RECLAIM0x40u
> -#define ___GFP_WRITE 0x80u
> -#define ___GFP_KSWAPD_RECLAIM0x100u
> -/* If the above are modified, __GFP_BITS_SHIFT may need updating */
> +#define ___GFP_DMA   (1u << __GFP_DMA_SHIFT)
> +#define ___GFP_HIGHMEM   (1u << __GFP_HIGHMEM_SHIFT)
> +#define ___GFP_DMA32 (1u << __GFP_DMA32_SHIFT)
> +#define ___GFP_MOVABLE   (1u << __GFP_MOVABLE_SHIFT)
> +#define ___GFP_RECLAIMABLE   (1u << __GFP_RECLAIMABLE_SHIFT)
> +#define ___GFP_HIGH  (1u << __GFP_HIGH_SHIFT)
> +#define ___GFP_IO(1u << __GFP_IO_SHIFT)
> +#define ___GFP_FS(1u << __GFP_FS_SHIFT)
> +#define ___GFP_COLD  (1u << __GFP_COLD_SHIFT)
> +#define ___GFP_NOWARN(1u << __GFP_NOWARN_SHIFT)
> +#define ___GFP_REPEAT(1u << __GFP_REPEAT_SHIFT)
> +#define ___GFP_NOFAIL(1u << __GFP_NOFAIL_SHIFT)
> +#define ___GFP_NORETRY   (1u << __GFP_NORETRY_SHIFT)
> +#define ___GFP_MEMALLOC  (1u << __GFP_MEMALLOC_SHIFT)
> +#define ___GFP_COMP  (1u << __GFP_COMP_SHIFT)
> +#define ___GFP_ZERO  (1u << __GFP_ZERO_SHIFT)
> +#define ___GFP_NOMEMALLOC(1u << __GFP_NOMEMALLOC_SHIFT)
> +#define ___GFP_HARDWALL  (1u << __GFP_HARDWALL_SHIFT)
> +#define ___GFP_THISNODE  (1u << __GFP_THISNODE_SHIFT)
> +#define ___GFP_ATOMIC(1u << __GFP_ATOMIC_SHIFT)
> +#define ___GFP_ACCOUNT   (1u << __GFP_ACCOUNT_SHIFT)
> +#define ___GFP_NOTRACK   (1u << __GFP_NOTRACK_SHIFT)
> +#define ___GFP_DIRECT_RECLAIM(1u << __GFP_DIRECT_RECLAIM_SHIFT)
> +#define ___GFP_WRITE (1u << __GFP_WRITE_SHIFT)
> +#define ___GFP_KSWAPD_RECLAIM(1u << __GFP_KSWAPD_RECLAIM_SHIFT)
>  
>  /*
>   * Physical address zone modifiers (see linux/mmzone.h - low four bits)
> @@ -180,7 +209,6 @@ struct vm_area_struct;
>  #define __GFP_NOTRACK_FALSE_POSITIVE (__GFP_NOTRACK)
>  
>  /* Room for N __GFP_FOO bits */
> -#define __GFP_BITS_SHIFT 25
>  #define __GFP_BITS_MASK ((__force gfp_t)((1 << __GFP_BITS_SHIFT) - 1))
>  
>  

[PATCH 1/1] Remove hardcoding of ___GFP_xxx bitmasks

[Igor Stoppa]

The bitmasks used for ___GFP_xxx can be defined in terms of an enum,
which doesn't require manual updates to its values.

As bonus, __GFP_BITS_SHIFT is automatically kept consistent.

Signed-off-by: Igor Stoppa 
---
 include/linux/gfp.h | 82 +++--
 1 file changed, 55 insertions(+), 27 deletions(-)

diff --git a/include/linux/gfp.h b/include/linux/gfp.h
index 0fe0b62..2f894c5 100644
--- a/include/linux/gfp.h
+++ b/include/linux/gfp.h
@@ -14,33 +14,62 @@ struct vm_area_struct;
  * include/trace/events/mmflags.h and tools/perf/builtin-kmem.c
  */
 
+enum gfp_bitmask_shift {
+   __GFP_DMA_SHIFT = 0,
+   __GFP_HIGHMEM_SHIFT,
+   __GFP_DMA32_SHIFT,
+   __GFP_MOVABLE_SHIFT,
+   __GFP_RECLAIMABLE_SHIFT,
+   __GFP_HIGH_SHIFT,
+   __GFP_IO_SHIFT,
+   __GFP_FS_SHIFT,
+   __GFP_COLD_SHIFT,
+   __GFP_NOWARN_SHIFT,
+   __GFP_REPEAT_SHIFT,
+   __GFP_NOFAIL_SHIFT,
+   __GFP_NORETRY_SHIFT,
+   __GFP_MEMALLOC_SHIFT,
+   __GFP_COMP_SHIFT,
+   __GFP_ZERO_SHIFT,
+   __GFP_NOMEMALLOC_SHIFT,
+   __GFP_HARDWALL_SHIFT,
+   __GFP_THISNODE_SHIFT,
+   __GFP_ATOMIC_SHIFT,
+   __GFP_ACCOUNT_SHIFT,
+   __GFP_NOTRACK_SHIFT,
+   __GFP_DIRECT_RECLAIM_SHIFT,
+   __GFP_WRITE_SHIFT,
+   __GFP_KSWAPD_RECLAIM_SHIFT,
+   __GFP_BITS_SHIFT
+};
+
+
 /* Plain integer GFP bitmasks. Do not use this directly. */
-#define ___GFP_DMA 0x01u
-#define ___GFP_HIGHMEM 0x02u
-#define ___GFP_DMA32   0x04u
-#define ___GFP_MOVABLE 0x08u
-#define ___GFP_RECLAIMABLE 0x10u
-#define ___GFP_HIGH0x20u
-#define ___GFP_IO  0x40u
-#define ___GFP_FS  0x80u
-#define ___GFP_COLD0x100u
-#define ___GFP_NOWARN  0x200u
-#define ___GFP_REPEAT  0x400u
-#define ___GFP_NOFAIL  0x800u
-#define ___GFP_NORETRY 0x1000u
-#define ___GFP_MEMALLOC0x2000u
-#define ___GFP_COMP0x4000u
-#define ___GFP_ZERO0x8000u
-#define ___GFP_NOMEMALLOC  0x1u
-#define ___GFP_HARDWALL0x2u
-#define ___GFP_THISNODE0x4u
-#define ___GFP_ATOMIC  0x8u
-#define ___GFP_ACCOUNT 0x10u
-#define ___GFP_NOTRACK 0x20u
-#define ___GFP_DIRECT_RECLAIM  0x40u
-#define ___GFP_WRITE   0x80u
-#define ___GFP_KSWAPD_RECLAIM  0x100u
-/* If the above are modified, __GFP_BITS_SHIFT may need updating */
+#define ___GFP_DMA (1u << __GFP_DMA_SHIFT)
+#define ___GFP_HIGHMEM (1u << __GFP_HIGHMEM_SHIFT)
+#define ___GFP_DMA32   (1u << __GFP_DMA32_SHIFT)
+#define ___GFP_MOVABLE (1u << __GFP_MOVABLE_SHIFT)
+#define ___GFP_RECLAIMABLE (1u << __GFP_RECLAIMABLE_SHIFT)
+#define ___GFP_HIGH(1u << __GFP_HIGH_SHIFT)
+#define ___GFP_IO  (1u << __GFP_IO_SHIFT)
+#define ___GFP_FS  (1u << __GFP_FS_SHIFT)
+#define ___GFP_COLD(1u << __GFP_COLD_SHIFT)
+#define ___GFP_NOWARN  (1u << __GFP_NOWARN_SHIFT)
+#define ___GFP_REPEAT  (1u << __GFP_REPEAT_SHIFT)
+#define ___GFP_NOFAIL  (1u << __GFP_NOFAIL_SHIFT)
+#define ___GFP_NORETRY (1u << __GFP_NORETRY_SHIFT)
+#define ___GFP_MEMALLOC(1u << __GFP_MEMALLOC_SHIFT)
+#define ___GFP_COMP(1u << __GFP_COMP_SHIFT)
+#define ___GFP_ZERO(1u << __GFP_ZERO_SHIFT)
+#define ___GFP_NOMEMALLOC  (1u << __GFP_NOMEMALLOC_SHIFT)
+#define ___GFP_HARDWALL(1u << __GFP_HARDWALL_SHIFT)
+#define ___GFP_THISNODE(1u << __GFP_THISNODE_SHIFT)
+#define ___GFP_ATOMIC  (1u << __GFP_ATOMIC_SHIFT)
+#define ___GFP_ACCOUNT (1u << __GFP_ACCOUNT_SHIFT)
+#define ___GFP_NOTRACK (1u << __GFP_NOTRACK_SHIFT)
+#define ___GFP_DIRECT_RECLAIM  (1u << __GFP_DIRECT_RECLAIM_SHIFT)
+#define ___GFP_WRITE   (1u << __GFP_WRITE_SHIFT)
+#define ___GFP_KSWAPD_RECLAIM  (1u << __GFP_KSWAPD_RECLAIM_SHIFT)
 
 /*
  * Physical address zone modifiers (see linux/mmzone.h - low four bits)
@@ -180,7 +209,6 @@ struct vm_area_struct;
 #define __GFP_NOTRACK_FALSE_POSITIVE (__GFP_NOTRACK)
 
 /* Room for N __GFP_FOO bits */
-#define __GFP_BITS_SHIFT 25
 #define __GFP_BITS_MASK ((__force gfp_t)((1 << __GFP_BITS_SHIFT) - 1))
 
 /*
-- 
2.9.3

[PATCH 1/1] Remove hardcoding of ___GFP_xxx bitmasks

[Igor Stoppa]

The bitmasks used for ___GFP_xxx can be defined in terms of an enum,
which doesn't require manual updates to its values.

As bonus, __GFP_BITS_SHIFT is automatically kept consistent.

Signed-off-by: Igor Stoppa 
---
 include/linux/gfp.h | 82 +++--
 1 file changed, 55 insertions(+), 27 deletions(-)

diff --git a/include/linux/gfp.h b/include/linux/gfp.h
index 0fe0b62..2f894c5 100644
--- a/include/linux/gfp.h
+++ b/include/linux/gfp.h
@@ -14,33 +14,62 @@ struct vm_area_struct;
  * include/trace/events/mmflags.h and tools/perf/builtin-kmem.c
  */
 
+enum gfp_bitmask_shift {
+   __GFP_DMA_SHIFT = 0,
+   __GFP_HIGHMEM_SHIFT,
+   __GFP_DMA32_SHIFT,
+   __GFP_MOVABLE_SHIFT,
+   __GFP_RECLAIMABLE_SHIFT,
+   __GFP_HIGH_SHIFT,
+   __GFP_IO_SHIFT,
+   __GFP_FS_SHIFT,
+   __GFP_COLD_SHIFT,
+   __GFP_NOWARN_SHIFT,
+   __GFP_REPEAT_SHIFT,
+   __GFP_NOFAIL_SHIFT,
+   __GFP_NORETRY_SHIFT,
+   __GFP_MEMALLOC_SHIFT,
+   __GFP_COMP_SHIFT,
+   __GFP_ZERO_SHIFT,
+   __GFP_NOMEMALLOC_SHIFT,
+   __GFP_HARDWALL_SHIFT,
+   __GFP_THISNODE_SHIFT,
+   __GFP_ATOMIC_SHIFT,
+   __GFP_ACCOUNT_SHIFT,
+   __GFP_NOTRACK_SHIFT,
+   __GFP_DIRECT_RECLAIM_SHIFT,
+   __GFP_WRITE_SHIFT,
+   __GFP_KSWAPD_RECLAIM_SHIFT,
+   __GFP_BITS_SHIFT
+};
+
+
 /* Plain integer GFP bitmasks. Do not use this directly. */
-#define ___GFP_DMA 0x01u
-#define ___GFP_HIGHMEM 0x02u
-#define ___GFP_DMA32   0x04u
-#define ___GFP_MOVABLE 0x08u
-#define ___GFP_RECLAIMABLE 0x10u
-#define ___GFP_HIGH0x20u
-#define ___GFP_IO  0x40u
-#define ___GFP_FS  0x80u
-#define ___GFP_COLD0x100u
-#define ___GFP_NOWARN  0x200u
-#define ___GFP_REPEAT  0x400u
-#define ___GFP_NOFAIL  0x800u
-#define ___GFP_NORETRY 0x1000u
-#define ___GFP_MEMALLOC0x2000u
-#define ___GFP_COMP0x4000u
-#define ___GFP_ZERO0x8000u
-#define ___GFP_NOMEMALLOC  0x1u
-#define ___GFP_HARDWALL0x2u
-#define ___GFP_THISNODE0x4u
-#define ___GFP_ATOMIC  0x8u
-#define ___GFP_ACCOUNT 0x10u
-#define ___GFP_NOTRACK 0x20u
-#define ___GFP_DIRECT_RECLAIM  0x40u
-#define ___GFP_WRITE   0x80u
-#define ___GFP_KSWAPD_RECLAIM  0x100u
-/* If the above are modified, __GFP_BITS_SHIFT may need updating */
+#define ___GFP_DMA (1u << __GFP_DMA_SHIFT)
+#define ___GFP_HIGHMEM (1u << __GFP_HIGHMEM_SHIFT)
+#define ___GFP_DMA32   (1u << __GFP_DMA32_SHIFT)
+#define ___GFP_MOVABLE (1u << __GFP_MOVABLE_SHIFT)
+#define ___GFP_RECLAIMABLE (1u << __GFP_RECLAIMABLE_SHIFT)
+#define ___GFP_HIGH(1u << __GFP_HIGH_SHIFT)
+#define ___GFP_IO  (1u << __GFP_IO_SHIFT)
+#define ___GFP_FS  (1u << __GFP_FS_SHIFT)
+#define ___GFP_COLD(1u << __GFP_COLD_SHIFT)
+#define ___GFP_NOWARN  (1u << __GFP_NOWARN_SHIFT)
+#define ___GFP_REPEAT  (1u << __GFP_REPEAT_SHIFT)
+#define ___GFP_NOFAIL  (1u << __GFP_NOFAIL_SHIFT)
+#define ___GFP_NORETRY (1u << __GFP_NORETRY_SHIFT)
+#define ___GFP_MEMALLOC(1u << __GFP_MEMALLOC_SHIFT)
+#define ___GFP_COMP(1u << __GFP_COMP_SHIFT)
+#define ___GFP_ZERO(1u << __GFP_ZERO_SHIFT)
+#define ___GFP_NOMEMALLOC  (1u << __GFP_NOMEMALLOC_SHIFT)
+#define ___GFP_HARDWALL(1u << __GFP_HARDWALL_SHIFT)
+#define ___GFP_THISNODE(1u << __GFP_THISNODE_SHIFT)
+#define ___GFP_ATOMIC  (1u << __GFP_ATOMIC_SHIFT)
+#define ___GFP_ACCOUNT (1u << __GFP_ACCOUNT_SHIFT)
+#define ___GFP_NOTRACK (1u << __GFP_NOTRACK_SHIFT)
+#define ___GFP_DIRECT_RECLAIM  (1u << __GFP_DIRECT_RECLAIM_SHIFT)
+#define ___GFP_WRITE   (1u << __GFP_WRITE_SHIFT)
+#define ___GFP_KSWAPD_RECLAIM  (1u << __GFP_KSWAPD_RECLAIM_SHIFT)
 
 /*
  * Physical address zone modifiers (see linux/mmzone.h - low four bits)
@@ -180,7 +209,6 @@ struct vm_area_struct;
 #define __GFP_NOTRACK_FALSE_POSITIVE (__GFP_NOTRACK)
 
 /* Room for N __GFP_FOO bits */
-#define __GFP_BITS_SHIFT 25
 #define __GFP_BITS_MASK ((__force gfp_t)((1 << __GFP_BITS_SHIFT) - 1))
 
 /*
-- 
2.9.3