linux-kernel - Re: [PATCH UPDATED 11/11] blkcg: unify blkg's for blkcg policies

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <20120203205910.GB14209@google.com>
Date:	Fri, 3 Feb 2012 12:59:10 -0800
From:	Tejun Heo <tj@...nel.org>
To:	Vivek Goyal <vgoyal@...hat.com>
Cc:	axboe@...nel.dk, ctalbott@...gle.com, rni@...gle.com,
	linux-kernel@...r.kernel.org
Subject: Re: [PATCH UPDATED 11/11] blkcg: unify blkg's for blkcg policies

Hey, Vivek.

On Fri, Feb 03, 2012 at 02:41:05PM -0500, Vivek Goyal wrote:
> On Wed, Feb 01, 2012 at 04:37:30PM -0800, Tejun Heo wrote:
> > As a transitional step to untangle blkg management, elvswitch and
> > policy [de]registration, all blkgs except the root blkg are being shot
> > down during elvswitch and bypass.  This patch adds blkg_root_update()
> > to update root blkg in place on policy change.  This is hacky and racy
> > but should be good enough as interim step until we get locking
> > simplified and switch over to proper in-place update for all blkgs.
> 
> - So we don't shoot down root group over elevator switch and policy
>   changes because we are not sure if we will be able to alloc new
>   group? It is not like elevator where we don't free the old one till
>   we have made sure that new one is allocated and initialized properly.

No, because we policies cache root group and we don't have mechanism
to update them.  I could have added that but root group management
should be moved to blkcg core anyway and in-place update will be
applied to all blkgs, so I just chose a dirty shortcut as an interim
step.

> - I am assuming that we will change  blkg_destroy_all() later to also
>   take policy as argument and only destroy policy data of respective
>   policy and not the whole group. (Well I guess we can destroy the whole
>   group if it was only policy on the group). 

Yeap, that's what's scheduled.

> [..]
> >  static struct blkio_group *blkg_alloc(struct blkio_cgroup *blkcg,
> > -				      struct request_queue *q,
> > -				      struct blkio_policy_type *pol)
> > +				      struct request_queue *q)
> 
> Comment before this function still mentions "pol" as function argument.

Will update.

> [..]
> > @@ -776,43 +786,49 @@ blkiocg_reset_stats(struct cgroup *cgrou
> >  #endif
> >  
> >  	blkcg = cgroup_to_blkio_cgroup(cgroup);
> > +	spin_lock(&blkio_list_lock);
> >  	spin_lock_irq(&blkcg->lock);
> 
> Isn't blkcg lock enough to protect against policy registration/deregistration.
> A policy can not add/delete a group to cgroup list without blkcg list. 

But pol list can change regardless of that, no?

Thanks.

-- 
tejun
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/