Re: [PATCH 21/25] Use allocation flags as an index to the zone watermark

Mel Gorman
2009-04-22T10:21:05Z

On Wed, Apr 22, 2009 at 09:26:10AM +0900, KOSAKI Motohiro wrote:
> > ALLOC_WMARK_MIN, ALLOC_WMARK_LOW and ALLOC_WMARK_HIGH determin whether
> > pages_min, pages_low or pages_high is used as the zone watermark when
> > allocating the pages. Two branches in the allocator hotpath determine which
> > watermark to use. This patch uses the flags as an array index and places
> > the three watermarks in a union with an array so it can be offset. This
> > means the flags can be used as an array index and reduces the branches
> > taken.
> >
> > Signed-off-by: Mel Gorman <mel@csn.ul.ie>
> > Reviewed-by: Christoph Lameter <cl@linux-foundation.org>
> > ---
> > include/linux/mmzone.h | 8 +++++++-
> > mm/page_alloc.c | 18 ++++++++----------
> > 2 files changed, 15 insertions(+), 11 deletions(-)
> >
> > diff --git a/include/linux/mmzone.h b/include/linux/mmzone.h
> > index f82bdba..c1fa208 100644
> > --- a/include/linux/mmzone.h
> > +++ b/include/linux/mmzone.h
> > @@ -275,7 +275,13 @@ struct zone_reclaim_stat {
> >
> > struct zone {
> > /* Fields commonly accessed by the page allocator */
> > - unsigned long pages_min, pages_low, pages_high;
> > + union {
> > + struct {
> > + unsigned long pages_min, pages_low, pages_high;
> > + };
> > + unsigned long pages_mark[3];
> > + };
> > +
>
> hmmm... I don't like union hack.
> Why can't we change all caller to use page_mark?
>

Because pages_min, pages_low and pages_high are such well understood concepts
and their current use is easy to userstand. It could all be changed to getters
and setters with a patch to update all call sites or having symbolic names
for the index and forcing the use of the array but when I started doing that,
the code looked worse to my eye, not better. The union is a relatively
small hack but well contained within the one place that cares.

> > /*
> > * We don't know if the memory that we're going to allocate will be freeable
> > * or/and it will be released eventually, so to avoid totally wasting several
> > diff --git a/mm/page_alloc.c b/mm/page_alloc.c
> > index 376d848..e61867e 100644
> > --- a/mm/page_alloc.c
> > +++ b/mm/page_alloc.c
> > @@ -1157,10 +1157,13 @@ failed:
> > return NULL;
> > }
> >
> > -#define ALLOC_NO_WATERMARKS 0x01 /* don't check watermarks at all */
> > -#define ALLOC_WMARK_MIN 0x02 /* use pages_min watermark */
> > -#define ALLOC_WMARK_LOW 0x04 /* use pages_low watermark */
> > -#define ALLOC_WMARK_HIGH 0x08 /* use pages_high watermark */
> > +/* The WMARK bits are used as an index zone->pages_mark */
> > +#define ALLOC_WMARK_MIN 0x00 /* use pages_min watermark */
> > +#define ALLOC_WMARK_LOW 0x01 /* use pages_low watermark */
> > +#define ALLOC_WMARK_HIGH 0x02 /* use pages_high watermark */
> > +#define ALLOC_NO_WATERMARKS 0x08 /* don't check watermarks at all */
> > +#define ALLOC_WMARK_MASK 0x07 /* Mask to get the watermark bits */
>
> the mask only use two bit. but mask definition is three bit (0x07), why?
>

I was thinking ALLOC_NO_WATERMARKS-1 and that all the lower bits must be
cleared and left the value of 0x08 to occupy same number of bits even though
that wasn't necessary. As suggested I'll change this to

ALLOC_NO_WATERMARKS 0x04
WMARK_MASK (1-ALLOC_NO_WATERMARKS)

>
> > +
> > #define ALLOC_HARDER 0x10 /* try to alloc harder */
> > #define ALLOC_HIGH 0x20 /* __GFP_HIGH set */
> > #ifdef CONFIG_CPUSETS
> > @@ -1463,12 +1466,7 @@ zonelist_scan:
> >
> > if (!(alloc_flags & ALLOC_NO_WATERMARKS)) {
> > unsigned long mark;
> > - if (alloc_flags & ALLOC_WMARK_MIN)
> > - mark = zone->pages_min;
> > - else if (alloc_flags & ALLOC_WMARK_LOW)
> > - mark = zone->pages_low;
> > - else
> > - mark = zone->pages_high;
> > + mark = zone->pages_mark[alloc_flags & ALLOC_WMARK_MASK];
> > if (!zone_watermark_ok(zone, order, mark,
> > classzone_idx, alloc_flags)) {
> > if (!zone_reclaim_mode ||
> > --
> > 1.5.6.5
> >
>
>
>

--
Mel Gorman
Part-time Phd Student Linux Technology Center
University of Limerick IBM Dublin Software Lab