lkml.org 
[lkml]   [2022]   [Jan]   [9]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
/
SubjectRe: [PATCH v5 2/2] block: cancel all throttled bios in del_gendisk()
From
Date
在 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@huawei.com> 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()?"
> .
>

\
 
 \ /
  Last update: 2022-01-10 02:29    [W:1.079 / U:0.008 seconds]
©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site