On 4/30/07, Robert P. J. Day <[email protected]> wrote:
On Mon, 30 Apr 2007, Jan Engelhardt wrote:
>
> >> > drivers/scsi/aic7xxx_old.c: aic_dev = kmalloc(sizeof(struct aic_dev_data), GFP_ATOMIC | GFP_KERNEL);
> >> > drivers/message/i2o/device.c: resblk = kmalloc(buflen + 8, GFP_KERNEL | GFP_ATOMIC);
> >> >
> >> > clarification?
> >>
> >> GFP_ATOMIC implies that the memory comes from the zones which
> >> GFP_KERNEL also uses. So the above usage of GFP_KERNEL is redundant
> >> and should be removed.
>
> include/linux/gfp.h:
> #define GFP_ATOMIC (__GFP_HIGH)
> #define GFP_KERNEL (__GFP_WAIT | __GFP_IO | __GFP_FS)
>
> So combining GFP_ATOMIC with GFP_KERNEL gives you
> "allow io, allow fs, allow waiting, and use emergency pools when it's getting
> tight"
> which to me looks like a valid, but probably unwanted combination.
Yes. This all appears to be a case of some unfortunate naming used
here. GFP_KERNEL (== GFP_that_can_sleep, as defined currently) clearly
*cannot* go with GFP_ATOMIC (== GFP_that_cannot_sleep, for atomic
contexts, which is why GFP_ATOMIC exists). But the way these are
defined presently means that their combination is *not* invalid
(although of dubious usability).
As a matter of style, the author there could've written (GFP_KERNEL |
__GFP_HIGH) instead, but I'm not sure how much sense that makes
because once we specify GFP_KERNEL, we essentially bring out the heavy
weaponry to *make* some free space for ourselves if it isn't there
already (and we're prepared to sleep when all that happens) -- so
where's the need left to scavenge into emergency pools anymore?
__GFP_HIGH only makes sense for poor atomic contexts for whom sleeping
(when we try_to_free other pages to satisfy our allocation request) is
not an option, which is precisely how things are presently.
at this point, maybe i'll just leave this in the hands of those who
know far more about it than i do. but, while we're here, are there
any *other* combinations that wouldn't make any sense? might as well
check for those in my cleanup script as well.
What combinations make sense for a particular user must be left to him
and his usage context. So although (GFP_KERNEL | GFP_ATOMIC) does not
make sense in the way these are generally used (or were invented for),
but the combination *as defined presently* is not "invalid". I'm not
sure whether we really need to bother with putting in any checks in
__alloc_pages() -- this is only nonsensical usage at worst, not a bug.
p.s. as a suggestion that borders on overkill, one could always add a
configuration debugging option that, when set, compiles code into
kmalloc() that does a sanity check on its type flag arguments.
Eek.
> >hang on ... based on an email i just got, is that reference to
> >GFP_KERNEL "redundant" or "conflicting"? big difference there. and
> >is the proper fix to remove "GFP_KERNEL" in both cases?
Not necessarily. I haven't looked around that code you mentioned, but
the solution is pretty simple:
1. If you're in_atomic() context, that GFP_KERNEL is a BUG and *must*
be removed.
2. If not, that GFP_ATOMIC is redundant / nonsensical and *can* be removed.
-
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to [email protected]
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
[Index of Archives]
[Kernel Newbies]
[Netfilter]
[Bugtraq]
[Photo]
[Stuff]
[Gimp]
[Yosemite News]
[MIPS Linux]
[ARM Linux]
[Linux Security]
[Linux RAID]
[Video 4 Linux]
[Linux for the blind]
[Linux Resources]
