Messages in this thread | | | Date | Fri, 6 Feb 2009 11:18:12 +0900 | From | KAMEZAWA Hiroyuki <> | Subject | Re: [RFC][PATCH] Reduce size of swap_cgroup by CSS ID |
| |
On Fri, 06 Feb 2009 10:07:40 +0800 Li Zefan <lizf@cn.fujitsu.com> wrote:
> > +/* > > + * A helper function to get mem_cgroup from ID. must be called under > > + * rcu_read_lock(). Because css_tryget() is called under this, css_put > > + * should be called later. > > + */ > > +static struct mem_cgroup *mem_cgroup_lookup_get(unsigned short id) > > +{ > > + struct cgroup_subsys_state *css; > > + > > + /* ID 0 is unused ID */ > > + if (!id) > > + return NULL; > > + css = css_lookup(&mem_cgroup_subsys, id); > > + if (css && css_tryget(css)) > > + return container_of(css, struct mem_cgroup, css); > > + return NULL; > > +} > > the returned mem_cgroup needn't be protected by rcu_read_lock(), so I > think this is better: > rcu_read_lock(); > css = css_lookup(&mem_cgroup_subsys, id); > rcu_read_unlock(); > and no lock is needed when calling mem_cgroup_lookup_get(). > Hmm, maybe you're right.
> > * Returns old value at success, NULL at failure. > > * (Of course, old value can be NULL.) > > */ > > -struct mem_cgroup *swap_cgroup_record(swp_entry_t ent, struct mem_cgroup *mem) > > +unsigned short swap_cgroup_record(swp_entry_t ent, unsigned short id) > > kernel-doc needs to be updated > ok.
> > * lookup_swap_cgroup - lookup mem_cgroup tied to swap entry > > * @ent: swap entry to be looked up. > > * > > - * Returns pointer to mem_cgroup at success. NULL at failure. > > + * Returns CSS ID of mem_cgroup at success. NULL at failure. > > s/NULL/0/ > yes.
Okay, thanks.
BTW, this patch is totally buggy ;( Sorry. Below is my memo. ==
- mem_cgroup can by destroyed() while there are swap_cgroup point to mem_cgroup. If there are reference from swap_cgroup, kfree() is delayed. - But this patch just uses css_tryget(). css_tryget() returnes false if mem_cgroup is destroyed. - So, refcnt for destroyed mem_cgroup will not be decreased and memory for mem_cgroup will be never freed.
- Even if we use ID instead of pointer, the situation does not change. we have to prevent ID to be reused.... - One choice for removing all swap_account at destroy() is forget all swap accounts. But this may need "scan" all swap_cgroup in the system at rmdir(). This will be unacceptably slow on large swap systems. ==
I'll post v2 but it will be still tricky.
Thanks, -Kame
| |