Re: [PATCH]: Clean up of __alloc_pages

Paul Jackson
2005-10-30T01:47:28Z

A couple more items:
1) Lets try for a consistent use of type "gfp_t" for gfp_mask.
2) The can_try_harder flag values were driving me nuts.
3) The "inline" you added to buffered_rmqueue() blew up my compile.
4) The return from try_to_free_pages() was put in "i" for no evident reason.
5) I have no clue what the replenish flag you added to buffered_rmqueue does.

You patch has:
> can_try_harder can have following
> * values:
> * -1 => No need to check for the watermarks.
> * 0 => Don't go too low down in deeps below the low watermark (GFP_HIGH)
> * 1 => Go far below the low watermark. See zone_watermark_ok (RT TASK)

Later on, you have an inequality test on this value:
if ((can_try_harder >= 0)
and a non-zero test:
if (can_try_harder)

That's three magic values, not even in increasing order of "how hard
one should try", tested a couple of different ways that requires
absorbing the complete details of the three values and their ordering
before one can read the code.

I'd like to use a enum type, to put names to these 3 values, and I'd
like to put them in order of increasing desperation.

The following patch tweaks the gfp_mask type, makes can_try_harder
an enum, removes the inline from buffered_rmqueue(), doesn't stash
the return value of try_to_free_pages(), and removes buffered_rmqueue's
replenish flag. It applies on top of your patch. If you like it, I'd
be happiest if you picked it up and incorporated it into your work.

I compiled it once, never tested or booted.

Signed-off-by: Paul Jackson <pj@sgi.com>

---

include/linux/mmzone.h | 9 ++++++++-
mm/page_alloc.c | 39 +++++++++++++++++++--------------------
mm/vmscan.c | 9 ++++++---
3 files changed, 33 insertions(+), 24 deletions(-)

--- 2.6.14-rc5-mm1-cpuset-patches.orig/include/linux/mmzone.h 2005-10-25 09:37:45.154588634 -0700
+++ 2.6.14-rc5-mm1-cpuset-patches/include/linux/mmzone.h 2005-10-29 17:58:28.433716118 -0700
@@ -330,8 +330,15 @@ void get_zone_counts(unsigned long *acti
unsigned long *free);
void build_all_zonelists(void);
void wakeup_kswapd(struct zone *zone, int order);
+
+typedef enum {
+ dont_try_harder, /* Don't go much below low watermark (GFP_HIGH) */
+ try_harder, /* Go far below low watermark (RT TASK) */
+ try_really_hard /* Ignore all watermarks */
+} can_try_harder_t;
+
int zone_watermark_ok(struct zone *z, int order, unsigned long mark,
- int alloc_type, int can_try_harder, int gfp_high);
+ int alloc_type, can_try_harder_t can_try_harder, int gfp_high);

#ifdef CONFIG_HAVE_MEMORY_PRESENT
void memory_present(int nid, unsigned long start, unsigned long end);
--- 2.6.14-rc5-mm1-cpuset-patches.orig/mm/page_alloc.c 2005-10-29 14:44:55.371576062 -0700
+++ 2.6.14-rc5-mm1-cpuset-patches/mm/page_alloc.c 2005-10-29 18:40:41.358953388 -0700
@@ -713,8 +713,8 @@ static inline void prep_zero_page(struct
* we cheat by calling it from here, in the order > 0 path. Saves a branch
* or two.
*/
-struct inline page *
-buffered_rmqueue(struct zone *zone, int order, gfp_t gfp_flags, int replenish)
+struct page *
+buffered_rmqueue(struct zone *zone, int order, gfp_t gfp_flags)
{
unsigned long flags;
struct page *page = NULL;
@@ -725,7 +725,7 @@ buffered_rmqueue(struct zone *zone, int

pcp = &zone_pcp(zone, get_cpu())->pcp[cold];
local_irq_save(flags);
- if ((pcp->count <= pcp->low) && replenish)
+ if (pcp->count <= pcp->low)
pcp->count += rmqueue_bulk(zone, 0,
pcp->batch, &pcp->list);
if (pcp->count) {
@@ -756,16 +756,12 @@ buffered_rmqueue(struct zone *zone, int
}

/* get_page_from_freeliest loops through all the possible zones
- * to find out if it can allocate a page. can_try_harder can have following
- * values:
- * -1 => No need to check for the watermarks.
- * 0 => Don't go too low down in deeps below the low watermark (GFP_HIGH)
- * 1 => Go far below the low watermark. See zone_watermark_ok (RT TASK)
+ * to find out if it can allocate a page.
*/

static struct page *
-get_page_from_freelist(unsigned int __nocast gfp_mask, unsigned int order,
- struct zone **zones, int can_try_harder)
+get_page_from_freelist(gfp_t gfp_mask, unsigned int order,
+ struct zone **zones, can_try_harder_t can_try_harder)
{
struct zone *z;
struct page *page = NULL;
@@ -780,13 +776,13 @@ get_page_from_freelist(unsigned int __no
if (!cpuset_zone_allowed(z, gfp_mask))
continue;

- if ((can_try_harder >= 0) &&
+ if ((can_try_harder < try_really_hard) &&
(!zone_watermark_ok(z, order, z->pages_low,
classzone_idx, can_try_harder,
gfp_mask & __GFP_HIGH)))
continue;

- page = buffered_rmqueue(z, order, gfp_mask, 1);
+ page = buffered_rmqueue(z, order, gfp_mask);
if (page)
break;
}
@@ -798,7 +794,7 @@ get_page_from_freelist(unsigned int __no
* of the allocation.
*/
int zone_watermark_ok(struct zone *z, int order, unsigned long mark,
- int classzone_idx, int can_try_harder, int gfp_high)
+ int classzone_idx, can_try_harder_t can_try_harder, int gfp_high)
{
/* free_pages my go negative - that's OK */
long min = mark, free_pages = z->free_pages - (1 << order) + 1;
@@ -806,7 +802,7 @@ int zone_watermark_ok(struct zone *z, in

if (gfp_high)
min -= min / 2;
- if (can_try_harder)
+ if (can_try_harder >= try_harder)
min -= min / 4;

if (free_pages <= min + z->lowmem_reserve[classzone_idx])
@@ -878,7 +874,7 @@ static inline void __stack_trace(struct
}

static inline void set_page_owner(struct page *page,
- unsigned int order, unsigned int gfp_mask)
+ unsigned int order, gfp_t gfp_mask)
{
unsigned long address, bp;
#ifdef CONFIG_X86_64
@@ -906,7 +902,7 @@ __alloc_pages(gfp_t gfp_mask, unsigned i
struct task_struct *p = current;
int i;
int do_retry;
- int can_try_harder;
+ can_try_harder_t can_try_harder = dont_try_harder;

might_sleep_if(wait);

@@ -915,7 +911,8 @@ __alloc_pages(gfp_t gfp_mask, unsigned i
* cannot run direct reclaim, or is the caller has realtime scheduling
* policy
*/
- can_try_harder = (unlikely(rt_task(p)) && !in_interrupt()) || !wait;
+ if ((unlikely(rt_task(p)) && !in_interrupt()) || !wait)
+ can_try_harder = try_harder;

zones = zonelist->zones; /* the list of zones suitable for gfp_mask */

@@ -924,7 +921,8 @@ __alloc_pages(gfp_t gfp_mask, unsigned i
return NULL;
}
restart:
- page = get_page_from_freelist(gfp_mask|__GFP_HARDWALL, order, zones, 0);
+ page = get_page_from_freelist(gfp_mask|__GFP_HARDWALL, order, zones,
+ dont_try_harder);
if (page)
goto got_pg;

@@ -951,7 +949,8 @@ restart:
&& !in_interrupt()) {
if (!(gfp_mask & __GFP_NOMEMALLOC)) {
/* go through the zonelist yet again, ignoring mins */
- page = get_page_from_freelist(gfp_mask, order, zones, -1);
+ page = get_page_from_freelist(gfp_mask, order, zones,
+ try_really_hard);
if (page)
goto got_pg;
}
@@ -970,7 +969,7 @@ rebalance:
reclaim_state.reclaimed_slab = 0;
p->reclaim_state = &reclaim_state;

- i = try_to_free_pages(zones, gfp_mask);
+ try_to_free_pages(zones, gfp_mask);

p->reclaim_state = NULL;
p->flags &= ~PF_MEMALLOC;
--- 2.6.14-rc5-mm1-cpuset-patches.orig/mm/vmscan.c 2005-10-25 09:38:47.225573361 -0700
+++ 2.6.14-rc5-mm1-cpuset-patches/mm/vmscan.c 2005-10-29 17:21:39.268886029 -0700
@@ -1082,7 +1082,8 @@ loop_again:
continue;

if (!zone_watermark_ok(zone, order,
- zone->pages_high, 0, 0, 0)) {
+ zone->pages_high, 0, 0,
+ dont_try_harder)) {
end_zone = i;
goto scan;
}
@@ -1119,7 +1120,8 @@ scan:

if (nr_pages == 0) { /* Not software suspend */
if (!zone_watermark_ok(zone, order,
- zone->pages_high, end_zone, 0, 0))
+ zone->pages_high, end_zone,
+ 0, dont_try_harder))
all_zones_ok = 0;
}
zone->temp_priority = priority;
@@ -1267,7 +1269,8 @@ void wakeup_kswapd(struct zone *zone, in
return;

pgdat = zone->zone_pgdat;
- if (zone_watermark_ok(zone, order, zone->pages_low, 0, 0, 0))
+ if (zone_watermark_ok(zone, order, zone->pages_low, 0, 0,
+ dont_try_harder))
return;
if (pgdat->kswapd_max_order < order)
pgdat->kswapd_max_order = order;

--
I won't rest till it's the best ...
Programmer, Linux Scalability
Paul Jackson <pj@sgi.com> 1.925.600.0401