Date:	Mon, 30 Apr 2007 17:30:31 +0530
From:	"Satyam Sharma" <satyam.sharma@...il.com>
To:	"Robert P. J. Day" <rpjday@...dspring.com>
Cc:	"Jan Engelhardt" <jengelh@...ux01.gwdg.de>,
	"Andrew Morton" <akpm@...ux-foundation.org>,
	"Linux Kernel Mailing List" <linux-kernel@...r.kernel.org>
Subject: Re: can a kmalloc be both GFP_ATOMIC and GFP_KERNEL at the same time?

On 4/30/07, Robert P. J. Day <rpjday@...dspring.com> 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 majordomo@...r.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Powered by blists - more mailing lists