Re: [Xen-devel] [PATCHv1 4/8] xen/balloon: find non-conflicting regions to place hotplugged memory

[Daniel Kiper <daniel.kiper@xxxxxxxxxx>]

On Thu, Jun 25, 2015 at 06:10:59PM +0100, David Vrabel wrote:
> Instead of placing hotplugged memory at the end of RAM (which may
> conflict with PCI devices or reserved regions) use allocate_resource()
> to get a new, suitably aligned resource that does not conflict.
>
> Signed-off-by: David Vrabel <david.vrabel@xxxxxxxxxx>

In general Reviewed-by: Daniel Kiper <daniel.kiper@xxxxxxxxxx>

but two nitpicks below...

> ---
>  drivers/xen/balloon.c |   63 
> +++++++++++++++++++++++++++++++++++++++++--------
>  1 file changed, 53 insertions(+), 10 deletions(-)
>
> diff --git a/drivers/xen/balloon.c b/drivers/xen/balloon.c
> index fd93369..d0121ee 100644
> --- a/drivers/xen/balloon.c
> +++ b/drivers/xen/balloon.c
> @@ -54,6 +54,7 @@
>  #include <linux/memory.h>
>  #include <linux/memory_hotplug.h>
>  #include <linux/percpu-defs.h>
> +#include <linux/slab.h>
>
>  #include <asm/page.h>
>  #include <asm/pgalloc.h>
> @@ -208,6 +209,42 @@ static bool balloon_is_inflated(void)
>               return false;
>  }
>
> +static struct resource *additional_memory_resource(phys_addr_t size)
> +{
> +     struct resource *res;
> +     int ret;
> +
> +     res = kzalloc(sizeof(*res), GFP_KERNEL);
> +     if (!res)
> +             return NULL;
> +
> +     res->name = "System RAM";
> +     res->flags = IORESOURCE_MEM | IORESOURCE_BUSY;
> +
> +     ret = allocate_resource(&iomem_resource, res,
> +                             size, 0, -1,
> +                             PAGES_PER_SECTION * PAGE_SIZE, NULL, NULL);
> +     if (ret < 0) {
> +             pr_err("Cannot allocate new System RAM resource\n");
> +             kfree(res);
> +             return NULL;
> +     }
> +
> +     return res;
> +}
> +
> +static void release_memory_resource(struct resource *resource)
> +{
> +     if (!resource)
> +             return;

Please add one empty line here.

> +     /*
> +      * No need to reset region to identity mapped since we now
> +      * know that no I/O can be in this region
> +      */
> +     release_resource(resource);
> +     kfree(resource);
> +}
> +
>  /*
>   * reserve_additional_memory() adds memory region of size >= credit above
>   * max_pfn. New region is section aligned and size is modified to be multiple
> @@ -221,13 +258,17 @@ static bool balloon_is_inflated(void)
>
>  static enum bp_state reserve_additional_memory(long credit)
>  {
> +     struct resource *resource;
>       int nid, rc;
> -     u64 hotplug_start_paddr;
> -     unsigned long balloon_hotplug = credit;
> +     unsigned long balloon_hotplug;
> +
> +     balloon_hotplug = round_up(credit, PAGES_PER_SECTION);
> +
> +     resource = additional_memory_resource(balloon_hotplug * PAGE_SIZE);
> +     if (!resource)
> +             goto err;
>
> -     hotplug_start_paddr = PFN_PHYS(SECTION_ALIGN_UP(max_pfn));
> -     balloon_hotplug = round_up(balloon_hotplug, PAGES_PER_SECTION);
> -     nid = memory_add_physaddr_to_nid(hotplug_start_paddr);
> +     nid = memory_add_physaddr_to_nid(resource->start);
>
>  #ifdef CONFIG_XEN_HAVE_PVMMU
>          /*
> @@ -242,21 +283,20 @@ static enum bp_state reserve_additional_memory(long 
> credit)
>       if (!xen_feature(XENFEAT_auto_translated_physmap)) {
>               unsigned long pfn, i;
>
> -             pfn = PFN_DOWN(hotplug_start_paddr);
> +             pfn = PFN_DOWN(resource->start);
>               for (i = 0; i < balloon_hotplug; i++) {
>                       if (!set_phys_to_machine(pfn + i, INVALID_P2M_ENTRY)) {
>                               pr_warn("set_phys_to_machine() failed, no 
> memory added\n");
> -                             return BP_ECANCELED;
> +                             goto err;
>                       }
>                  }
>       }
>  #endif
>
> -     rc = add_memory(nid, hotplug_start_paddr, balloon_hotplug << 
> PAGE_SHIFT);
> -
> +     rc = add_memory_resource(nid, resource);
>       if (rc) {
>               pr_warn("Cannot add additional memory (%i)\n", rc);
> -             return BP_ECANCELED;
> +             goto err;
>       }
>
>       balloon_hotplug -= credit;
> @@ -265,6 +305,9 @@ static enum bp_state reserve_additional_memory(long 
> credit)
>       balloon_stats.balloon_hotplug = balloon_hotplug;
>
>       return BP_DONE;

Ditto.

> +  err:
> +     release_memory_resource(resource);
> +     return BP_ECANCELED;
>  }
>
>  static void xen_online_page(struct page *page)
> --
> 1.7.10.4

Daniel

_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxx
http://lists.xen.org/xen-devel