diff mbox

[RFC] KVM: PPC: Book3S HV: Fall back to same size HPT in allocation ioctl

Message ID 87bmzrpd37.fsf@concordia.ellerman.id.au (mailing list archive)
State Not Applicable
Headers show

Commit Message

Michael Ellerman Sept. 13, 2016, 11:57 p.m. UTC
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:



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.

cheers

Comments

Paul Mackerras Sept. 14, 2016, 12:12 a.m. UTC | #1
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.
Anshuman Khandual Sept. 14, 2016, 3:52 a.m. UTC | #2
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.
Anshuman Khandual Sept. 14, 2016, 3:55 a.m. UTC | #3
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.
Anshuman Khandual Sept. 14, 2016, 5:28 a.m. UTC | #4
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 mbox

Patch

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)