[PATCH trivial] block: GFP_ATOMIC is __GFP_HIGH

[PATCH trivial] block: GFP_ATOMIC is __GFP_HIGH

[Hugh Dickins]

GFP_ATOMIC is __GFP_HIGH: no need for alloc_io_context() to add that.

Signed-off-by: Hugh Dickins <hugh@veritas.com>
---

 block/blk-ioc.c |    2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

--- linux-next/block/blk-ioc.c	2008-05-12 02:01:05.000000000 +0100
+++ linux/block/blk-ioc.c	2008-06-27 14:08:00.000000000 +0100
@@ -98,7 +98,7 @@ struct io_context *alloc_io_context(gfp_
 		ret->last_waited = jiffies; /* doesn't matter... */
 		ret->nr_batch_requests = 0; /* because this is 0 */
 		ret->aic = NULL;
-		INIT_RADIX_TREE(&ret->radix_root, GFP_ATOMIC | __GFP_HIGH);
+		INIT_RADIX_TREE(&ret->radix_root, GFP_ATOMIC);
 		INIT_HLIST_HEAD(&ret->cic_list);
 		ret->ioc_data = NULL;
 	}

Re: [PATCH trivial] block: GFP_ATOMIC is __GFP_HIGH

[Eduard - Gabriel Munteanu]

On Sun, 29 Jun 2008 00:54:27 +0100 (BST)
Hugh Dickins <hugh@veritas.com> wrote:

> GFP_ATOMIC is __GFP_HIGH: no need for alloc_io_context() to add that.
> 
> Signed-off-by: Hugh Dickins <hugh@veritas.com>
> ---
> 
>  block/blk-ioc.c |    2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> --- linux-next/block/blk-ioc.c	2008-05-12 02:01:05.000000000
> +0100 +++ linux/block/blk-ioc.c	2008-06-27 14:08:00.000000000
> +0100 @@ -98,7 +98,7 @@ struct io_context *alloc_io_context(gfp_
>  		ret->last_waited = jiffies; /* doesn't matter... */
>  		ret->nr_batch_requests = 0; /* because this is 0 */
>  		ret->aic = NULL;
> -		INIT_RADIX_TREE(&ret->radix_root, GFP_ATOMIC |
> __GFP_HIGH);
> +		INIT_RADIX_TREE(&ret->radix_root, GFP_ATOMIC);
>  		INIT_HLIST_HEAD(&ret->cic_list);
>  		ret->ioc_data = NULL;
>  	}

Hi,

I'm not sure this is a good idea: GFP_ATOMIC and __GFP_HIGH are
semantically different, even though they are equivalent at the moment.
Have you seen GFP_NOWAIT's definition?
/* This equals 0, but use constants in case they ever change */
#define GFP_NOWAIT      (GFP_ATOMIC & ~__GFP_HIGH)

I think it's best to look at what that code intends to do, not at what
it does at the moment. Definitions for gfp flags might change in the
future.

If the code does not _semantically_ need __GFP_HIGH, then your commit
message should indicate so, rather than comparing it with GFP_ATOMIC.


	Cheers,
	Eduard

Re: [PATCH trivial] block: GFP_ATOMIC is __GFP_HIGH

[Hugh Dickins]

On Sun, 29 Jun 2008, Eduard - Gabriel Munteanu wrote:
> On Sun, 29 Jun 2008 00:54:27 +0100 (BST)
> Hugh Dickins <hugh@veritas.com> wrote:
> 
> > GFP_ATOMIC is __GFP_HIGH: no need for alloc_io_context() to add that.
> > 
> > -		INIT_RADIX_TREE(&ret->radix_root, GFP_ATOMIC |
> > __GFP_HIGH);
> > +		INIT_RADIX_TREE(&ret->radix_root, GFP_ATOMIC);
> 
> I'm not sure this is a good idea: GFP_ATOMIC and __GFP_HIGH are
> semantically different, even though they are equivalent at the moment.
> Have you seen GFP_NOWAIT's definition?
> /* This equals 0, but use constants in case they ever change */
> #define GFP_NOWAIT      (GFP_ATOMIC & ~__GFP_HIGH)
> 
> I think it's best to look at what that code intends to do, not at what
> it does at the moment. Definitions for gfp flags might change in the
> future.
> 
> If the code does not _semantically_ need __GFP_HIGH, then your commit
> message should indicate so, rather than comparing it with GFP_ATOMIC.

I disagree.  It is somewhat accidental that GFP_ATOMIC sets no other
bit than __GFP_HIGH - there might have been a __GFP_ATOMIC bit - which
is why the GFP_NOWAIT definition makes some sense; but it is not
accidental that GFP_ATOMIC includes __GFP_HIGH - it's precisely when
we're atomic that we need access to those extra reserves; and where
we don't actually want them then we do say GFP_NOWAIT not GFP_ATOMIC.

I expect the gfp flags will change in the future; but unless I missed
somewhere, amongst all the places which specify GFP_ATOMIC throughout
the kernel, this is the only one which ors in __GFP_HIGH too.  I don't
believe it expected access to extra extra reserves!  So I thought we'd
do best to remove the anomaly.

(But what I'd actually intended to grep for was __GFP_HIGHMEM.)

Hugh

Re: [PATCH trivial] block: GFP_ATOMIC is __GFP_HIGH

[Eduard - Gabriel Munteanu]

On Sun, 29 Jun 2008 07:16:49 +0100 (BST)
Hugh Dickins <hugh@veritas.com> wrote:

> but it is not
> accidental that GFP_ATOMIC includes __GFP_HIGH - it's precisely when
> we're atomic that we need access to those extra reserves; and where
> we don't actually want them then we do say GFP_NOWAIT not GFP_ATOMIC.

I would expect GFP_ATOMIC just prevents sleeping, while it _could_ fail
(in theory) unless it is allowed to touch the emergency pools.

Actually, in many/most atomic contexts bail-out paths are possible for
allocation failures. And many/most of these atomic contexts have no
special reason to require emergency memory. Think about the usual
allocations enclosed within spinlocks.

> I expect the gfp flags will change in the future; but unless I missed
> somewhere, amongst all the places which specify GFP_ATOMIC throughout
> the kernel, this is the only one which ors in __GFP_HIGH too.  I don't
> believe it expected access to extra extra reserves!  So I thought we'd
> do best to remove the anomaly.

Yes, it seems this is the only place where this occurs.

Although I did not read all the code and resolved its implications, it
seems like it actually needs something like __GFP_NOFAIL (?) instead of
__GFP_HIGH. The slab itself is created with SLAB_PANIC.

> (But what I'd actually intended to grep for was __GFP_HIGHMEM.)
> 
> Hugh


	Eduard

Re: [PATCH trivial] block: GFP_ATOMIC is __GFP_HIGH

[Jens Axboe]

On Sun, Jun 29 2008, Eduard - Gabriel Munteanu wrote:
> On Sun, 29 Jun 2008 07:16:49 +0100 (BST)
> Hugh Dickins <hugh@veritas.com> wrote:
> 
> > but it is not
> > accidental that GFP_ATOMIC includes __GFP_HIGH - it's precisely when
> > we're atomic that we need access to those extra reserves; and where
> > we don't actually want them then we do say GFP_NOWAIT not GFP_ATOMIC.
> 
> I would expect GFP_ATOMIC just prevents sleeping, while it _could_ fail
> (in theory) unless it is allowed to touch the emergency pools.
> 
> Actually, in many/most atomic contexts bail-out paths are possible for
> allocation failures. And many/most of these atomic contexts have no
> special reason to require emergency memory. Think about the usual
> allocations enclosed within spinlocks.

I have to agree with Eduard here - GFP_ATOMIC means "don't block"
primarily, whether it has a given priority or not is something you would
have to look up. So it's more readable with the __GFP_HIGH manually
added.

> > I expect the gfp flags will change in the future; but unless I missed
> > somewhere, amongst all the places which specify GFP_ATOMIC throughout
> > the kernel, this is the only one which ors in __GFP_HIGH too.  I don't
> > believe it expected access to extra extra reserves!  So I thought we'd
> > do best to remove the anomaly.
> 
> Yes, it seems this is the only place where this occurs.
> 
> Although I did not read all the code and resolved its implications, it
> seems like it actually needs something like __GFP_NOFAIL (?) instead of
> __GFP_HIGH. The slab itself is created with SLAB_PANIC.

It's not a big deal, it'll recover fine.

-- 
Jens Axboe