Messages in this thread |  | | | Date | Thu, 22 Jan 2009 11:59:08 +0900 | | From | KAMEZAWA Hiroyuki <> | | Subject | Re: [PATCH 1/4] cgroup: add CSS ID |
| |
On Thu, 22 Jan 2009 11:37:29 +0900 Daisuke Nishimura <nishimura@mxp.nes.nec.co.jp> wrote:
> Hi. > > Sorry for very late reply. > > It looks good in general. > Just a few comments. > > > +/** > > + * css_lookup - lookup css by id > > + * @ss: cgroup subsys to be looked into. > > + * @id: the id > > + * > > + * Returns pointer to cgroup_subsys_state if there is valid one with id. > > + * NULL if not. Should be called under rcu_read_lock() > > + */ > > +struct cgroup_subsys_state *css_lookup(struct cgroup_subsys *ss, int id) > > +{ > > + struct css_id *cssid = NULL; > > + > > + BUG_ON(!ss->use_id); > > + cssid = idr_find(&ss->idr, id); > > + > > + if (unlikely(!cssid)) > > + return NULL; > > + > > + return rcu_dereference(cssid->css); > > +} > > + > Just for clarification, is there any user of this function ? > (I agree it's natulal to define 'lookup' function, though.) > A user in my plan is swap_cgroup.
> > +/** > > + * css_get_next - lookup next cgroup under specified hierarchy. > > + * @ss: pointer to subsystem > > + * @id: current position of iteration. > > + * @root: pointer to css. search tree under this. > > + * @foundid: position of found object. > > + * > > + * Search next css under the specified hierarchy of rootid. Calling under > > + * rcu_read_lock() is necessary. Returns NULL if it reaches the end. > > + */ > > +struct cgroup_subsys_state * > > +css_get_next(struct cgroup_subsys *ss, int id, > > + struct cgroup_subsys_state *root, int *foundid) > > +{ > > + struct cgroup_subsys_state *ret = NULL; > > + struct css_id *tmp; > > + int tmpid; > > + int rootid = css_id(root); > > + int depth = css_depth(root); > > + > I think it's safe here, but isn't it better to call css_id/css_depth > under rcu_read_lock(they call rcu_dereference) ? > As commented, this css_get_next() call should be called under rcu_read_lock().
> > + if (!rootid) > > + return NULL; > > + > > + BUG_ON(!ss->use_id); > > + rcu_read_lock(); > > + /* fill start point for scan */ > > + tmpid = id; > > + while (1) { > > + /* > > + * scan next entry from bitmap(tree), tmpid is updated after > > + * idr_get_next(). > > + */ > > + spin_lock(&ss->id_lock); > > + tmp = idr_get_next(&ss->idr, &tmpid); > > + spin_unlock(&ss->id_lock); > > + > > + if (!tmp) > > + break; > > + if (tmp->depth >= depth && tmp->stack[depth] == rootid) { > Can't be css_is_ancestor used here ? > I think it would be more easy to understand. > Hmm, it requires
css_is_ancestor(tmp->css, root);
and adds memory barriers to acsess tmp->css and tmp->css->id, root->id (compiler will not optimize these accesses because of memory barrier.)
So, I think bare code is better here.
Thank you for review.
-Kame
|  |