Re: [PATCH 1/6] CKRM: Basic changes to the core kernel

Dave Hansen
2005-04-04T13:45:19Z

> static inline void
> add_page_to_active_list(struct zone *zone, struct page *page)
> {
> list_add(&page->lru, &zone->active_list);
> zone->nr_active++;
> + ckrm_mem_inc_active(page);
> }

Are any of the current zone statistics used any more when this is
compiled in?

Also, why does everything have to say ckrm_* on it? What if somebody
else comes along and wants to use the same functions to do some other
kind of accounting?

I think names like this are plenty long and descriptive enough:

mem_inc_active(page);
clear_page_class(page);
set_page_class(...);

I'd drop the "ckrm_".

> +#define PG_ckrm_account 21 /* CKRM accounting */

Are you sure you really need this bit *and* a whole new pointer in
'struct page'? We already do some tricks with ->mapping so that we can
tell what is stored in it. You could easily do something with the low
bit of your new structure member.

> @@ -355,6 +356,7 @@ free_pages_bulk(struct zone *zone, int c
> /* have to delete it as __free_pages_bulk list manipulates */
> list_del(&page->lru);
> __free_pages_bulk(page, zone, order);
> + ckrm_clear_page_class(page);
> ret++;
> }
> spin_unlock_irqrestore(&zone->lock, flags);

When your option is on, how costly is the addition of code, here? How
much does it hurt the microbenchmarks? How much larger does it
make .text?

> + if (!in_interrupt() && !ckrm_class_limit_ok(ckrm_get_mem_class(p)))
> + return NULL;

ckrm_class_limit_ok() is called later on in the same hot path, and
there's a for loop in there over each zone. How expensive is this on
SGI's machines? What about an 8-node x44[05]? Why can't you call it
from interrupts?

-- Dave