On Fri, 2009-10-09 at 15:41 -0500, Robert Jennings wrote:
> The Collaborative Memory Manager (CMM) module allocates individual pages
> over time that are not migratable. On a long running system this can
> severely impact the ability to find enough pages to support a hotplug
> memory remove operation.
>
> This patch adds a memory isolation notifier and a memory hotplug notifier.
> The memory isolation notifier will return the number of pages found
> in the range specified. This is used to determine if all of the used
> pages in a pageblock are owned by the balloon (or other entities in
> the notifier chain). The hotplug notifier will free pages in the range
> which is to be removed. The priority of this hotplug notifier is low
> so that it will be called near last, this helps avoids removing loaned
> pages in operations that fail due to other handlers.
>
> CMM activity will be halted when hotplug remove operations are active
> and resume activity after a delay period to allow the hypervisor time
> to adjust.
>
> Signed-off-by: Robert Jennings <r...@linux.vnet.ibm.com>
Do you need me to merge that via the powerpc tree after the relevant
generic parts go in ? This is 2.6.33 material ?
> +module_param_named(hotplug_delay, hotplug_delay, uint, S_IRUGO | S_IWUSR);
> +MODULE_PARM_DESC(delay, "Delay (in seconds) after memory hotplug remove "
> + "before activity resumes. "
> + "[Default=" __stringify(CMM_HOTPLUG_DELAY) "]");
What is the above ? That sounds scary :-)
> module_param_named(oom_kb, oom_kb, uint, S_IRUGO | S_IWUSR);
> MODULE_PARM_DESC(oom_kb, "Amount of memory in kb to free on OOM. "
> "[Default=" __stringify(CMM_OOM_KB) "]");
> @@ -88,6 +101,8 @@ struct cmm_page_array {
> static unsigned long loaned_pages;
> static unsigned long loaned_pages_target;
> static unsigned long oom_freed_pages;
> +static atomic_t hotplug_active = ATOMIC_INIT(0);
> +static atomic_t hotplug_occurred = ATOMIC_INIT(0);
That sounds like a hand made lock with atomics... rarely a good idea,
tends to miss appropriate barriers etc...
> static struct cmm_page_array *cmm_page_list;
> static DEFINE_SPINLOCK(cmm_lock);
> @@ -110,6 +125,9 @@ static long cmm_alloc_pages(long nr)
> cmm_dbg("Begin request for %ld pages\n", nr);
>
> while (nr) {
> + if (atomic_read(&hotplug_active))
> + break;
> +
Ok so I'm not familiar with that whole memory hotplug stuff, so the code
might be right, but wouldn't the above be racy anyways in case hotplug
just becomes active after this statement ?
Shouldn't you use a mutex_trylock instead ? That has clearer semantics
and will provide the appropriate memory barriers.
> addr = __get_free_page(GFP_NOIO | __GFP_NOWARN |
> __GFP_NORETRY | __GFP_NOMEMALLOC);
> if (!addr)
> @@ -119,8 +137,10 @@ static long cmm_alloc_pages(long nr)
> if (!pa || pa->index >= CMM_NR_PAGES) {
> /* Need a new page for the page list. */
> spin_unlock(&cmm_lock);
> - npa = (struct cmm_page_array *)__get_free_page(GFP_NOIO
> | __GFP_NOWARN |
> -
> __GFP_NORETRY | __GFP_NOMEMALLOC);
> + npa = (struct cmm_page_array *)__get_free_page(
> + GFP_NOIO | __GFP_NOWARN |
> + __GFP_NORETRY | __GFP_NOMEMALLOC |
> + __GFP_MOVABLE);
> if (!npa) {
> pr_info("%s: Can not allocate new page list\n",
> __func__);
> free_page(addr);
> @@ -273,9 +293,23 @@ static int cmm_thread(void *dummy)
> while (1) {
> timeleft = msleep_interruptible(delay * 1000);
>
> - if (kthread_should_stop() || timeleft) {
> - loaned_pages_target = loaned_pages;
> + if (kthread_should_stop() || timeleft)
> break;
> +
> + if (atomic_read(&hotplug_active)) {
> + cmm_dbg("Hotplug operation in progress, activity "
> + "suspended\n");
> + continue;
> + }
> +
> + if (atomic_dec_if_positive(&hotplug_occurred) >= 0) {
> + cmm_dbg("Hotplug operation has occurred, loaning "
> + "activity suspended for %d seconds.\n",
> + hotplug_delay);
> + timeleft = msleep_interruptible(hotplug_delay * 1000);
> + if (kthread_should_stop() || timeleft)
> + break;
> + continue;
> }
I have less problems with hotplug_occured but if you use a
mutex_trylock, overall, you can turn the above into a normal int instead
of an atomic.
../..
> +static int cmm_memory_cb(struct notifier_block *self,
> + unsigned long action, void *arg)
> +{
> + int ret = 0;
> +
> + switch (action) {
> + case MEM_GOING_OFFLINE:
> + atomic_set(&hotplug_active, 1);
So that would become a mutex_lock(). Added advantage is that
it would wait for a current CMM operation to complete.
Cheers,
Ben.
_______________________________________________
Linuxppc-dev mailing list
Linuxppc-dev@lists.ozlabs.org
https://lists.ozlabs.org/listinfo/linuxppc-dev
