Re: [PATCH 4/5 v3] kernel handling of memory DLPAR

[Nathan Fontenot]

Michael Ellerman wrote:
> 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()?

Ahhh.. I did not know of kstrdup().  I will update to use this instead.

> > +	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() ?

ok, will switch to of_find_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.

yes, will update.

> > +
> > +	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.

Agreed.  I think I will make this update in a separate patch.  Updating
this to use a of_drconf struct would benefit this code as well as the code
in powerpc/numa.c that also deals with manipulating this property.

> > +	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?

Yes.  In this case the lmb is already assigned to the system.

> > +	}
> > +
> > +	rc = acquire_drc(drmem[i].drc_index);
> > +	if (rc) {
> > +		of_node_put(dn);
> > +		return -1;
>
> -1 ?

Yeah, bad choice.

> > +	}
> > +
> > +	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 ?

Another bad return code choice.

> > +}
> > +
> > +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?

... and again...

> > +
> > +	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.

Yes, this is possible.  I will clean this up.

> > +	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?

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 :)

I didn't know any other way to implement this.  If using a weak symbol is ok
I will leave it.  I am open to suggestions if there is a better way to do this.

thanks for reviewing the patch.

-Nathan

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