Messages in this thread | | | Date | Mon, 26 Sep 2011 19:44:40 -0300 | From | Glauber Costa <> | Subject | Re: [PATCH v3 1/7] Basic kernel memory functionality for the Memory Controller |
| |
>> #endif >> >> - >> +#ifdef CONFIG_CGROUP_MEM_RES_CTLR_KMEM >> +int do_kmem_account __read_mostly = 1; >> +#else >> +#define do_kmem_account 0 >> +#endif > > > Hmm, do we really need this boot option ? > From my experience to have swap-accounting boot option, > this scares us ;) I think config is enough.
If no one else wants it, I can remove it. I personally don't need it, just wanted to follow the convention laid down by swap here.
> > > >> /* >> * Statistics for memory cgroup. >> */ >> @@ -270,6 +274,10 @@ struct mem_cgroup { >> */ >> struct res_counter memsw; >> /* >> + * the counter to account for kmem usage. >> + */ >> + struct res_counter kmem; >> + /* >> * Per cgroup active and inactive list, similar to the >> * per zone LRU lists. >> */ >> @@ -321,6 +329,11 @@ struct mem_cgroup { >> */ >> unsigned long move_charge_at_immigrate; >> /* >> + * Should kernel memory limits be stabilished independently >> + * from user memory ? >> + */ >> + int kmem_independent; >> + /* >> * percpu counter. >> */ >> struct mem_cgroup_stat_cpu *stat; >> @@ -388,9 +401,14 @@ enum charge_type { >> }; >> >> /* for encoding cft->private value on file */ >> -#define _MEM (0) >> -#define _MEMSWAP (1) >> -#define _OOM_TYPE (2) >> + >> +enum mem_type { >> + _MEM = 0, >> + _MEMSWAP, >> + _OOM_TYPE, >> + _KMEM, >> +}; >> + > > ok, nice clean up. > > >> #define MEMFILE_PRIVATE(x, val) (((x)<< 16) | (val)) >> #define MEMFILE_TYPE(val) (((val)>> 16)& 0xffff) >> #define MEMFILE_ATTR(val) ((val)& 0xffff) >> @@ -3943,10 +3961,15 @@ static inline u64 mem_cgroup_usage(struct mem_cgroup *mem, bool swap) >> u64 val; >> >> if (!mem_cgroup_is_root(mem)) { >> + val = 0; >> + if (!mem->kmem_independent) >> + val = res_counter_read_u64(&mem->kmem, RES_USAGE); > >> if (!swap) >> - return res_counter_read_u64(&mem->res, RES_USAGE); >> + val += res_counter_read_u64(&mem->res, RES_USAGE); >> else >> - return res_counter_read_u64(&mem->memsw, RES_USAGE); >> + val += res_counter_read_u64(&mem->memsw, RES_USAGE); >> + >> + return val; >> } >> >> val = mem_cgroup_recursive_stat(mem, MEM_CGROUP_STAT_CACHE); >> @@ -3979,6 +4002,10 @@ static u64 mem_cgroup_read(struct cgroup *cont, struct cftype *cft) >> else >> val = res_counter_read_u64(&mem->memsw, name); >> break; >> + case _KMEM: >> + val = res_counter_read_u64(&mem->kmem, name); >> + break; >> + >> default: >> BUG(); >> break; >> @@ -4756,6 +4783,21 @@ static int mem_cgroup_reset_vmscan_stat(struct cgroup *cgrp, >> return 0; >> } >> >> +#ifdef CONFIG_CGROUP_MEM_RES_CTLR_KMEM >> +static u64 kmem_limit_independent_read(struct cgroup *cont, struct cftype *cft) >> +{ >> + return mem_cgroup_from_cont(cont)->kmem_independent; >> +} >> + >> +static int kmem_limit_independent_write(struct cgroup *cont, struct cftype *cft, >> + u64 val) >> +{ >> + cgroup_lock(); >> + mem_cgroup_from_cont(cont)->kmem_independent = !!val; >> + cgroup_unlock(); > > Hm. This code allows that parent/child can have different settings. > Could you add parent-child check as.. > > "If parent sets use_hierarchy==1, children must have the same kmem_independent value > with parant's one." Agree. > How do you think ? I think a hierarchy must have the same config. Yes, I think this is reasonable.
> > BTW...I don't like naming a little ;) > > memory->consolidated/shared/?????_kmem_accounting ? > Or > memory->kmem_independent_accounting ? > > or some better naming ?
I can go with kmem_independent_accounting if you like, it is fine by me.
| |