linux-kernel - Re: [PATCH R2 7/7] xen/balloon: Xen memory balloon driver with memory hotplug support

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <1294094002.18937.110.camel@nimitz>
Date:	Mon, 03 Jan 2011 14:33:22 -0800
From:	Dave Hansen <dave@...ux.vnet.ibm.com>
To:	Daniel Kiper <dkiper@...-space.pl>
Cc:	ian.campbell@...rix.com, akpm@...ux-foundation.org,
	andi.kleen@...el.com, haicheng.li@...ux.intel.com,
	fengguang.wu@...el.com, jeremy@...p.org, konrad.wilk@...cle.com,
	dan.magenheimer@...cle.com, v.tolstov@...fip.ru,
	xen-devel@...ts.xensource.com, linux-kernel@...r.kernel.org,
	linux-mm@...ck.org
Subject: Re: [PATCH R2 7/7] xen/balloon: Xen memory balloon driver with
 memory hotplug support

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 from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/