在 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()?"
.
