Messages in this thread | | | Date | Sun, 29 Jun 2008 07:16:49 +0100 (BST) | From | Hugh Dickins <> | Subject | Re: [PATCH trivial] block: GFP_ATOMIC is __GFP_HIGH |
| |
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
| |