Re: [PATCH v5 2/2] block: cancel all throttled bios in del_gendisk()

[yukuai (C)]

在 2022/01/08 5:36, Paul E. McKenney 写道:
> On Fri, Jan 07, 2022 at 11:18:47AM -1000, Tejun Heo wrote:
> > Hello,
> >
> > On Fri, Jan 07, 2022 at 04:05:19PM +0100, Michal Koutný wrote:
> > > On Fri, Dec 10, 2021 at 04:31:43PM +0800, Yu Kuai <yukuai3@xxxxxxxxxx> wrote:
> > > > +	 * queue_lock is held, rcu lock is not needed here.
> > > > +	 */
> > > > +	blkg_for_each_descendant_post(blkg, pos_css, td->queue->root_blkg)
> > > > +		tg_drain_bios(&blkg_to_tg(blkg)->service_queue);
> > >
> > > FTR, I acknowledge this can work due to RCU peculiarities, however, I
> > > don't understand why is it preferred against more robust explict
> > > rcu_read_lock().
> > >
> > > (All in all, this isn't a deal breaker and I'm not confident evaluating
> > > the rest of the patch.)
> >
> > Cc'ing Paul for RCU. Paul, this nit is around whether or not to use
> > rcu_read_lock() in an irq disabled section. I can see both sides of the
> > arguments - it's weird to put in an extra rcu_read_lock() when technically
> > unnecessary but it's also nice to have something explicit and structured to
> > mark parts which require RCU protection. Putting in a comment is okay but
> > consistency is difficult to achieve that way.
> >
> > Maybe all these are unnecessary as lockdep would be able to catch them
> > anyway, or maybe we'd want something to explicitly mark RCU protected
> > sections. I don't know but whichever way, I think we need to establish a
> > convention.
>
> The easiest thing to do is to use rcu_dereference_sched() instead of
> rcu_dereference().  This will cause lockdep to insist on preemption
> (for example, interrupts) being disabled.
>
> Or is this a case where a function containing rcu_dereference() is invoked
> with interrupts disabled from some call sites and under rcu_read_lock()
> protection from other call sites?  In this case, it is usually best to
> include that redundant rcu_read_lock() [1].

Hi,

This is the later case, so I guess I'll include that redundant
rcu_read_lock().

Thanks,
Kuai
> 							Thanx, Paul
>
> [1]	If you are a glutton for punishment, or if you would otherwise
> 	have to add a cubic goatskin of rcu_read_lock() calls, you
> 	could instead write this priceless gem in place of the calls to
> 	rcu_dereference() in that common function:
>
> 	p = rcu_dereference_check(ptr, rcu_read_lock_sched_held());
>
> 	This would cause lockdep to be happy with either rcu_read_lock()
> 	or preemption being disabled.
>
> 	This is more precise, and would be preferable in some cases,
> 	for example, if there were lots of hotpath callsites with
> 	interrupts disabled.  "Do we add 25 pairs of rcu_read_lock()
> 	and rcu_read_unlock()?	Or do we add just the one ugly
> 	rcu_dereference_check()?"
> .