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

[Anshuman Khandual <khandual@linux.vnet.ibm.com>]

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.