Message ID | 87bmzrpd37.fsf@concordia.ellerman.id.au (mailing list archive) |
---|---|
State | Not Applicable |
Headers | show |
On Wed, Sep 14, 2016 at 09:57:48AM +1000, Michael Ellerman wrote: > Anshuman Khandual <khandual@linux.vnet.ibm.com> writes: > > > When the HPT size is explicitly passed on from the userspace, currently > > the KVM_PPC_ALLOCATE_HTAB will try to allocate the requested size of HPT > > from reserved CMA area and if that is not possible, the allocation just > > fails. With the commit 572abd563befd56 ("KVM: PPC: Book3S HV: Don't fall > > back to smaller HPT size in allocation ioctl"), it does not even try to > > allocate the same order pages from the page allocator before failing for > > good. Same order allocation should be attempted from the page allocator > > as a fallback option when the CMA allocation attempt fails. > > It looks like if CMA is not configured we will just fail instantly. > > So this does look like something we should fix. > > But I think it is just a bug in commit 572abd563bef ("KVM: PPC: Book3S > HV: Don't fall back to smaller HPT size in allocation ioctl"), which did: > > diff --git a/arch/powerpc/kvm/book3s_64_mmu_hv.c b/arch/powerpc/kvm/book3s_64_mmu_hv.c > index 1f9c0a17f445..10722b1e38b5 100644 > --- a/arch/powerpc/kvm/book3s_64_mmu_hv.c > +++ b/arch/powerpc/kvm/book3s_64_mmu_hv.c > @@ -70,7 +70,8 @@ long kvmppc_alloc_hpt(struct kvm *kvm, u32 *htab_orderp) > } > > /* Lastly try successively smaller sizes from the page allocator */ > - while (!hpt && order > PPC_MIN_HPT_ORDER) { > + /* Only do this if userspace didn't specify a size via ioctl */ > + while (!hpt && order > PPC_MIN_HPT_ORDER && !htab_orderp) { > hpt = __get_free_pages(GFP_KERNEL|__GFP_ZERO|__GFP_REPEAT| > __GFP_NOWARN, order - PAGE_SHIFT); > if (!hpt) > > > Instead of guarding the loop entry with !htab_orderp, it should have > allowed the loop to enter, but prevented it from iterating if the > allocation fails and htab_orderp != 0. You're right. I'll fix it. Paul.
On 09/14/2016 05:27 AM, Michael Ellerman wrote: > Anshuman Khandual <khandual@linux.vnet.ibm.com> writes: > >> When the HPT size is explicitly passed on from the userspace, currently >> the KVM_PPC_ALLOCATE_HTAB will try to allocate the requested size of HPT >> from reserved CMA area and if that is not possible, the allocation just >> fails. With the commit 572abd563befd56 ("KVM: PPC: Book3S HV: Don't fall >> back to smaller HPT size in allocation ioctl"), it does not even try to >> allocate the same order pages from the page allocator before failing for >> good. Same order allocation should be attempted from the page allocator >> as a fallback option when the CMA allocation attempt fails. > > It looks like if CMA is not configured we will just fail instantly. Right and also we have this fallback registered any way. I wonder why we are still debating about the need of a fallback mechanism when we already have got one. > > So this does look like something we should fix. > > But I think it is just a bug in commit 572abd563bef ("KVM: PPC: Book3S > HV: Don't fall back to smaller HPT size in allocation ioctl"), which did: Hmm, I think its something the commit missed to accommodate for. But maybe yes, its a bug in the commit. > > diff --git a/arch/powerpc/kvm/book3s_64_mmu_hv.c b/arch/powerpc/kvm/book3s_64_mmu_hv.c > index 1f9c0a17f445..10722b1e38b5 100644 > --- a/arch/powerpc/kvm/book3s_64_mmu_hv.c > +++ b/arch/powerpc/kvm/book3s_64_mmu_hv.c > @@ -70,7 +70,8 @@ long kvmppc_alloc_hpt(struct kvm *kvm, u32 *htab_orderp) > } > > /* Lastly try successively smaller sizes from the page allocator */ > - while (!hpt && order > PPC_MIN_HPT_ORDER) { > + /* Only do this if userspace didn't specify a size via ioctl */ > + while (!hpt && order > PPC_MIN_HPT_ORDER && !htab_orderp) { > hpt = __get_free_pages(GFP_KERNEL|__GFP_ZERO|__GFP_REPEAT| > __GFP_NOWARN, order - PAGE_SHIFT); > if (!hpt) > > > Instead of guarding the loop entry with !htab_orderp, it should have > allowed the loop to enter, but prevented it from iterating if the > allocation fails and htab_orderp != 0. Right and thats what Aneesh's proposed patch (in the other thread) does.
On 09/14/2016 05:42 AM, Paul Mackerras wrote: > On Wed, Sep 14, 2016 at 09:57:48AM +1000, Michael Ellerman wrote: >> > Anshuman Khandual <khandual@linux.vnet.ibm.com> writes: >> > >>> > > When the HPT size is explicitly passed on from the userspace, currently >>> > > the KVM_PPC_ALLOCATE_HTAB will try to allocate the requested size of HPT >>> > > from reserved CMA area and if that is not possible, the allocation just >>> > > fails. With the commit 572abd563befd56 ("KVM: PPC: Book3S HV: Don't fall >>> > > back to smaller HPT size in allocation ioctl"), it does not even try to >>> > > allocate the same order pages from the page allocator before failing for >>> > > good. Same order allocation should be attempted from the page allocator >>> > > as a fallback option when the CMA allocation attempt fails. >> > >> > It looks like if CMA is not configured we will just fail instantly. >> > >> > So this does look like something we should fix. >> > >> > But I think it is just a bug in commit 572abd563bef ("KVM: PPC: Book3S >> > HV: Don't fall back to smaller HPT size in allocation ioctl"), which did: >> > >> > diff --git a/arch/powerpc/kvm/book3s_64_mmu_hv.c b/arch/powerpc/kvm/book3s_64_mmu_hv.c >> > index 1f9c0a17f445..10722b1e38b5 100644 >> > --- a/arch/powerpc/kvm/book3s_64_mmu_hv.c >> > +++ b/arch/powerpc/kvm/book3s_64_mmu_hv.c >> > @@ -70,7 +70,8 @@ long kvmppc_alloc_hpt(struct kvm *kvm, u32 *htab_orderp) >> > } >> > >> > /* Lastly try successively smaller sizes from the page allocator */ >> > - while (!hpt && order > PPC_MIN_HPT_ORDER) { >> > + /* Only do this if userspace didn't specify a size via ioctl */ >> > + while (!hpt && order > PPC_MIN_HPT_ORDER && !htab_orderp) { >> > hpt = __get_free_pages(GFP_KERNEL|__GFP_ZERO|__GFP_REPEAT| >> > __GFP_NOWARN, order - PAGE_SHIFT); >> > if (!hpt) >> > >> > >> > Instead of guarding the loop entry with !htab_orderp, it should have >> > allowed the loop to enter, but prevented it from iterating if the >> > allocation fails and htab_orderp != 0. > You're right. I'll fix it. Thanks Paul, so I will not be sending follow up patch on this.
On 09/14/2016 05:27 AM, Michael Ellerman wrote: > Anshuman Khandual <khandual@linux.vnet.ibm.com> writes: Not sure whether this mail ever went. Sending it again. > >> > When the HPT size is explicitly passed on from the userspace, currently >> > the KVM_PPC_ALLOCATE_HTAB will try to allocate the requested size of HPT >> > from reserved CMA area and if that is not possible, the allocation just >> > fails. With the commit 572abd563befd56 ("KVM: PPC: Book3S HV: Don't fall >> > back to smaller HPT size in allocation ioctl"), it does not even try to >> > allocate the same order pages from the page allocator before failing for >> > good. Same order allocation should be attempted from the page allocator >> > as a fallback option when the CMA allocation attempt fails. > It looks like if CMA is not configured we will just fail instantly. Right and also we have this fallback registered any way. I wonder why we are still debating about the need of a fallback mechanism when we already have got one. > > So this does look like something we should fix. > > But I think it is just a bug in commit 572abd563bef ("KVM: PPC: Book3S > HV: Don't fall back to smaller HPT size in allocation ioctl"), which did: Hmm, I think its something the commit missed to accommodate for. But maybe yes, its a bug in the commit. > > diff --git a/arch/powerpc/kvm/book3s_64_mmu_hv.c b/arch/powerpc/kvm/book3s_64_mmu_hv.c > index 1f9c0a17f445..10722b1e38b5 100644 > --- a/arch/powerpc/kvm/book3s_64_mmu_hv.c > +++ b/arch/powerpc/kvm/book3s_64_mmu_hv.c > @@ -70,7 +70,8 @@ long kvmppc_alloc_hpt(struct kvm *kvm, u32 *htab_orderp) > } > > /* Lastly try successively smaller sizes from the page allocator */ > - while (!hpt && order > PPC_MIN_HPT_ORDER) { > + /* Only do this if userspace didn't specify a size via ioctl */ > + while (!hpt && order > PPC_MIN_HPT_ORDER && !htab_orderp) { > hpt = __get_free_pages(GFP_KERNEL|__GFP_ZERO|__GFP_REPEAT| > __GFP_NOWARN, order - PAGE_SHIFT); > if (!hpt) > > > Instead of guarding the loop entry with !htab_orderp, it should have > allowed the loop to enter, but prevented it from iterating if the > allocation fails and htab_orderp != 0. Right and thats what Aneesh's proposed patch (in the other thread) does.
diff --git a/arch/powerpc/kvm/book3s_64_mmu_hv.c b/arch/powerpc/kvm/book3s_64_mmu_hv.c index 1f9c0a17f445..10722b1e38b5 100644 --- a/arch/powerpc/kvm/book3s_64_mmu_hv.c +++ b/arch/powerpc/kvm/book3s_64_mmu_hv.c @@ -70,7 +70,8 @@ long kvmppc_alloc_hpt(struct kvm *kvm, u32 *htab_orderp) } /* Lastly try successively smaller sizes from the page allocator */ - while (!hpt && order > PPC_MIN_HPT_ORDER) { + /* Only do this if userspace didn't specify a size via ioctl */ + while (!hpt && order > PPC_MIN_HPT_ORDER && !htab_orderp) { hpt = __get_free_pages(GFP_KERNEL|__GFP_ZERO|__GFP_REPEAT| __GFP_NOWARN, order - PAGE_SHIFT); if (!hpt)