Message-ID: <faca65d7-6d4b-7e4f-5b36-4fdf3710b0e3@arm.com>
Date:   Tue, 12 Feb 2019 14:54:36 +0000
From:   Robin Murphy <robin.murphy@....com>
To:     Michal Hocko <mhocko@...nel.org>
Cc:     linux-mm@...ck.org, linux-kernel@...r.kernel.org,
        gregkh@...uxfoundation.org, rafael@...nel.org,
        akpm@...ux-foundation.org, osalvador@...e.de
Subject: Re: [PATCH v2] mm/memory-hotplug: Add sysfs hot-remove trigger

On 12/02/2019 08:33, Michal Hocko wrote:
> On Mon 11-02-19 17:50:46, Robin Murphy wrote:
>> ARCH_MEMORY_PROBE is a useful thing for testing and debugging hotplug,
>> but being able to exercise the (arguably trickier) hot-remove path would
>> be even more useful. Extend the feature to allow removal of offline
>> sections to be triggered manually to aid development.
>>
>> Since process dictates the new sysfs entry be documented, let's also
>> document the existing probe entry to match - better 13-and-a-half years
>> late than never, as they say...
> 
> The probe sysfs is quite dubious already TBH. Apart from testing, is
> anybody using it for something real? Do we need to keep an API for
> something testing only? Why isn't a customer testing module enough for
> such a purpose?

 From the arm64 angle, beyond "conventional" servers where we can 
hopefully assume ACPI, I can imagine there being embedded/HPC setups 
(not all as esoteric as that distributed-memory dRedBox thing), as well 
as virtual machines, that are DT-based with minimal runtime firmware. 
I'm none too keen on the idea either, but if such systems want to 
support physical hotplug then driving it from userspace might be the 
only reasonable approach. I'm just loath to actually document it as 
anything other than a developer feature so as not to give the impression 
that I consider it anything other than a last resort for production use. 
I do note that my x86 distro kernel has ARCH_MEMORY_PROBE enabled 
despite it being "for testing".

> In other words, why do we have to add an API that has to be maintained
> for ever for a testing only purpose?

There's already half the API being maintained, though, so adding the 
corresponding other half alongside it doesn't seem like that great an 
overhead, regardless of how it ends up getting used. Ultimately, though, 
it's a patch I wrote because I needed it, and if everyone else is 
adamant that it's not useful enough then fair enough - it's at least in 
the list archives now so I can sleep happy that I've done my 
"contributing back" bit as best I could :)

> Besides that, what is the reason to use __remove_memory rather than the
> exported remove_memory which does an additional locking.

For the same reason that probe uses __add_memory() rather than 
add_memory() - I can't claim to understand *exactly* why 
lock_device_hotplug_sysfs() does what it does compared to 
lock_device_hotplug() (even after reading 5e33bc4165f3), but I can only 
assume it's safest to be consistent with the other attributes here.

> Also, we do
> trust root to do sane things but are we sure that the current BUG-land
> mines in the hotplug code are ready enough to be exported for playing?

Well, the point of this particular implementation as opposed to other 
approaches is that it's impossible by construction to even attempt to 
remove something which isn't an exact, valid memory_block. AFAICS that 
should make it at least as robust as any other hot-remove caller.

Robin.

>> Signed-off-by: Robin Murphy <robin.murphy@....com>
>> ---
>>
>> v2: Use is_memblock_offlined() helper, write up documentation
>>
>>   .../ABI/testing/sysfs-devices-memory          | 25 +++++++++++
>>   drivers/base/memory.c                         | 42 ++++++++++++++++++-
>>   2 files changed, 66 insertions(+), 1 deletion(-)
>>
>> diff --git a/Documentation/ABI/testing/sysfs-devices-memory b/Documentation/ABI/testing/sysfs-devices-memory
>> index deef3b5723cf..02a4250964e0 100644
>> --- a/Documentation/ABI/testing/sysfs-devices-memory
>> +++ b/Documentation/ABI/testing/sysfs-devices-memory
>> @@ -91,3 +91,28 @@ Description:
>>   		memory section directory.  For example, the following symbolic
>>   		link is created for memory section 9 on node0.
>>   		/sys/devices/system/node/node0/memory9 -> ../../memory/memory9
>> +
>> +What:		/sys/devices/system/memory/probe
>> +Date:		October 2005
>> +Contact:	Linux Memory Management list <linux-mm@...ck.org>
>> +Description:
>> +		The file /sys/devices/system/memory/probe is write-only, and
>> +		when written will simulate a physical hot-add of a memory
>> +		section at the given address. For example, assuming a section
>> +		of unused memory exists at physical address 0x80000000, it can
>> +		be introduced to the kernel with the following command:
>> +		# echo 0x80000000 > /sys/devices/system/memory/probe
>> +Users:		Memory hotplug testing and development
>> +
>> +What:		/sys/devices/system/memory/memoryX/remove
>> +Date:		February 2019
>> +Contact:	Linux Memory Management list <linux-mm@...ck.org>
>> +Description:
>> +		The file /sys/devices/system/memory/memoryX/remove is
>> +		write-only, and when written with a boolean 'true' value will
>> +		simulate a physical hot-remove of that memory section. For
>> +		example, assuming a 1GB section size, the section added by the
>> +		above "probe" example could be removed again with the following
>> +		command:
>> +		# echo 1 > /sys/devices/system/memory/memory2/remove
>> +Users:		Memory hotplug testing and development
>> diff --git a/drivers/base/memory.c b/drivers/base/memory.c
>> index 048cbf7d5233..1ba9d1a6ba5e 100644
>> --- a/drivers/base/memory.c
>> +++ b/drivers/base/memory.c
>> @@ -521,7 +521,44 @@ static ssize_t probe_store(struct device *dev, struct device_attribute *attr,
>>   }
>>   
>>   static DEVICE_ATTR_WO(probe);
>> -#endif
>> +
>> +#ifdef CONFIG_MEMORY_HOTREMOVE
>> +static ssize_t remove_store(struct device *dev, struct device_attribute *attr,
>> +			    const char *buf, size_t count)
>> +{
>> +	struct memory_block *mem = to_memory_block(dev);
>> +	unsigned long start_pfn = section_nr_to_pfn(mem->start_section_nr);
>> +	bool remove;
>> +	int ret;
>> +
>> +	ret = kstrtobool(buf, &remove);
>> +	if (ret)
>> +		return ret;
>> +	if (!remove)
>> +		return count;
>> +
>> +	if (!is_memblock_offlined(mem))
>> +		return -EBUSY;
>> +
>> +	ret = lock_device_hotplug_sysfs();
>> +	if (ret)
>> +		return ret;
>> +
>> +	if (device_remove_file_self(dev, attr)) {
>> +		__remove_memory(pfn_to_nid(start_pfn), PFN_PHYS(start_pfn),
>> +				MIN_MEMORY_BLOCK_SIZE * sections_per_block);
>> +		ret = count;
>> +	} else {
>> +		ret = -EBUSY;
>> +	}
>> +
>> +	unlock_device_hotplug();
>> +	return ret;
>> +}
>> +
>> +static DEVICE_ATTR_WO(remove);
>> +#endif /* CONFIG_MEMORY_HOTREMOVE */
>> +#endif /* CONFIG_ARCH_MEMORY_PROBE */
>>   
>>   #ifdef CONFIG_MEMORY_FAILURE
>>   /*
>> @@ -615,6 +652,9 @@ static struct attribute *memory_memblk_attrs[] = {
>>   	&dev_attr_removable.attr,
>>   #ifdef CONFIG_MEMORY_HOTREMOVE
>>   	&dev_attr_valid_zones.attr,
>> +#ifdef CONFIG_ARCH_MEMORY_PROBE
>> +	&dev_attr_remove.attr,
>> +#endif
>>   #endif
>>   	NULL
>>   };
>> -- 
>> 2.20.1.dirty
> 

Powered by blists - more mailing lists