On Thu, Jul 13, 2023 at 10:05:46AM -0700, Sandeep Dhavale wrote: > Hello All, > > Let me answer some of the questions raised here. > > * Performance aspect > EROFS is one of the popular filesystem of choice in Android for read > only partitions > as it provides 30%+ storage space savings with compression. > In addition to microbenchmarks, boot times and cold app launch > benchmarks are very > important to the Android ecosystem as they directly translate to > responsiveness from user point of view. We saw some > performance penalty in cold app launch benchmarks in a few scenarios. > Analysis showed that the significant variance was coming from the > scheduling cost while decompression cost was more or less the same. > Please see the [1] which shows scheduling costs for kthread vs kworker. > > > Just out of curiosity, exactly how much is it costing to trigger the > workqueue? > I think the cost to trigger is not much, it's the actual scheduling latency for > the thread is the one which we want to cut down. And if we are already in > thread context then there is no point in incurring any extra cost if > we can detect > it reliably. That is what erofs check is trying to do. > > >One additional question... What is your plan for kernels built with > CONFIG_PREEMPT_COUNT=n? > If there is no reliable way to detect if we can block or not then in that > case erofs has no option but to schedule the kworker. > > * Regarding BLK_MQ_F_BLOCKING > As mentioned by Gao in the thread this is a property of blk-mq device > underneath, > so erofs cannot control it has it has to work with different types of > block devices. > > * Regarding rcu_is_watching() > > >I am assuming you mean you would grab the mutex accidentally when in an RCU > reader, and might_sleep() presumably in the mutex internal code will scream? > > Thank you Paul for explaining in detail why it is important. I can get > the V2 going. > >From the looking at the code at kernel/sched/core.c which only looks > at rcu_preempt_depth(), > I am thinking it may still get triggered IIUC. > > > The following is untested, and is probably quite buggy, but it should > provide you with a starting point. > .. > > Yes, that can fix the problem at hand as the erofs check also looks > for rcu_preempt_depth(). > A similar approach was discarded as rcu_preempt_depth() was though to > be low level > and we used rcu_read_lock_any_held() which is the superset until we > figured out inconsistency > when ! CONFIG_DEBUG_LOCK_ALLOC. Thank you for the background. > Paul, Joel, > Shall we fix the rcu_read_lock_*held() regardless of how erofs > improves the check? Help me out here. Exactly what is broken with rcu_read_lock_*held(), keeping in mind their intended use for lockdep-based debugging? Thanx, Paul > Thanks, > Sandeep. > > [1] https://lore.kernel.org/linux-erofs/20230208093322.75816-1-hsiangkao@xxxxxxxxxxxxxxxxx/
