On Fri, Jul 14, 2023 at 11:54:47PM +0800, Alan Huang wrote: > > > 2023年7月14日 23:35，Alan Huang <mmpgouride@xxxxxxxxx> 写道： > > > >> > >> 2023年7月14日 10:16，Paul E. McKenney <paulmck@xxxxxxxxxx> 写道： > >> > >> 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@xxxxxxxxxxxxxxxxx> wrote: > >>>>> On 2023/7/13 22:07, Joel Fernandes wrote: > >>>>>> On Thu, Jul 13, 2023 at 12:59 AM Gao Xiang <hsiangkao@xxxxxxxxxxxxxxxxx> 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@xxxxxxxxxx > >>>>>>>>> [2] https://lore.kernel.org/r/CAHk-=wgE9kORADrDJ4nEsHHLirqPCZ1tGaEPAZejHdZ03qCOGg@xxxxxxxxxxxxxx > >>>>>>>>> [3] https://lore.kernel.org/r/CAB=BE-SBtO6vcoyLNA9F-9VaN5R0t3o_Zn+FW8GbO6wyUqFneQ@xxxxxxxxxxxxxx > >>>>>>>>> [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. Is there some other field that would work? Thanx, Paul > > 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 > >
