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

Chandra Seetharaman
2005-04-05T17:30:59Z

On Mon, Apr 04, 2005 at 06:45:13AM -0700, Dave Hansen wrote:
> > 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?

They are being used. The reason I left them is that if you want those
statictics with just ckrm info, we need to go thru all the defined classes,
which might be costly, depend on the number of classes.
>
> 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_".

Because we got some review comments to keep it that way :).... Currently
they do ckrm specific things. In future if that changes, we can change the
name too.

>
> > +#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.

I think I canavoid using the bit. The problem with having a pointer in page
data structure is two-fold:
1. goes over the page-cahe (I ran cache-bench with mem controller
enabled, and didn't see much of a difference. will post the
new results sometime soon)
2. additional memory used, especially in large systems

Using the mapping logic, we can avoid problem (1), but increase problem (2)
with added complexity and run-time logic. I am looking a way to avoid both
the problems, any help appreciated.

>
> > @@ -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

As I said earlier cache-bench doesn't show much effect. Will post that and
other results sometime soon.
> make .text?
------------------ 2612-rc1.... no memory controller patch applied
vmlinux-nomem: file format elf32-i386

Sections:
Idx Name Size VMA LMA File off Algn
0 .text 002455d5 c0100000 c0100000 00001000 2**4
CONTENTS, ALLOC, LOAD, READONLY, CODE
------------------ 2612-rc1.... mem ctlr patch applied, config turned off
vmlinux-mem_out: file format elf32-i386

Sections:
Idx Name Size VMA LMA File off Algn
0 .text 00245575 c0100000 c0100000 00001000 2**4
CONTENTS, ALLOC, LOAD, READONLY, CODE
------------------ 2612-rc1.... mem ctlr patch applied, config turned on
vmlinux-mem_in: file format elf32-i386

Sections:
Idx Name Size VMA LMA File off Algn
0 .text 00248195 c0100000 c0100000 00001000 2**4
CONTENTS, ALLOC, LOAD, READONLY, CODE
------------------

>
> > + 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

It doesn't get into the for loop unless the class is over the limit(which
is not a frequent event). Also, the loop is just to wakeup kswapd once..
may be I can get rid of that and use pgdat_list directly.

> SGI's machines? What about an 8-node x44[05]? Why can't you call it
> from interrupts?

I just wanted to avoid limit related failures in interrupt context, as it
might lead to wierd problems.
>
> -- Dave