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/