mm/hugetlb: make hugetlb_lock irq safe

inconsistent {SOFTIRQ-ON-W} -> {IN-SOFTIRQ-W} usage.

swapper/68/0 [HC0[0]:SC1[1]:HE1:SE0] takes:
0000000052a030a7 (hugetlb_lock){+.?.}, at: free_huge_page+0x9c/0x340 {SOFTIRQ-ON-W} state was registered at:

   lock_acquire+0xd4/0x230
   _raw_spin_lock+0x44/0x70
   set_max_huge_pages+0x4c/0x360
   hugetlb_sysctl_handler_common+0x108/0x160
   proc_sys_call_handler+0x134/0x190
   __vfs_write+0x3c/0x1f0
   vfs_write+0xd8/0x220

Quoted 1 line · Aneesh Kumar K.V

inconsistent {SOFTIRQ-ON-W} -> {IN-SOFTIRQ-W} usage.

How do you go from "can be taken in softirq context" problem report to "must disable hard interrupts" solution? Please explain why spin_lock_bh() is not a sufficient fix.

Quoted 10 lines

swapper/68/0 [HC0[0]:SC1[1]:HE1:SE0] takes:
0000000052a030a7 (hugetlb_lock){+.?.}, at: free_huge_page+0x9c/0x340 {SOFTIRQ-ON-W} state was registered at:

    lock_acquire+0xd4/0x230
    _raw_spin_lock+0x44/0x70
    set_max_huge_pages+0x4c/0x360
    hugetlb_sysctl_handler_common+0x108/0x160
    proc_sys_call_handler+0x134/0x190
    __vfs_write+0x3c/0x1f0
    vfs_write+0xd8/0x220

Also, this only seems to trigger here. Is it possible we _already_ have softirqs disabled through every other code path, and it's just this one sysctl handler that needs to disable softirqs? Rather than every lock access?

I'm not seeing any analysis in this patch description, just a kneejerk "lockdep complained, must disable interrupts".

Quoted 18 lines · Matthew Wilcox

Quoted 1 line · Aneesh Kumar K.V

inconsistent {SOFTIRQ-ON-W} -> {IN-SOFTIRQ-W} usage.

How do you go from "can be taken in softirq context" problem report to "must disable hard interrupts" solution? Please explain why spin_lock_bh() is not a sufficient fix.

Quoted 10 lines

swapper/68/0 [HC0[0]:SC1[1]:HE1:SE0] takes:
0000000052a030a7 (hugetlb_lock){+.?.}, at: free_huge_page+0x9c/0x340 {SOFTIRQ-ON-W} state was registered at:

    lock_acquire+0xd4/0x230
    _raw_spin_lock+0x44/0x70
    set_max_huge_pages+0x4c/0x360
    hugetlb_sysctl_handler_common+0x108/0x160
    proc_sys_call_handler+0x134/0x190
    __vfs_write+0x3c/0x1f0
    vfs_write+0xd8/0x220

Quoted 4 lines

Also, this only seems to trigger here. Is it possible we _already_ have softirqs disabled through every other code path, and it's just this one sysctl handler that needs to disable softirqs? Rather than every lock access?

Are you asking whether I looked at moving that put_page to a worker thread?

I didn't. The reason I looked at current patch is to enable the usage of put_page() from irq context. We do allow that for non hugetlb pages. So was not sure adding that additional restriction for hugetlb
is really needed. Further the conversion to irqsave/irqrestore was straightforward.

Quoted 5 lines

I didn't. The reason I looked at current patch is to enable the usage of put_page() from irq context. We do allow that for non hugetlb pages. So was not sure adding that additional restriction for hugetlb
is really needed. Further the conversion to irqsave/irqrestore was straightforward.

straightforward, sure. but is it the right thing to do? do we want to be able to put_page() a hugetlb page from hardirq context?

Quoted 8 lines · On Wed, 5 Sep 2018 06:48:48 -0700 Matthew Wilcox <[email protected]> wrote:

Quoted 5 lines

I didn't. The reason I looked at current patch is to enable the usage of put_page() from irq context. We do allow that for non hugetlb pages. So was not sure adding that additional restriction for hugetlb
is really needed. Further the conversion to irqsave/irqrestore was straightforward.

straightforward, sure. but is it the right thing to do? do we want to be able to put_page() a hugetlb page from hardirq context?

Calling put_page() against a huge page from hardirq seems like the right thing to do - even if it's rare now, it will presumably become more common as the hugepage virus spreads further across the kernel. And the present asymmetry is quite a wart.

That being said, arch/powerpc/mm/mmu_context_iommu.c:mm_iommu_free() is the only known site which does this (yes?)

so perhaps we could put some
stopgap workaround into that site and add a runtime warning into the put_page() code somewhere to detect puttage of huge pages from hardirq and softirq contexts.

And attention will need to be paid to -stable backporting. How long has mm_iommu_free() existed, and been doing this?

Quoted 4 lines

so perhaps we could put some
stopgap workaround into that site and add a runtime warning into the put_page() code somewhere to detect puttage of huge pages from hardirq and softirq contexts.

I think we would add the warning/etc at free_huge_page. The issue would only apply to hugetlb pages, not THP.

But, the more I think about it the more I think Aneesh's patch to do spin_lock/unlock_irqsave is the right way to go. Currently, we only know of one place where a put_page of hugetlb pages is done from softirq context. So, we could take the spin_lock/unlock_bh as Matthew suggested. When the powerpc iommu code was added, I doubt this was taken into account. I would be afraid of someone adding put_page from hardirq context.

Quoted 8 lines · On Wed, 5 Sep 2018 14:35:11 -0700 Mike Kravetz <[email protected]> wrote:

Quoted 4 lines

so perhaps we could put some
stopgap workaround into that site and add a runtime warning into the put_page() code somewhere to detect puttage of huge pages from hardirq and softirq contexts.

I think we would add the warning/etc at free_huge_page. The issue would only apply to hugetlb pages, not THP.

Quoted 6 lines

But, the more I think about it the more I think Aneesh's patch to do spin_lock/unlock_irqsave is the right way to go. Currently, we only know of one place where a put_page of hugetlb pages is done from softirq context. So, we could take the spin_lock/unlock_bh as Matthew suggested. When the powerpc iommu code was added, I doubt this was taken into account. I would be afraid of someone adding put_page from hardirq context.

Me too. If we're going to do this, surely we should make hugepages behave in the same fashion as PAGE_SIZE pages.

Quoted 17 lines · Andrew Morton

Quoted 14 lines · On Wed, 5 Sep 2018 14:35:11 -0700 Mike Kravetz <[email protected]> wrote:

Quoted 4 lines

so perhaps we could put some
stopgap workaround into that site and add a runtime warning into the put_page() code somewhere to detect puttage of huge pages from hardirq and softirq contexts.

I think we would add the warning/etc at free_huge_page. The issue would only apply to hugetlb pages, not THP.

But, the more I think about it the more I think Aneesh's patch to do spin_lock/unlock_irqsave is the right way to go. Currently, we only know of one place where a put_page of hugetlb pages is done from softirq context. So, we could take the spin_lock/unlock_bh as Matthew suggested. When the powerpc iommu code was added, I doubt this was taken into account. I would be afraid of someone adding put_page from hardirq context.

Quoted 2 lines

Me too. If we're going to do this, surely we should make hugepages behave in the same fashion as PAGE_SIZE pages.

But these aren't vanilla hugepages, they're specifically hugetlbfs pages. I don't believe there's any problem with calling put_page() on a normally allocated huge page or THP.

BTW, free_huge_page called by put_page for hugetlbfs pages may also take a subpool specific lock via spin_lock(). See hugepage_subpool_put_pages. So, this would also need to take irq context into account.

Quoted 18 lines · Matthew Wilcox

Quoted 17 lines · Andrew Morton

Quoted 14 lines · On Wed, 5 Sep 2018 14:35:11 -0700 Mike Kravetz <[email protected]> wrote:

Quoted 4 lines

so perhaps we could put some
stopgap workaround into that site and add a runtime warning into the put_page() code somewhere to detect puttage of huge pages from hardirq and softirq contexts.

I think we would add the warning/etc at free_huge_page. The issue would only apply to hugetlb pages, not THP.

But, the more I think about it the more I think Aneesh's patch to do spin_lock/unlock_irqsave is the right way to go. Currently, we only know of one place where a put_page of hugetlb pages is done from softirq context. So, we could take the spin_lock/unlock_bh as Matthew suggested. When the powerpc iommu code was added, I doubt this was taken into account. I would be afraid of someone adding put_page from hardirq context.

Quoted 6 lines

Quoted 2 lines

Me too. If we're going to do this, surely we should make hugepages behave in the same fashion as PAGE_SIZE pages.

But these aren't vanilla hugepages, they're specifically hugetlbfs pages. I don't believe there's any problem with calling put_page() on a normally allocated huge page or THP.

Right
The powerpc iommu code (at least) treated hugetlbfs pages as any other page (huge, THP or base) and called put_page from softirq context.

It seems there are at least two ways to address this:
1) Prohibit this behavior for hugetlbfs pages
2) Try to make hugetlbfs pages behave like other pages WRT put_page

Quoted 11 lines · Andrew Morton

Quoted 8 lines · On Wed, 5 Sep 2018 06:48:48 -0700 Matthew Wilcox <[email protected]> wrote:

Quoted 5 lines

I didn't. The reason I looked at current patch is to enable the usage of put_page() from irq context. We do allow that for non hugetlb pages. So was not sure adding that additional restriction for hugetlb
is really needed. Further the conversion to irqsave/irqrestore was straightforward.

straightforward, sure. but is it the right thing to do? do we want to be able to put_page() a hugetlb page from hardirq context?

Quoted 7 lines

Calling put_page() against a huge page from hardirq seems like the right thing to do - even if it's rare now, it will presumably become more common as the hugepage virus spreads further across the kernel. And the present asymmetry is quite a wart.

That being said, arch/powerpc/mm/mmu_context_iommu.c:mm_iommu_free() is the only known site which does this (yes?)

IIUC, the powerpc iommu code 'remaps' user allocated hugetlb pages. It is these pages that are of issue at put_page time. I'll admit that code is new to me and I may not fully understand. However, if this is accurate then it makes it really difficult to track down any other similar usage patterns. I can not find a reference to PageHuge in the powerpc iommu code.

Quoted 5 lines

so perhaps we could put some
stopgap workaround into that site and add a runtime warning into the put_page() code somewhere to detect puttage of huge pages from hardirq and softirq contexts.

I think we would add the warning/etc at free_huge_page. The issue would only apply to hugetlb pages, not THP.

But, the more I think about it the more I think Aneesh's patch to do spin_lock/unlock_irqsave is the right way to go. Currently, we only know of one place where a put_page of hugetlb pages is done from softirq context. So, we could take the spin_lock/unlock_bh as Matthew suggested. When the powerpc iommu code was added, I doubt this was taken into account. I would be afraid of someone adding put_page from hardirq context.

Quoted 8 lines · On Wed, 5 Sep 2018 06:48:48 -0700 Matthew Wilcox <[email protected]> wrote:

Quoted 5 lines

I didn't. The reason I looked at current patch is to enable the usage of put_page() from irq context. We do allow that for non hugetlb pages. So was not sure adding that additional restriction for hugetlb
is really needed. Further the conversion to irqsave/irqrestore was straightforward.

straightforward, sure. but is it the right thing to do? do we want to be able to put_page() a hugetlb page from hardirq context?

Calling put_page() against a huge page from hardirq seems like the right thing to do - even if it's rare now, it will presumably become more common as the hugepage virus spreads further across the kernel. And the present asymmetry is quite a wart.

That being said, arch/powerpc/mm/mmu_context_iommu.c:mm_iommu_free() is the only known site which does this (yes?) so perhaps we could put some stopgap workaround into that site and add a runtime warning into the put_page() code somewhere to detect puttage of huge pages from hardirq and softirq contexts.

And attention will need to be paid to -stable backporting. How long has mm_iommu_free() existed, and been doing this?

Quoted 24 lines · Aneesh Kumar K.V

Quoted 22 lines · Matthew Wilcox

Quoted 1 line · Aneesh Kumar K.V

   inconsistent {SOFTIRQ-ON-W} -> {IN-SOFTIRQ-W} usage.

How do you go from "can be taken in softirq context" problem report to "must disable hard interrupts" solution? Please explain why spin_lock_bh() is not a sufficient fix.

Quoted 10 lines

   swapper/68/0 [HC0[0]:SC1[1]:HE1:SE0] takes:
   0000000052a030a7 (hugetlb_lock){+.?.}, at: free_huge_page+0x9c/0x340
   {SOFTIRQ-ON-W} state was registered at:
     lock_acquire+0xd4/0x230
     _raw_spin_lock+0x44/0x70
     set_max_huge_pages+0x4c/0x360
     hugetlb_sysctl_handler_common+0x108/0x160
     proc_sys_call_handler+0x134/0x190
     __vfs_write+0x3c/0x1f0
     vfs_write+0xd8/0x220

Also, this only seems to trigger here. Is it possible we _already_ have softirqs disabled through every other code path, and it's just this one sysctl handler that needs to disable softirqs? Rather than every lock access?

Quoted 1 line

Are you asking whether I looked at moving that put_page to a worker thread?

No. I'm asking "why not disable softirqs in the sysctl handler". Or perhaps equivalently, just replace spin_lock() with spin_lock_bh() in set_max_huge_pages().

Quoted 5 lines

I didn't. The reason I looked at current patch is to enable the usage of put_page() from irq context. We do allow that for non hugetlb pages. So was not sure adding that additional restriction for hugetlb
is really needed. Further the conversion to irqsave/irqrestore was straightforward.

straightforward, sure. but is it the right thing to do? do we want to be able to put_page() a hugetlb page from hardirq context?