On 3/11/21 4:02 AM, Michal Hocko wrote: > On Thu 11-03-21 12:36:51, Peter Zijlstra wrote: >> On Thu, Mar 11, 2021 at 12:09:15PM +0100, Michal Hocko wrote: >> >>> Sorry for being dense but I do not follow. You have provided the >>> following example >>> spin_lock(&A); >>> <IRQ> >>> spin_lock(&A); >>> >>> if A == hugetlb_lock then we should never reenter with >>> free_huge_page >> >> What I'm saying is that if irq_disabled(), the that interrupt cannot >> happen, so the second spin_lock cannot happen, so the deadlock cannot >> happen. >> >> So: '!irqs_disabled() && in_atomic()' is sufficient to avoid the IRQ >> recursion deadlock. > > OK, then I understand your point now. I thought you were arguing > an actual deadlock scenario. As I've said irq_disabled check would be > needed for sleeping operations that we already do. > >> Also, Linus hates constructs like this: >> >> https://lkml.kernel.org/r/CAHk-=wht7kAeyR5xEW2ORj7m0hibVxZ3t+2ie8vNHLQfdbN2_g@xxxxxxxxxxxxxx >> >>> From the code simplicity POV (and hugetlb has grown a lot of complexity) >>> it would be really easiest to make sure __free_huge_page to be called >>> from a non-atomic process context. There are few ways to do that >>> - defer each call to a WQ - user visible which sucks >>> - defer from atomic or otherwise non-sleeping contextx - requires >>> reliable in_atomic AFAICS >>> - defer sleeping operations - makes the code flow more complex and it >>> would be again user visible in some cases. >>> >>> So I would say we are in "pick your own poison" kind of situation. >> >> Just to be clear: >> >> NAK on this patch and any and all ductape crap. Fix it properly, make >> hugetlb_lock, spool->lock IRQ-safe, move the workqueue into the CMA >> thing. >> >> The code really doesn't look _that_ complicated. > > Fair enough. As I've said I am not a great fan of this patch either > but it is a quick fix for a likely long term problem. If reworking the > hugetlb locking is preferable then be it. Thanks you Michal and Peter. This patch was mostly about starting a discussion, as this topic came up in a couple different places. I included the 'train wreck' of how we got here just for a bit of history. I'll start working on a proper fix. -- Mike Kravetz
