Re: [5/5] pseries: Implement memory hotplug remove in the kernel

[Nathan Fontenot <nfont@linux.vnet.ibm.com>]

On 09/17/2014 02:07 AM, Michael Ellerman wrote:
> 
> On Mon, 2014-09-15 at 15:33 -0500, Nathan Fontenot wrote:
>> This patch adds the ability to do memory hotplug remove in the kernel.
>>
>> Currently the hotplug add/remove of memory is handled by the drmgr
>> command. The drmgr command performs the add/remove by performing
>> some work in user-space and making requests to the kernel to handle
>> other pieces. By moving all of the work to the kernel we can do the
>> add and remove faster, and provide a common place to do memory hotplug
>> for both the PowerVM and PowerKVM environments.
>>
>> Signed-off-by: Nathan Fontenot <nfont@linux.vnet.ibm.com>
>> ---
>>  arch/powerpc/platforms/pseries/hotplug-memory.c |  140 +++++++++++++++++++++++
>>  1 file changed, 139 insertions(+), 1 deletion(-)
>>
>> diff --git a/arch/powerpc/platforms/pseries/hotplug-memory.c b/arch/powerpc/platforms/pseries/hotplug-memory.c
>> index b254773..160c424 100644
>> --- a/arch/powerpc/platforms/pseries/hotplug-memory.c
>> +++ b/arch/powerpc/platforms/pseries/hotplug-memory.c
>> @@ -193,7 +193,137 @@ static int pseries_remove_mem_node(struct device_node *np)
>>  	pseries_remove_memblock(base, lmb_size);
>>  	return 0;
>>  }
>> +
>> +static int lmb_is_removable(struct of_drconf_cell *lmb)
>> +{
> 
> Do we not already have something like this?

No. Perhaps your thinking of the code in drivers/base/memory.c that
handles the sysfs removable file. That code just calls the same
is_mem_section_removable() routine.

> 
>> +	int i, scns_per_block;
>> +	int rc = 1;
> 
> I can see this makes the &= work below.
> 
> But what if block_sz / MIN_MEMORY_BLOCK_SIZE = 0 ?

If that happens, something else is really wrong. Most
likely a malformed device tree.

For pseries MIN_MEMORY_BLOCK_SIZE is defined to be the smallest
LMB size we suppport, 16MB.

I can add a pr_warn() statement here and bail if that happens.

> 
>> +	unsigned long pfn, block_sz;
>> +	u64 phys_addr;
>> +
>> +	phys_addr = be64_to_cpu(lmb->base_addr);
>> +	block_sz = memory_block_size_bytes();
>> +	scns_per_block = block_sz / MIN_MEMORY_BLOCK_SIZE;
>> +
>> +	for (i = 0; i < scns_per_block; i++) {
>> +		pfn = PFN_DOWN(phys_addr);
>> +		if (!pfn_present(pfn))
>> +			continue;
>> +
>> +		rc &= is_mem_section_removable(pfn, PAGES_PER_SECTION);
>> +		phys_addr += MIN_MEMORY_BLOCK_SIZE;
>> +	}
>> +
>> +	return rc;
>> +}
> 
>> +static int dlpar_memory_remove(struct pseries_hp_errorlog *hp_elog)
>> +{
> 
> ...
> 
>> +}
> 
> Most of the same comments as for add.
> 

ok, I'll go through them and apply them to the remove code.

Thanks for the review.

-Nathan