On Wed, Nov 29, 2017 at 11:38:24AM -0500, Serhii Popovych wrote:
> There are several points of improvements:
>
> 1) Make kvmppc_free_hpt() check if allocation is made before attempt
> to release. This follows kfree(p) semantics where p == NULL.
>
> 2) Return initialized @info parameter from kvmppc_allocate_hpt()
> even if allocation fails.
>
> This allows to use kvmppc_free_hpt() in the caller without
> checking that preceded kvmppc_allocate_hpt() was successful
>
> p = kmalloc(size, gfp);
> kfree(p);
>
> which is correct for both p != NULL and p == NULL. Followup
> change will rely on this behaviour.
>
> 3) Better code reuse: kvmppc_free_hpt() can be reused on error
> path in kvmppc_allocate_hpt() to avoid code duplication.
>
> 4) No need to check for !hpt if allocated from CMA: neither
> pfn_to_kaddr() nor page_to_pfn() is 0 in case of page != NULL.
>
> Signed-off-by: Serhii Popovych <[email protected]>
Reviewed-by: David Gibson <[email protected]>
> ---
> arch/powerpc/kvm/book3s_64_mmu_hv.c | 54 ++++++++++++++++++-------------------
> 1 file changed, 26 insertions(+), 28 deletions(-)
>
> diff --git a/arch/powerpc/kvm/book3s_64_mmu_hv.c b/arch/powerpc/kvm/book3s_64_mmu_hv.c
> index 0534aab..3e9abd9 100644
> --- a/arch/powerpc/kvm/book3s_64_mmu_hv.c
> +++ b/arch/powerpc/kvm/book3s_64_mmu_hv.c
> @@ -82,47 +82,44 @@ struct kvm_resize_hpt {
> int kvmppc_allocate_hpt(struct kvm_hpt_info *info, u32 order)
> {
> unsigned long hpt = 0;
> - int cma = 0;
> - struct page *page = NULL;
> - struct revmap_entry *rev;
> + int err, cma = 0;
> + struct page *page;
> + struct revmap_entry *rev = NULL;
> unsigned long npte;
>
> + err = -EINVAL;
> if ((order < PPC_MIN_HPT_ORDER) || (order > PPC_MAX_HPT_ORDER))
> - return -EINVAL;
> + goto out;
>
> + err = -ENOMEM;
> page = kvm_alloc_hpt_cma(1ul << (order - PAGE_SHIFT));
> if (page) {
> hpt = (unsigned long)pfn_to_kaddr(page_to_pfn(page));
> memset((void *)hpt, 0, (1ul << order));
> cma = 1;
> - }
> -
> - if (!hpt)
> + } else {
> hpt = __get_free_pages(GFP_KERNEL|__GFP_ZERO|__GFP_RETRY_MAYFAIL
> |__GFP_NOWARN, order - PAGE_SHIFT);
> -
> - if (!hpt)
> - return -ENOMEM;
> + if (!hpt)
> + goto out;
> + }
>
> /* HPTEs are 2**4 bytes long */
> npte = 1ul << (order - 4);
>
> /* Allocate reverse map array */
> - rev = vmalloc(sizeof(struct revmap_entry) * npte);
> - if (!rev) {
> - if (cma)
> - kvm_free_hpt_cma(page, 1 << (order - PAGE_SHIFT));
> - else
> - free_pages(hpt, order - PAGE_SHIFT);
> - return -ENOMEM;
> - }
> -
> + rev = vmalloc(sizeof(*rev) * npte);
> + if (rev)
> + err = 0;
> +out:
> info->order = order;
> info->virt = hpt;
> info->cma = cma;
> info->rev = rev;
>
> - return 0;
> + if (err)
> + kvmppc_free_hpt(info);
> + return err;
> }
>
> void kvmppc_set_hpt(struct kvm *kvm, struct kvm_hpt_info *info)
> @@ -190,12 +187,14 @@ void kvmppc_free_hpt(struct kvm_hpt_info *info)
> {
> vfree(info->rev);
> info->rev = NULL;
> - if (info->cma)
> - kvm_free_hpt_cma(virt_to_page(info->virt),
> - 1 << (info->order - PAGE_SHIFT));
> - else if (info->virt)
> - free_pages(info->virt, info->order - PAGE_SHIFT);
> - info->virt = 0;
> + if (info->virt) {
> + if (info->cma)
> + kvm_free_hpt_cma(virt_to_page(info->virt),
> + 1 << (info->order - PAGE_SHIFT));
> + else
> + free_pages(info->virt, info->order - PAGE_SHIFT);
> + info->virt = 0;
> + }
> info->order = 0;
> }
>
> @@ -1423,8 +1422,7 @@ static void resize_hpt_release(struct kvm *kvm, struct kvm_resize_hpt *resize)
> if (!resize)
> return;
>
> - if (resize->hpt.virt)
> - kvmppc_free_hpt(&resize->hpt);
> + kvmppc_free_hpt(&resize->hpt);
>
> kvm->arch.resize_hpt = NULL;
> kfree(resize);
--
David Gibson | I'll have my music baroque, and my code
david AT gibson.dropbear.id.au | minimalist, thank you. NOT _the_ _other_
| _way_ _around_!
http://www.ozlabs.org/~dgibson