From: Michael Ellerman <michael@ellerman•id.au>
To: Nathan Fontenot <nfont@austin•ibm.com>
Cc: linuxppc-dev@ozlabs•org, linux-kernel@vger•kernel.org
Subject: Re: [PATCH 4/5 v3] kernel handling of memory DLPAR
Date: Wed, 14 Oct 2009 09:31:01 +1100 [thread overview]
Message-ID: <1255473061.21871.40.camel@concordia> (raw)
In-Reply-To: <4AD4C346.20801@austin.ibm.com>
[-- Attachment #1: Type: text/plain, Size: 6749 bytes --]
On Tue, 2009-10-13 at 13:13 -0500, Nathan Fontenot wrote:
> This adds the capability to DLPAR add and remove memory from the kernel. The
Hi Nathan,
Sorry to only get around to reviewing version 3, time is a commodity in
short supply :)
> Index: powerpc/arch/powerpc/platforms/pseries/dlpar.c
> ===================================================================
> --- powerpc.orig/arch/powerpc/platforms/pseries/dlpar.c 2009-10-08 11:08:42.000000000 -0500
> +++ powerpc/arch/powerpc/platforms/pseries/dlpar.c 2009-10-13 13:08:22.000000000 -0500
> @@ -16,6 +16,10 @@
> #include <linux/notifier.h>
> #include <linux/proc_fs.h>
> #include <linux/spinlock.h>
> +#include <linux/memory_hotplug.h>
> +#include <linux/sysdev.h>
> +#include <linux/sysfs.h>
> +
>
> #include <asm/prom.h>
> #include <asm/machdep.h>
> @@ -404,11 +408,165 @@
> return 0;
> }
>
> +#ifdef CONFIG_MEMORY_HOTPLUG
> +
> +static struct property *clone_property(struct property *old_prop)
> +{
> + struct property *new_prop;
> +
> + new_prop = kzalloc((sizeof *new_prop), GFP_KERNEL);
> + if (!new_prop)
> + return NULL;
> +
> + new_prop->name = kzalloc(strlen(old_prop->name) + 1, GFP_KERNEL);
kstrdup()?
> + new_prop->value = kzalloc(old_prop->length + 1, GFP_KERNEL);
> + if (!new_prop->name || !new_prop->value) {
> + free_property(new_prop);
> + return NULL;
> + }
> +
> + strcpy(new_prop->name, old_prop->name);
> + memcpy(new_prop->value, old_prop->value, old_prop->length);
> + new_prop->length = old_prop->length;
> +
> + return new_prop;
> +}
> +
> +int platform_probe_memory(u64 phys_addr)
> +{
> + struct device_node *dn;
> + struct property *new_prop, *old_prop;
> + struct property *lmb_sz_prop;
> + struct of_drconf_cell *drmem;
> + u64 lmb_size;
> + int num_entries, i, rc;
> +
> + if (!phys_addr)
> + return -EINVAL;
> +
> + dn = of_find_node_by_path("/ibm,dynamic-reconfiguration-memory");
> + if (!dn)
> + return -EINVAL;
> +
> + lmb_sz_prop = of_find_property(dn, "ibm,lmb-size", NULL);
> + lmb_size = *(u64 *)lmb_sz_prop->value;
of_get_property() ?
> +
> + old_prop = of_find_property(dn, "ibm,dynamic-memory", NULL);
I know we should never fail to find these properties, but it would be
nice to check just in case.
> +
> + num_entries = *(u32 *)old_prop->value;
> + drmem = (struct of_drconf_cell *)
> + ((char *)old_prop->value + sizeof(u32));
You do this dance twice (see below), a struct might make it cleaner.
> + for (i = 0; i < num_entries; i++) {
> + u64 lmb_end_addr = drmem[i].base_addr + lmb_size;
> + if (phys_addr >= drmem[i].base_addr
> + && phys_addr < lmb_end_addr)
> + break;
> + }
> +
> + if (i >= num_entries) {
> + of_node_put(dn);
> + return -EINVAL;
> + }
> +
> + if (drmem[i].flags & DRCONF_MEM_ASSIGNED) {
> + of_node_put(dn);
> + return 0;
This is the already added case?
> + }
> +
> + rc = acquire_drc(drmem[i].drc_index);
> + if (rc) {
> + of_node_put(dn);
> + return -1;
-1 ?
> + }
> +
> + new_prop = clone_property(old_prop);
> + drmem = (struct of_drconf_cell *)
> + ((char *)new_prop->value + sizeof(u32));
> +
> + drmem[i].flags |= DRCONF_MEM_ASSIGNED;
> + prom_update_property(dn, new_prop, old_prop);
> +
> + rc = blocking_notifier_call_chain(&pSeries_reconfig_chain,
> + PSERIES_DRCONF_MEM_ADD,
> + &drmem[i].base_addr);
> + if (rc == NOTIFY_BAD) {
> + prom_update_property(dn, old_prop, new_prop);
> + release_drc(drmem[i].drc_index);
> + }
> +
> + of_node_put(dn);
> + return rc == NOTIFY_BAD ? -1 : 0;
-1 ?
> +}
> +
> +static ssize_t memory_release_store(struct class *class, const char *buf,
> + size_t count)
> +{
> + unsigned long drc_index;
> + struct device_node *dn;
> + struct property *new_prop, *old_prop;
> + struct of_drconf_cell *drmem;
> + int num_entries;
> + int i, rc;
> +
> + rc = strict_strtoul(buf, 0, &drc_index);
> + if (rc)
> + return -EINVAL;
> +
> + dn = of_find_node_by_path("/ibm,dynamic-reconfiguration-memory");
> + if (!dn)
> + return 0;
0 really?
> +
> + old_prop = of_find_property(dn, "ibm,dynamic-memory", NULL);
> + new_prop = clone_property(old_prop);
> +
> + num_entries = *(u32 *)new_prop->value;
> + drmem = (struct of_drconf_cell *)
> + ((char *)new_prop->value + sizeof(u32));
> +
> + for (i = 0; i < num_entries; i++) {
> + if (drmem[i].drc_index == drc_index)
> + break;
> + }
> +
> + if (i >= num_entries) {
> + free_property(new_prop);
> + of_node_put(dn);
> + return -EINVAL;
> + }
Couldn't use old_prop up until here? They're identical aren't they, so
you can do the clone here and you can avoid the free in the above error
case.
> + drmem[i].flags &= ~DRCONF_MEM_ASSIGNED;
> + prom_update_property(dn, new_prop, old_prop);
> +
> + rc = blocking_notifier_call_chain(&pSeries_reconfig_chain,
> + PSERIES_DRCONF_MEM_REMOVE,
> + &drmem[i].base_addr);
> + if (rc != NOTIFY_BAD)
> + rc = release_drc(drc_index);
> +
> + if (rc)
> + prom_update_property(dn, old_prop, new_prop);
> +
> + of_node_put(dn);
> + return rc ? -1 : count;
-1, EPERM?
> +}
> +
> +static struct class_attribute class_attr_mem_release =
> + __ATTR(release, S_IWUSR, NULL, memory_release_store);
> +#endif
> +
> static int pseries_dlpar_init(void)
> {
> if (!machine_is(pseries))
> return 0;
>
> +#ifdef CONFIG_MEMORY_HOTPLUG
> + if (sysfs_create_file(&memory_sysdev_class.kset.kobj,
> + &class_attr_mem_release.attr))
> + printk(KERN_INFO "DLPAR: Could not create sysfs memory "
> + "release file\n");
> +#endif
> +
> return 0;
> }
> device_initcall(pseries_dlpar_init);
> Index: powerpc/arch/powerpc/mm/mem.c
> ===================================================================
> --- powerpc.orig/arch/powerpc/mm/mem.c 2009-10-08 11:07:45.000000000 -0500
> +++ powerpc/arch/powerpc/mm/mem.c 2009-10-08 11:08:54.000000000 -0500
> @@ -111,8 +111,19 @@
> #ifdef CONFIG_MEMORY_HOTPLUG
>
> #ifdef CONFIG_NUMA
> +int __attribute ((weak)) platform_probe_memory(u64 start)
__weak
Though be careful, I think this is vulnerable to a bug in some
toolchains where the compiler will inline this version. See the comment
around early_irq_init() in kernel/softirq.c for example.
This will need to be a ppc_md hook as soon as another platform supports
memory hotplug, though that may be never :)
> +{
> + return 0;
> +}
> +
> int memory_add_physaddr_to_nid(u64 start)
> {
> + int rc;
> +
> + rc = platform_probe_memory(start);
> + if (rc)
> + return rc;
> +
> return hot_add_scn_to_nid(start);
> }
> #endif
cheers
[-- Attachment #2: This is a digitally signed message part --]
[-- Type: application/pgp-signature, Size: 197 bytes --]
next prev parent reply other threads:[~2009-10-13 22:31 UTC|newest]
Thread overview: 14+ messages / expand[flat|nested] mbox.gz Atom feed top
2009-09-18 14:56 [PATCH 0/5 v2] kernel handling of dynamic logical partitioning Nathan Fontenot
2009-09-18 14:59 ` [PATCH 1/5 v2] dynamic logical partitioning infrastructure Nathan Fontenot
2009-10-13 18:06 ` [PATCH 1/5 v3] " Nathan Fontenot
2009-09-18 15:01 ` [PATCH 2/5 v2] move of_drconf_cell definition to prom.h Nathan Fontenot
2009-09-18 15:02 ` [PATCH 3/5 v2] Export memory_sysdev_class Nathan Fontenot
2009-09-18 15:03 ` [PATCH 4/5 v2] kernel handling of memory DLPAR Nathan Fontenot
2009-10-13 18:13 ` [PATCH 4/5 v3] " Nathan Fontenot
2009-10-13 22:31 ` Michael Ellerman [this message]
2009-10-15 15:23 ` Nathan Fontenot
2009-09-18 15:04 ` [PATCH 5/5 v2] kernel handling of CPU DLPAR Nathan Fontenot
2009-10-13 18:14 ` Nathan Fontenot
2009-10-13 22:30 ` Michael Ellerman
2009-10-15 15:40 ` Nathan Fontenot
2009-10-16 0:52 ` Michael Ellerman
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=1255473061.21871.40.camel@concordia \
--to=michael@ellerman$(echo .)id.au \
--cc=linux-kernel@vger$(echo .)kernel.org \
--cc=linuxppc-dev@ozlabs$(echo .)org \
--cc=nfont@austin$(echo .)ibm.com \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox