Re: [PATCH V8 07/11] blk-mq: stop to handle IO and drain IO before hctx becomes inactive

[Ming Lei <ming.lei@redhat.com>]

On Wed, Apr 29, 2020 at 09:07:29AM +0100, Will Deacon wrote:
> On Wed, Apr 29, 2020 at 10:16:12AM +0800, Ming Lei wrote:
> > On Tue, Apr 28, 2020 at 05:58:37PM +0200, Peter Zijlstra wrote:
> > > On Sat, Apr 25, 2020 at 05:48:32PM +0200, Christoph Hellwig wrote:
> > > >  		atomic_inc(&data.hctx->nr_active);
> > > >  	}
> > > >  	data.hctx->tags->rqs[rq->tag] = rq;
> > > >  
> > > >  	/*
> > > > +	 * Ensure updates to rq->tag and tags->rqs[] are seen by
> > > > +	 * blk_mq_tags_inflight_rqs.  This pairs with the smp_mb__after_atomic
> > > > +	 * in blk_mq_hctx_notify_offline.  This only matters in case a process
> > > > +	 * gets migrated to another CPU that is not mapped to this hctx.
> > > >  	 */
> > > > +	if (rq->mq_ctx->cpu != get_cpu())
> > > >  		smp_mb();
> > > > +	put_cpu();
> > > 
> > > This looks exceedingly weird; how do you think you can get to another
> > > CPU and not have an smp_mb() implied in the migration itself? Also, what
> > 
> > What we need is one smp_mb() between the following two OPs:
> > 
> > 1) 
> >    rq->tag = rq->internal_tag;
> >    data.hctx->tags->rqs[rq->tag] = rq;
> > 
> > 2) 
> > 	if (unlikely(test_bit(BLK_MQ_S_INACTIVE, &rq->mq_hctx->state)))
> > 
> > And the pair of the above barrier is in blk_mq_hctx_notify_offline().
> 
> I'm struggling with this, so let me explain why. My understanding of the
> original patch [1] and your explanation above is that you want *either* of
> the following behaviours
> 
>   - __blk_mq_get_driver_tag() (i.e. (1) above) and test_bit(BLK_MQ_S_INACTIVE, ...)
>     run on the same CPU with barrier() between them, or
> 
>   - There is a migration and therefore an implied smp_mb() between them
> 
> However, given that most CPUs can speculate loads (and therefore the
> test_bit() operation), I don't understand how the "everything runs on the
> same CPU" is safe if a barrier() is required.  In other words, if the
> barrier() is needed to prevent the compiler hoisting the load, then the CPU
> can still cause problems.

Do you think the speculate loads may return wrong value of
BLK_MQ_S_INACTIVE bit in case of single CPU? BTW, writing the bit is
done on the same CPU. If yes, this machine may not obey cache consistency,
IMO.

Also smp_mb() is really barrier() in case of non-SMP, looks non-SMP code
still works well without other barrier required even though with
speculate loads.

Thanks,
Ming

> 
> Thanks,
> 
> Will
> 
> [1] https://lore.kernel.org/linux-block/20200424102351.475641-8-ming.lei@redhat.com/
> 

-- 
Ming