Message-Id: <6E5326AD-9A5D-4570-906A-BDE8257B6F0C@gmail.com>
Date:   Fri, 14 Jul 2023 23:54:47 +0800
From:   Alan Huang <mmpgouride@...il.com>
To:     paulmck@...nel.org
Cc:     Joel Fernandes <joel@...lfernandes.org>,
        Gao Xiang <hsiangkao@...ux.alibaba.com>,
        Sandeep Dhavale <dhavale@...gle.com>,
        Frederic Weisbecker <frederic@...nel.org>,
        Neeraj Upadhyay <quic_neeraju@...cinc.com>,
        Josh Triplett <josh@...htriplett.org>,
        Boqun Feng <boqun.feng@...il.com>,
        Steven Rostedt <rostedt@...dmis.org>,
        Mathieu Desnoyers <mathieu.desnoyers@...icios.com>,
        Lai Jiangshan <jiangshanlai@...il.com>,
        Zqiang <qiang.zhang1211@...il.com>,
        Matthias Brugger <matthias.bgg@...il.com>,
        AngeloGioacchino Del Regno 
        <angelogioacchino.delregno@...labora.com>,
        linux-erofs@...ts.ozlabs.org, xiang@...nel.org,
        Will Shiu <Will.Shiu@...iatek.com>, kernel-team@...roid.com,
        rcu@...r.kernel.org, linux-kernel@...r.kernel.org,
        linux-arm-kernel@...ts.infradead.org,
        linux-mediatek@...ts.infradead.org
Subject: Re: [PATCH v1] rcu: Fix and improve RCU read lock checks when
 !CONFIG_DEBUG_LOCK_ALLOC


> 2023年7月14日 23:35，Alan Huang <mmpgouride@...il.com> 写道：
> 
>> 
>> 2023年7月14日 10:16，Paul E. McKenney <paulmck@...nel.org> 写道：
>> 
>> On Thu, Jul 13, 2023 at 09:33:35AM -0700, Paul E. McKenney wrote:
>>> On Thu, Jul 13, 2023 at 11:33:24AM -0400, Joel Fernandes wrote:
>>>> On Thu, Jul 13, 2023 at 10:34 AM Gao Xiang <hsiangkao@...ux.alibaba.com> wrote:
>>>>> On 2023/7/13 22:07, Joel Fernandes wrote:
>>>>>> On Thu, Jul 13, 2023 at 12:59 AM Gao Xiang <hsiangkao@...ux.alibaba.com> wrote:
>>>>>>> On 2023/7/13 12:52, Paul E. McKenney wrote:
>>>>>>>> On Thu, Jul 13, 2023 at 12:41:09PM +0800, Gao Xiang wrote:
>>>>>>> 
>>>>>>> ...
>>>>>>> 
>>>>>>>>> 
>>>>>>>>> There are lots of performance issues here and even a plumber
>>>>>>>>> topic last year to show that, see:
>>>>>>>>> 
>>>>>>>>> [1] https://lore.kernel.org/r/20230519001709.2563-1-tj@kernel.org
>>>>>>>>> [2] https://lore.kernel.org/r/CAHk-=wgE9kORADrDJ4nEsHHLirqPCZ1tGaEPAZejHdZ03qCOGg@mail.gmail.com
>>>>>>>>> [3] https://lore.kernel.org/r/CAB=BE-SBtO6vcoyLNA9F-9VaN5R0t3o_Zn+FW8GbO6wyUqFneQ@mail.gmail.com
>>>>>>>>> [4] https://lpc.events/event/16/contributions/1338/
>>>>>>>>> and more.
>>>>>>>>> 
>>>>>>>>> I'm not sure if it's necessary to look info all of that,
>>>>>>>>> andSandeep knows more than I am (the scheduling issue
>>>>>>>>> becomes vital on some aarch64 platform.)
>>>>>>>> 
>>>>>>>> Hmmm...  Please let me try again.
>>>>>>>> 
>>>>>>>> Assuming that this approach turns out to make sense, the resulting
>>>>>>>> patch will need to clearly state the performance benefits directly in
>>>>>>>> the commit log.
>>>>>>>> 
>>>>>>>> And of course, for the approach to make sense, it must avoid breaking
>>>>>>>> the existing lockdep-RCU debugging code.
>>>>>>>> 
>>>>>>>> Is that more clear?
>>>>>>> 
>>>>>>> Personally I'm not working on Android platform any more so I don't
>>>>>>> have a way to reproduce, hopefully Sandeep could give actually
>>>>>>> number _again_ if dm-verity is enabled and trigger another
>>>>>>> workqueue here and make a comparsion why the scheduling latency of
>>>>>>> the extra work becomes unacceptable.
>>>>>>> 
>>>>>> 
>>>>>> Question from my side, are we talking about only performance issues or
>>>>>> also a crash? It appears z_erofs_decompress_pcluster() takes
>>>>>> mutex_lock(&pcl->lock);
>>>>>> 
>>>>>> So if it is either in an RCU read-side critical section or in an
>>>>>> atomic section, like the softirq path, then it may
>>>>>> schedule-while-atomic or trigger RCU warnings.
>>>>>> 
>>>>>> z_erofs_decompressqueue_endio
>>>>>> -> z_erofs_decompress_kickoff
>>>>>> ->z_erofs_decompressqueue_work
>>>>>>  ->z_erofs_decompress_queue
>>>>>>   -> z_erofs_decompress_pcluster
>>>>>>    -> mutex_lock
>>>>>> 
>>>>> 
>>>>> Why does the softirq path not trigger a workqueue instead?
>>>> 
>>>> I said "if it is". I was giving a scenario. mutex_lock() is not
>>>> allowed in softirq context or in an RCU-reader.
>>>> 
>>>>>> Per Sandeep in [1], this stack happens under RCU read-lock in:
>>>>>> 
>>>>>> #define __blk_mq_run_dispatch_ops(q, check_sleep, dispatch_ops) \
>>>>>> [...]
>>>>>>                rcu_read_lock();
>>>>>>                (dispatch_ops);
>>>>>>                rcu_read_unlock();
>>>>>> [...]
>>>>>> 
>>>>>> Coming from:
>>>>>> blk_mq_flush_plug_list ->
>>>>>>                           blk_mq_run_dispatch_ops(q,
>>>>>>                                __blk_mq_flush_plug_list(q, plug));
>>>>>> 
>>>>>> and __blk_mq_flush_plug_list does this:
>>>>>>          q->mq_ops->queue_rqs(&plug->mq_list);
>>>>>> 
>>>>>> This somehow ends up calling the bio_endio and the
>>>>>> z_erofs_decompressqueue_endio which grabs the mutex.
>>>>>> 
>>>>>> So... I have a question, it looks like one of the paths in
>>>>>> __blk_mq_run_dispatch_ops() uses SRCU.  Where are as the alternate
>>>>>> path uses RCU. Why does this alternate want to block even if it is not
>>>>>> supposed to? Is the real issue here that the BLK_MQ_F_BLOCKING should
>>>>>> be set? It sounds like you want to block in the "else" path even
>>>>>> though BLK_MQ_F_BLOCKING is not set:
>>>>> 
>>>>> BLK_MQ_F_BLOCKING is not a flag that a filesystem can do anything with.
>>>>> That is block layer and mq device driver stuffs. filesystems cannot set
>>>>> this value.
>>>>> 
>>>>> As I said, as far as I understand, previously,
>>>>> .end_io() can only be called without RCU context, so it will be fine,
>>>>> but I don't know when .end_io() can be called under some RCU context
>>>>> now.
>>>> 
>>>>> From what Sandeep described, the code path is in an RCU reader. My
>>>> question is more, why doesn't it use SRCU instead since it clearly
>>>> does so if BLK_MQ_F_BLOCKING. What are the tradeoffs? IMHO, a deeper
>>>> dive needs to be made into that before concluding that the fix is to
>>>> use rcu_read_lock_any_held().
>>> 
>>> How can this be solved?
>>> 
>>> 1. Always use a workqueue.  Simple, but is said to have performance
>>> issues.
>>> 
>>> 2. Pass a flag in that indicates whether or not the caller is in an
>>> RCU read-side critical section.  Conceptually simple, but might
>>> or might not be reasonable to actually implement in the code as
>>> it exists now. (You tell me!)
>>> 
>>> 3. Create a function in z_erofs that gives you a decent
>>> approximation, maybe something like the following.
>>> 
>>> 4. Other ideas here.
>> 
>> 5. #3 plus make the corresponding Kconfig option select
>> PREEMPT_COUNT, assuming that any users needing compression in
>> non-preemptible kernels are OK with PREEMPT_COUNT being set.
>> (Some users of non-preemptible kernels object strenuously
>> to the added overhead from CONFIG_PREEMPT_COUNT=y.)
> 
> 6. Set one bit in bio->bi_private, check the bit and flip it in rcu_read_lock() path,
> then in z_erofs_decompressqueue_endio, check if the bit has changed.

Seems bad, read and modify bi_private is a bad idea.

> 
> Not sure if this is feasible or acceptable. :)
> 
>> 
>> Thanx, Paul
>> 
>>> The following is untested, and is probably quite buggy, but it should
>>> provide you with a starting point.
>>> 
>>> static bool z_erofs_wq_needed(void)
>>> {
>>> if (IS_ENABLED(CONFIG_PREEMPTION) && rcu_preempt_depth())
>>> return true;  // RCU reader
>>> if (IS_ENABLED(CONFIG_PREEMPT_COUNT) && !preemptible())
>>> return true;  // non-preemptible
>>> if (!IS_ENABLED(CONFIG_PREEMPT_COUNT))
>>> return true;  // non-preeemptible kernel, so play it safe
>>> return false;
>>> }
>>> 
>>> You break it, you buy it!  ;-)
>>> 
>>> Thanx, Paul

Powered by blists - more mailing lists