On 09/06/2018 03:05 AM, Mike Kravetz wrote:
On 09/05/2018 12:58 PM, Andrew Morton wrote:
On Wed, 5 Sep 2018 06:48:48 -0700 Matthew Wilcox <willy@xxxxxxxxxxxxx> wrote:
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?)
I guess so. It is the rcu callback to release the pinned pages.
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.
I don't know enough about vfio to comment about whether it is powerpc
specific. So the usage is w.r.t pass-through of usb device using vfio.
We do pin the entire guest ram. This pin is released later using the rcu
callbacks. We hit the issue when we use hugetlbfs backed guest ram.
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.
-aneesh
