Re: [RFC PATCH] mm/hugetlb: make hugetlb_lock irq safe

[Aneesh Kumar K.V]

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