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

[Paul Bolle]

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@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/