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

[David Hildenbrand]

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