Re: [PATCH 2/4] s390/sclp: Add support for dynamic (de)configuration of memory

[David Hildenbrand <david@redhat.com>]

>> Is there any reson this notifier is still needed? I'd assume we can just allow
>> for offlining + re-onlining as we please now.
>>
>> In fact, I'd assume we can get rid of the notifier entirely now?
> 
> I was initially uncertain about contains_standby_increment() use case
> and didnt change it here.  However, after testing by removing the
> contains_standby_increment() checks, I observed that the common memory
> hotplug code already prevents offlining a memory block that contains
> holes. This ensures safety without relying on these checks.
> 
> c5e79ef561b0 ("mm/memory_hotplug.c: don't allow to online/offline memory blocks with holes")

Rings a bell :)

> 
> i.e. #cp define storage 3504M standby 2148M
> This leads to a configuration where memory block 27 contains both
> assigned and standby incr.
> 
> But, offlining it will not succeed:
> chmem -d 0x00000000d8000000-0x00000000dfffffff
> chmem: Memory Block 27 (0x00000000d8000000-0x00000000dfffffff) disable
> failed: Invalid argument
> 
> Hence, I will remove it. Thanks.

Cool!

> 
>>> -	case MEM_PREPARE_ONLINE:
>>> -		/*
>>> -		 * Access the altmap_start_pfn and altmap_nr_pages fields
>>> -		 * within the struct memory_notify specifically when dealing
>>> -		 * with only MEM_PREPARE_ONLINE/MEM_FINISH_OFFLINE notifiers.
>>> -		 *
>>> -		 * When altmap is in use, take the specified memory range
>>> -		 * online, which includes the altmap.
>>> -		 */
>>> -		if (arg->altmap_nr_pages) {
>>> -			start = PFN_PHYS(arg->altmap_start_pfn);
>>> -			size += PFN_PHYS(arg->altmap_nr_pages);
>>> -		}
>>> -		rc = sclp_mem_change_state(start, size, 1);
>>> -		if (rc || !arg->altmap_nr_pages)
>>> -			break;
>>> -		/*
>>> -		 * Set CMMA state to nodat here, since the struct page memory
>>> -		 * at the beginning of the memory block will not go through the
>>> -		 * buddy allocator later.
>>> -		 */
>>> -		__arch_set_page_nodat((void *)__va(start), arg->altmap_nr_pages);
>>> +	default:
>>>    		break;
>>> -	case MEM_FINISH_OFFLINE:
>>> +	}
>>> +	return rc ? NOTIFY_BAD : NOTIFY_OK;
>>> +}
>>> +
>>> +static ssize_t config_mblock_show(struct kobject *kobj, struct kobj_attribute *attr, char *buf)
>>> +{
>>> +	struct mblock *mblock = container_of(kobj, struct mblock, kobj);
>>> +
>>> +	return sysfs_emit(buf, "%u\n", READ_ONCE(mblock->config));
>>> +}
>>> +
>>> +static ssize_t config_mblock_store(struct kobject *kobj, struct kobj_attribute *attr,
>>> +				   const char *buf, size_t count)
>>> +{
>>> +	unsigned long long addr, block_size;
>>
>> "unsigned long" should be sufficient I'm sure :)
> 
> Left over. I will do so.
> 
>>> +	struct memory_block *mem;
>>> +	struct mblock *mblock;
>>> +	unsigned char id;
>>> +	bool value;
>>> +	int rc;
>>> +
>>> +	rc = kstrtobool(buf, &value);
>>> +	if (rc)
>>> +		return rc;
>>> +	mblock = container_of(kobj, struct mblock, kobj);
>>> +	block_size = memory_block_size_bytes();
>>> +	addr = mblock->id * block_size;
>>> +	/*
>>> +	 * Hold device_hotplug_lock when adding/removing memory blocks.
>>> +	 * Additionally, also protect calls to find_memory_block() and
>>> +	 * sclp_attach_storage().
>>> +	 */
>>> +	rc = lock_device_hotplug_sysfs();
>>> +	if (rc)
>>> +		goto out;
>>> +	for_each_clear_bit(id, sclp_storage_ids, sclp_max_storage_id + 1)
>>> +		sclp_attach_storage(id);
>>> +	if (value) {
>>> +		if (mblock->config)
>>> +			goto out_unlock;
>>> +		rc = sclp_mem_change_state(addr, block_size, 1);
>>> +		if (rc)
>>> +			goto out_unlock;
>>>    		/*
>>> -		 * When altmap is in use, take the specified memory range
>>> -		 * offline, which includes the altmap.
>>> +		 * Set entire memory block CMMA state to nodat. Later, when
>>> +		 * page tables pages are allocated via __add_memory(), those
>>> +		 * regions are marked __arch_set_page_dat().
>>>    		 */
>>> -		if (arg->altmap_nr_pages) {
>>> -			start = PFN_PHYS(arg->altmap_start_pfn);
>>> -			size += PFN_PHYS(arg->altmap_nr_pages);
>>> +		__arch_set_page_nodat((void *)__va(addr), block_size >> PAGE_SHIFT);
>>> +		rc = __add_memory(0, addr, block_size,
>>> +				  mblock->memmap_on_memory ?
>>> +				  MHP_MEMMAP_ON_MEMORY | MHP_OFFLINE_INACCESSIBLE : MHP_NONE);
>>> +		if (rc)
>>> +			goto out_unlock;
>>
>> Do we have to undo the state change?
> 
> Intention was to keep error handling simple. In case of failure in
> add_memory(), we would have state set to 1 (not given back). But,
> subsequent configuration request for that block will not have an impact.

I mean, if we can cleanup easily here by doing another 
sclp_mem_change_state(), I think we should just do that.

I'd assume that sclp_mem_change_state() to 0 will usually not fail (I 
might be wrong :) ).

[...]

>>> -static int __init sclp_detect_standby_memory(void)
>>> +static int __init sclp_setup_memory(void)
>>>    {
>>>    	struct read_storage_sccb *sccb;
>>>    	int i, id, assigned, rc;
>>> +	struct mblock *mblocks;
>>> +	struct kset *kset;
>>>    	/* No standby memory in kdump mode */
>>>    	if (oldmem_data.start)
>>
>> Wouldn't we still want to create the ones for initial memory at least?
> 
> Intention was the following:
> configuration and deconfiguration of memory with optional
> memmap-on-memory is mostly needed for only standby memory.
> 
> If standby memory is absent or sclp is unavailable, we continue using
> the previous behavior (only software offline/online), since the sclp
> memory notifier was not registered in that case before either.

I mean, probably nobody in the kdump kernel cares about it either way, 
agreed.

-- 
Cheers

David / dhildenb