Re: [PATCH v3 3/3] acpi,memory-hotplug : add memory offline codeto acpi_memory_device_remove()

[Wen Congyang]

At 10/27/2012 01:14 AM, Toshi Kani Wrote:
> On Fri, 2012-10-26 at 18:31 +0800, wency@xxxxxxxxxxxxxx wrote:
>> From: Yasuaki Ishimatsu <isimatu.yasuaki@xxxxxxxxxxxxxx>
>>
>> The memory device can be removed by 2 ways:
>> 1. send eject request by SCI
>> 2. echo 1 >/sys/bus/pci/devices/PNP0C80:XX/eject
>>
>> In the 1st case, acpi_memory_disable_device() will be called.
>> In the 2nd case, acpi_memory_device_remove() will be called.
> 
> Hi Yasuaki, Wen,
> 
> Why do you need to have separate code design & implementation for the
> two cases?  In other words, can the 1st case simply use the same code
> path of the 2nd case, just like I did for the CPU hot-remove patch
> below?  It will simplify the code and make the memory notify handler
> more consistent with other handlers.
> https://lkml.org/lkml/2012/10/19/456

Yes, the 1st case can simply reuse the same code of the 2nd case.
It is another issue. The memory is not offlined and removed in 2nd
case. This patchset tries to fix this problem. After doing this,
we can merge the codes for the two cases.

But there is some bug in the code for 2nd case:
If offlining memory failed, we don't know such error in 2nd case, and
the kernel will in a dangerous state: the memory device is poweroffed
but the kernel is using it.

We should fix this bug before merging them.

Thanks
Wen Congyang

> 
> Thanks,
> -Toshi
> 
> 
>> acpi_memory_device_remove() will also be called when we unbind the
>> memory device from the driver acpi_memhotplug or a driver initialization
>> fails.
>>
>> acpi_memory_disable_device() has already implemented a code which
>> offlines memory and releases acpi_memory_info struct. But
>> acpi_memory_device_remove() has not implemented it yet.
>>
>> So the patch move offlining memory and releasing acpi_memory_info struct
>> codes to a new function acpi_memory_remove_memory(). And it is used by both
>> acpi_memory_device_remove() and acpi_memory_disable_device().
>>
>> CC: David Rientjes <rientjes@xxxxxxxxxx>
>> CC: Jiang Liu <liuj97@xxxxxxxxx>
>> CC: Len Brown <len.brown@xxxxxxxxx>
>> CC: Christoph Lameter <cl@xxxxxxxxx>
>> Cc: Minchan Kim <minchan.kim@xxxxxxxxx>
>> CC: Andrew Morton <akpm@xxxxxxxxxxxxxxxxxxxx>
>> CC: KOSAKI Motohiro <kosaki.motohiro@xxxxxxxxxxxxxx>
>> Signed-off-by: Yasuaki Ishimatsu <isimatu.yasuaki@xxxxxxxxxxxxxx>
>> Signed-off-by: Wen Congyang <wency@xxxxxxxxxxxxxx>
>> ---
>>  drivers/acpi/acpi_memhotplug.c | 31 ++++++++++++++++++++++++-------
>>  1 file changed, 24 insertions(+), 7 deletions(-)
>>
>> diff --git a/drivers/acpi/acpi_memhotplug.c b/drivers/acpi/acpi_memhotplug.c
>> index 666dac6..92c973a 100644
>> --- a/drivers/acpi/acpi_memhotplug.c
>> +++ b/drivers/acpi/acpi_memhotplug.c
>> @@ -316,16 +316,11 @@ static int acpi_memory_powerdown_device(struct acpi_memory_device *mem_device)
>>  	return 0;
>>  }
>>  
>> -static int acpi_memory_disable_device(struct acpi_memory_device *mem_device)
>> +static int acpi_memory_remove_memory(struct acpi_memory_device *mem_device)
>>  {
>>  	int result;
>>  	struct acpi_memory_info *info, *n;
>>  
>> -
>> -	/*
>> -	 * Ask the VM to offline this memory range.
>> -	 * Note: Assume that this function returns zero on success
>> -	 */
>>  	mutex_lock(&mem_device->list_lock);
>>  	list_for_each_entry_safe(info, n, &mem_device->res_list, list) {
>>  		if (info->enabled) {
>> @@ -333,10 +328,27 @@ static int acpi_memory_disable_device(struct acpi_memory_device *mem_device)
>>  			if (result)
>>  				return result;
>>  		}
>> +
>> +		list_del(&info->list);
>>  		kfree(info);
>>  	}
>>  	mutex_unlock(&mem_device->list_lock);
>>  
>> +	return 0;
>> +}
>> +
>> +static int acpi_memory_disable_device(struct acpi_memory_device *mem_device)
>> +{
>> +	int result;
>> +
>> +	/*
>> +	 * Ask the VM to offline this memory range.
>> +	 * Note: Assume that this function returns zero on success
>> +	 */
>> +	result = acpi_memory_remove_memory(mem_device);
>> +	if (result)
>> +		return result;
>> +
>>  	/* Power-off and eject the device */
>>  	result = acpi_memory_powerdown_device(mem_device);
>>  	if (result) {
>> @@ -487,12 +499,17 @@ static int acpi_memory_device_add(struct acpi_device *device)
>>  static int acpi_memory_device_remove(struct acpi_device *device, int type)
>>  {
>>  	struct acpi_memory_device *mem_device = NULL;
>> -
>> +	int result;
>>  
>>  	if (!device || !acpi_driver_data(device))
>>  		return -EINVAL;
>>  
>>  	mem_device = acpi_driver_data(device);
>> +
>> +	result = acpi_memory_remove_memory(mem_device);
>> +	if (result)
>> +		return result;
>> +
>>  	kfree(mem_device);
>>  
>>  	return 0;
> 
> 
> 

--
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/