> On Oct 5, 2019, at 7:29 PM, Andrew Morton wrote: > > On Fri, 4 Oct 2019 12:42:26 -0400 Qian Cai wrote: > >> It is unsafe to call printk() while zone->lock was held, i.e., >> >> zone->lock --> console_sem >> >> because the console could always allocate some memory in different code >> paths and form locking chains in an opposite order, >> >> console_sem --> * --> zone->lock >> >> As the result, it triggers lockdep splats like below and in [1]. It is >> fine to take zone->lock after has_unmovable_pages() (which has >> dump_stack()) in set_migratetype_isolate(). While at it, remove a >> problematic printk() in __offline_isolated_pages() only for debugging as >> well which will always disable lockdep on debug kernels. >> >> The problem is probably there forever, but neither many developers will >> run memory offline with the lockdep enabled nor admins in the field are >> lucky enough yet to hit a perfect timing which required to trigger a >> real deadlock. In addition, there aren't many places that call printk() >> while zone->lock was held. >> >> WARNING: possible circular locking dependency detected >> ------------------------------------------------------ >> test.sh/1724 is trying to acquire lock: >> 0000000052059ec0 (console_owner){-...}, at: console_unlock+0x >> 01: 328/0xa30 >> >> but task is already holding lock: >> 000000006ffd89c8 (&(&zone->lock)->rlock){-.-.}, at: start_iso >> 01: late_page_range+0x216/0x538 >> >> which lock already depends on the new lock. >> >> the existing dependency chain (in reverse order) is: >> >> -> #2 (&(&zone->lock)->rlock){-.-.}: >> lock_acquire+0x21a/0x468 >> _raw_spin_lock+0x54/0x68 >> get_page_from_freelist+0x8b6/0x2d28 >> __alloc_pages_nodemask+0x246/0x658 >> __get_free_pages+0x34/0x78 >> sclp_init+0x106/0x690 >> sclp_register+0x2e/0x248 >> sclp_rw_init+0x4a/0x70 >> sclp_console_init+0x4a/0x1b8 >> console_init+0x2c8/0x410 >> start_kernel+0x530/0x6a0 >> startup_continue+0x70/0xd0 > > This appears to be the core of our problem? No, that is just one of those many places could form the lock chain. console_lock -> other locks -> zone_lock Another example is, https://lore.kernel.org/lkml/1568823006.5576.178.camel@lca.pw/ It is easier to avoid, zone_lock -> console_lock rather than fixing the opposite. > At initialization time, > the sclp driver registers an inappropriate dependency with lockdep. It > does this by calling into the page allocator while holding sclp_lock. > But we don't *want* to teach lockdep that sclp_lock nests outside > zone->lock. We want the opposite. > > So can we address this class of problem by declaring "thou shalt not > call the page allocator while holding a lock which can be taken on the > prink path?". And then declare sclp to be defective. > > > And I think sclp is kinda buggy-but-lucky anyway: if console output is > directed to sclp device #0 and we're then trying to initialize sclp > device #1 then any printk which happens during that initialization will > deadlock. The driver escapes this by only supporting a single device > system-wide but it's not a model which drivers should generally follow. > > (And if sclp will only ever support a single device system-wide, why > the heck does it need to take sclp_lock() on the device initialization > path??) > >