linux-kernel - Re: [PATCH 5/5] CFQ: use proper locking for cache of last hit cic

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]

Date:	Wed, 08 Jun 2011 20:18:44 +0200
From:	Paul Bolle <pebolle@...cali.nl>
To:	Jens Axboe <axboe@...nel.dk>
Cc:	"Paul E. McKenney" <paulmck@...ux.vnet.ibm.com>,
	Vivek Goyal <vgoyal@...hat.com>, linux-kernel@...r.kernel.org
Subject: Re: [PATCH 5/5] CFQ: use proper locking for cache of last hit cic

On Mon, 2011-06-06 at 05:06 +0200, Jens Axboe wrote:
> On 2011-06-05 18:26, Paul Bolle wrote:
> > @@ -2704,8 +2706,13 @@ static void __cfq_exit_single_io_context(struct cfq_data *cfqd,
> >  	smp_wmb();
> >  	cic->key = cfqd_dead_key(cfqd);
> >  
> > -	if (ioc->last_cic == cic)
> > +	spin_lock_irqsave(&ioc->lock, flags);
> > +	rcu_read_lock();
> > +	last_cic = rcu_dereference(ioc->last_cic);
> > +	rcu_read_unlock();
> > +	if (last_cic == cic)
> >  		rcu_assign_pointer(ioc->last_cic, NULL);
> > +	spin_unlock_irqrestore(&ioc->lock, flags);
> 
> We don't need the ioc->lock for checking the cache, it would in fact
> defeat the purpose of using RCU.

Just to show that I'm RCU-challenged, is that because:
1) my use of locking on ioc->lock defends for a race that is not
actually possible; or
2) the worst thing that could happen is that some new and correct value
of ioc->last_cic will be replaced with NULL, which is simply not a big
deal?

> But this hunk will clash with the
> merged part anyway.

Looking at Paul's feedback I do have a feeling that in your commit
(9b50902db5eb8a220160fb89e95aa11967998d12, "cfq-iosched: fix locking
around ioc->ioc_data assignment") the line:
	if (rcu_dereference(ioc->ioc_data) == cic) {

could actually be replaced with:
	if (rcu_access_pointer(ioc->ioc_data) == cic) {

Is that correct?

> [...]
> See Pauls comment on this part.

You seem to be offline right now. When you're back online, could you
please say whether or not you accept the two renaming patches that you
have rejected a few days ago (and for which I gave some follow up
arguments)? After that I'll send an update to this patch (and its commit
message) to reflect Paul's review and your review.


Paul Bolle

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