On Wed, 2010-12-29 at 18:07 +0100, Daniel Kiper wrote:
> +config XEN_BALLOON_MEMORY_HOTPLUG
> + bool "Xen memory balloon driver with memory hotplug support"
> + default n
> + depends on XEN_BALLOON && MEMORY_HOTPLUG
> + help
> + Xen memory balloon driver with memory hotplug support allows expanding
> + memory available for the system above limit declared at system startup.
> + It is very useful on critical systems which require long run without
> + rebooting.
This might be better phrased as "Memory hotplug support for Xen balloon
driver". It might otherwise confuse people about whether they're seeing
some kind of choice or an _enhancement_ to the existing driver.
Also, why bother even making this a config option? What are the
downsides if it was always compiled in? You could even make it a
non-prompting Kconfig option and just automatically turn it on with
XEN_BALLOON && MEMORY_HOTPLUG.
> +static int allocate_memory_resource(struct resource **r, unsigned long nr_pages)
> {
> - unsigned long target = balloon_stats.target_pages;
> + int rc;
> + resource_size_t r_min, r_size;
> +
> + /*
> + * Look for first unused memory region starting at page
> + * boundary. Skip last memory section created at boot time
> + * becuase it may contains unused memory pages with PG_reserved
> + * bit not set (online_pages require PG_reserved bit set).
> + */
> +
> + *r = kzalloc(sizeof(struct resource), GFP_KERNEL);
>
> - target = min(target,
> - balloon_stats.current_pages +
> - balloon_stats.balloon_low +
> - balloon_stats.balloon_high);
> + if (!*r)
> + return -ENOMEM;
>
> - return target;
> + (*r)->name = "System RAM";
> + (*r)->flags = IORESOURCE_MEM | IORESOURCE_BUSY;
> + r_min = PFN_PHYS(section_nr_to_pfn(pfn_to_section_nr(balloon_stats.boot_max_pfn) + 1));
> + r_size = nr_pages << PAGE_SHIFT;
> +
> + rc = allocate_resource(&iomem_resource, *r, r_size, r_min,
> + ULONG_MAX, PAGE_SIZE, NULL, NULL);
> +
> + if (rc < 0) {
> + kfree(*r);
> + *r = NULL;
> + }
> +
> + return rc;
> }
The double-pointer stuff here ends up looking a little funky. Is there
any reason you don't just use ERR_PTRs? That might look a bit more
sane.
> +static void adjust_memory_resource(struct resource **r, unsigned long nr_pages)
> +{
> + if ((*r)->end + 1 - (nr_pages << PAGE_SHIFT) == (*r)->start) {
> + BUG_ON(release_resource(*r) < 0);
In some kernels, people do:
#define BUG_ON(...) do{}while(0)
to save space. If anyone ever does that with this code, it'll break
horribly. It's also hard to read these. So, please break logic actions
_out_ of the BUG_ON() arguments.
That's repeated in quite a few places in here. Make sure to go get them
all.
It also isn't evident what this patch set is trying to do until you get
down to this 7/7 patch. You might want to put a more complete
description in 0/7.
-- Dave
--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to majordomo@xxxxxxxxxx For more info on Linux MM,
see: http://www.linux-mm.org/ .
Fight unfair telecom policy in Canada: sign http://dissolvethecrtc.ca/
Don't email: <a href=mailto:"dont@xxxxxxxxx";> email@xxxxxxxxx </a>