Re: [PATCH] resource: Improve child resource handling in release_mem_region_adjustable()

[David Hildenbrand <david@redhat.com>]

> 
> Hi David,

Hi!

> 
> I am working on the item related to the last discussion -  dynamic
> runtime (de)configuration of memory on s390:
> https://lore.kernel.org/all/aCx-SJdHd-3Z12af@li-2b55cdcc-350b-11b2-a85c-a78bff51fc11.ibm.com/
> 
> I came across the problem when I tried to offline and remove the memory
> via /sys/firmware/memory interface.

Ah, that makes sense. Sorry that I didn't immediately connect the dots 
when seeing your name.

> 
> I have also modified lsmem (not yet upstream) to list deconfigured
> memory, which currently appears as offline. An additional "configured"
> column is also introduced to show the configuration state, but it is not
> displayed here yet (without --output-all).

Worth mentioning in the patch description, otherwise it's confusing.

> 
>>> 0x0000000150000000-0x0000000157ffffff  128M offline               42
> 
> True, this will not be shown with the master lsmem, since the sysfs
> entry is removed after deconfiguration.
> 
>> Do we need a Fixes: and CC stable?
> 
> It will reference commit 825f787bb496 ("resource: add
> release_mem_region_adjustable()"). Since the commit already states
> "enhance this logic when necessary," I did not add a Fixes tag.

So if this cannot be triggered yet, all good and no need for Fixes:.

I was assuming that maybe this can be triggered with ppc dlpar, so I was 
concerned.

> 
>>> Signed-off-by: Sumanth Korikkar <sumanthk@linux.ibm.com>
>>> ---
>>>    kernel/resource.c | 44 +++++++++++++++++++++++++++++++++++++++-----
>>>    1 file changed, 39 insertions(+), 5 deletions(-)
>>>
>>> diff --git a/kernel/resource.c b/kernel/resource.c
>>> index f9bb5481501a..c329b8a4aa2f 100644
>>> --- a/kernel/resource.c
>>> +++ b/kernel/resource.c
>>> @@ -1388,6 +1388,41 @@ void __release_region(struct resource *parent, resource_size_t start,
>>>    EXPORT_SYMBOL(__release_region);
>>>    #ifdef CONFIG_MEMORY_HOTREMOVE
>>> +static void append_child_to_parent(struct resource *new_parent, struct resource *new_child)
>>> +{
>>> +	struct resource *child;
>>> +
>>> +	child = new_parent->child;
>>> +	if (child) {
>>> +		while (child->sibling)
>>> +			child = child->sibling;
>>> +		child->sibling = new_child;
>>
>> Shouldn't we take care of the address ordering here? I guess this works
>> because we process them in left-to-right (lowest-to-highest) address.
> 
> __request_resource() adds the child resources in the increasing order.
> With that, we dont need to check the ordering again here.  True, here we
> process the child resources from lowest to highest address.
> 
>>> +	} else {
>>> +		new_parent->child = new_child;
>>> +	}
>>> +	new_child->parent = new_parent;
>>> +	new_child->sibling = NULL;
>>> +}
>>> +
>>> +static void move_children_to_parent(struct resource *old_parent,
>>> +				    struct resource *new_parent,
>>> +				    resource_size_t split_addr)
>>
>> I'd call this "reparent_child_resources". But actually the function is
>> weird. Because you only reparents some resources from old to now.
>>
>> Two questions:
>>
>> a) Is split_addr really required. Couldn't we derive that from "old_parent"
> 
> old_parent->end points to old end range before the split, so I think it
> doesnt tell where the split boundary is, until __adjust_resource() is
> called. Hence, split_addr was added.

Makes sense, that's also where the sanity checks happen.

Worth throwing in a comment for the function telling that lower was not 
adjusted yet.

-- 
Cheers

David / dhildenb