linux-kernel - Re: [PATCH 5/8] blkcg: make sure blkg_lookup() returns %NULL if @q is bypassing

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] [day] [month] [year] [list]

Date:	Mon, 16 Apr 2012 15:41:32 -0700
From:	"Paul E. McKenney" <paulmck@...ux.vnet.ibm.com>
To:	Vivek Goyal <vgoyal@...hat.com>
Cc:	Tejun Heo <tj@...nel.org>, axboe@...nel.dk, ctalbott@...gle.com,
	rni@...gle.com, linux-kernel@...r.kernel.org,
	cgroups@...r.kernel.org, containers@...ts.linux-foundation.org
Subject: Re: [PATCH 5/8] blkcg: make sure blkg_lookup() returns %NULL if @q
 is bypassing

On Fri, Apr 13, 2012 at 01:23:36PM -0400, Vivek Goyal wrote:
> On Fri, Apr 13, 2012 at 10:03:34AM -0700, Tejun Heo wrote:
> > Hey,
> > 
> > On Fri, Apr 13, 2012 at 12:00:53PM -0400, Vivek Goyal wrote:
> > > On Thu, Apr 12, 2012 at 04:29:37PM -0700, Tejun Heo wrote:
> > > 
> > > [..]
> > > >   * In bypass mode, only the dispatch FIFO queue of @q is used.  This
> > > >   * function makes @q enter bypass mode and drains all requests which were
> > > >   * throttled or issued before.  On return, it's guaranteed that no request
> > > > - * is being throttled or has ELVPRIV set.
> > > > + * is being throttled or has ELVPRIV set and blk_queue_bypass() is %true
> > > > + * inside queue or RCU read lock.
> > > >   */
> > > >  void blk_queue_bypass_start(struct request_queue *q)
> > > >  {
> > > > @@ -426,6 +427,7 @@ void blk_queue_bypass_start(struct request_queue *q)
> > > >  	spin_unlock_irq(q->queue_lock);
> > > >  
> > > >  	blk_drain_queue(q, false);
> > > > +	synchronize_rcu();
> > > 
> > > I guess this synchronize_rcu() needs some comments here to make it clear
> > > what it meant for. IIUC, you are protecting against policy data (stats
> > > update) which happen under rcu in throttling code? You want to make sure
> > > all these updaters are done before you go ahead with
> > > activation/deactivation of a policy.
> > > 
> > > Well, I am wondering if CFQ is policy being activated/deactivated why
> > > should we try to drain other policie's requests. Can't one continue
> > > to work without draining all the throttled requests. We probably just
> > > need to make sure new groups are not created.
> > 
> > So, I think synchronization rules like this are something which the
> > core should define.  cfq may not use it but the sync rules should
> > still be the same for all policies.  In this case, what the core
> > provides is "blk_queue_bypass() is guaranteed to be seen as %true
> > inside RCU read lock section once this function returns", which in
> > turn will guarantee that RCU read-lock protected blkg_lookup() is
> > guaranteed to fail once the function returns.  This property makes RCU
> > protected blkg_lookup() safe against queue bypassing, which is what we
> > want.
> 
> I think now synchronize_rcu() has become part of cfq_init_queue()
> effectively and that will slow down boot. In the past I had to remove
> it.

One alternative approach is to use synchronize_rcu_expedited().

							Thanx, Paul

--
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/