Re: [PATCH 6/8] blkio-cgroup: "use_hierarchy" interface without any functionality.

[Vivek Goyal <vgoyal@redhat.com>]

On Fri, Dec 17, 2010 at 11:06:46AM +0800, Gui Jianfeng wrote:
[..]
> >>>> +static int
> >>>> +blkiocg_use_hierarchy_write(struct cgroup *cgroup,
> >>>> +			    struct cftype *cftype, u64 val)
> >>>> +{
> >>>> +	struct blkio_cgroup *blkcg;
> >>>> +	struct blkio_group *blkg;
> >>>> +	struct hlist_node *n;
> >>>> +	struct blkio_policy_type *blkiop;
> >>>> +
> >>>> +	blkcg = cgroup_to_blkio_cgroup(cgroup);
> >>>> +
> >>>> +	if (val > 1 || !list_empty(&cgroup->children))
> >>>> +		return -EINVAL;
> >>>> +
> >>>> +	if (blkcg->use_hierarchy == val)
> >>>> +		return 0;
> >>>> +
> >>>> +	spin_lock(&blkio_list_lock);
> >>>> +	blkcg->use_hierarchy = val;
> >>>> +
> >>>> +	hlist_for_each_entry(blkg, n, &blkcg->blkg_list, blkcg_node) {
> >>>> +		list_for_each_entry(blkiop, &blkio_list, list) {
> >>>> +			/*
> >>>> +			 * If this policy does not own the blkg, do not change
> >>>> +			 * cfq group scheduling mode.
> >>>> +			 */
> >>>> +			if (blkiop->plid != blkg->plid)
> >>>> +				continue;
> >>>> +
> >>>> +			if (blkiop->ops.blkio_update_use_hierarchy_fn)
> >>>> +				blkiop->ops.blkio_update_use_hierarchy_fn(blkg,
> >>>> +									  val);
> >>> Should we really allow this? I mean allow changing hierarchy of a group
> >>> when there are already children groups. I think memory controller does
> >>> not allow this. We can design along the same lines. Keep use_hierarchy
> >>> as 0 by default. Allow changing it only if there are no children cgroups.
> >>> Otherwise we shall have to send notifications to subscribing policies
> >>> and then change their structure etc. Lets keep it simple.
> >> Yes, I really don't allow changing use_hierarchy if there are childre cgroups.
> >> Please consider following line in my patch.
> >>
> >> if (val > 1 || !list_empty(&cgroup->children))
> >> 	return -EINVAL;
> > 
> > If there are no children cgroups, then there can not be any children blkg
> > and there is no need to send any per blkg notification to each policy?
> 
> Firsly, In my patch, per blkg notification only happens on root blkg.
> Secondly, root cfqg is put onto "flat_service_tree" in flat mode,
> where for hierarchical mode, it don't belongs to anybody. When switching, it
> has to inform root cfqg to switch onto or switch off "flat_service_tree".
> 
> Anyway, If we're going to put root cfqg onto grp_service_tree regardless of
> flat or hierarchical mode, this piece of code can be gone.
> 

Exactly. Keeping everything on grp_service_tree() both for flat and
hierarchical mode will make sure no root group moving around business
and no notifications when use_hierarchy is set.

Thanks
Vivek