Re: [PATCH] powerpc/pseries: Don't attempt to acquire drc during memory hot add for assigned lmbs

[John Allen <jallen@linux.vnet.ibm.com>]

On 08/30/2017 09:35 AM, Nathan Fontenot wrote:
> On 08/29/2017 09:35 PM, Michael Ellerman wrote:
>> John Allen <jallen@linux.vnet.ibm.com> writes:
>>
>>> Check if an LMB is assigned before attempting to call dlpar_acquire_drc in
>>> order to avoid any unnecessary rtas calls. This substantially reduces the
>>> running time of memory hot add on lpars with large amounts of memory.
>>>
>>> Signed-off-by: John Allen <jallen@linux.vnet.ibm.com>
>>> ---
>>> diff --git a/arch/powerpc/platforms/pseries/hotplug-memory.c b/arch/powerpc/platforms/pseries/hotplug-memory.c
>>> index ca9b2f4..95cf2ff 100644
>>> --- a/arch/powerpc/platforms/pseries/hotplug-memory.c
>>> +++ b/arch/powerpc/platforms/pseries/hotplug-memory.c
>>> @@ -817,6 +817,9 @@ static int dlpar_memory_add_by_count(u32 lmbs_to_add, struct property *prop)
>>>  		return -EINVAL;
>>>
>>>  	for (i = 0; i < num_lmbs && lmbs_to_add != lmbs_added; i++) {
>>> +		if (lmbs[i].flags & DRCONF_MEM_ASSIGNED)
>>> +			continue;
>>> +
>>>  		rc = dlpar_acquire_drc(lmbs[i].drc_index);
>>>  		if (rc)
>>>  			continue;
>>
>> This doesn't build for me, see below. What compiler are you using to
>> test this?> 
>>   arch/powerpc/platforms/pseries/hotplug-memory.c: In function 'dlpar_memory':
>>   arch/powerpc/platforms/pseries/hotplug-memory.c:1081:2: error: 'rc' may be used uninitialized in this function [-Werror=maybe-uninitialized]
>>     return rc;
>>     ^
>>
> 
> I don't see this build failure either, looks like its time to update my
> compiler too.

This also builds for me. I'm using gcc version 4.8.5

> 
>> It's a bit confusing because you didn't modify that function, but the
>> function you did modify has been inlined into there.
>>
>> Possibly the compiler is wrong and we do always initialise rc, but it's
>> not clear at all.
>>
>> And it raises a bigger question, how is this supposed to actually work?
>>
>> If we go around the loop and find that something is already assigned:
>>
>> 	for (i = 0; i < num_lmbs && lmbs_to_add != lmbs_added; i++) {
>> 		if (lmbs[i].flags & DRCONF_MEM_ASSIGNED)
>> 			continue;
>>                 ...
>> 		lmbs_added++;
>>                 ...
>>         }
>>
>> We don't increment lmbs_added, so at the end of the loop we will see
>> that lmbs_added is not equal to lmbs_to_add, and so we remove
>> everything:
> 
> There is a loop just before this code verifies there are enough
> LMBs available to satisfy the add request. This counts the number
> of LMBs that do not have the assigned flag set. This is where we
> update lmbs_available and if there are not enough lmbs available to
> satisfy the request, we bail.
> 
> When the loop you referenced above exits we should have attempted to
> enough LMBs to satisfy the request.
> 
>>
>> 	if (lmbs_added != lmbs_to_add) {
>> 		pr_err("Memory hot-add failed, removing any added LMBs\n");
>>
>> 		for (i = 0; i < num_lmbs; i++) {
>> 			if (!lmbs[i].reserved)
>> 				continue;
>>
>> 			rc = dlpar_remove_lmb(&lmbs[i]);
>>
>>
> 
> if we get into this recovery path, we only try to remove any LMBs
> that were added in the loop above. The code marks any LMB that is
> added as 'reserved' so that we only try to remove LMBs marked
> as 'reserved' in the cleanup path.
> 
>> So it seems like if we ever hit that continue you added, the whole
>> operation will fail anyway. So I'm confused.
> 
> The added code was to skip over any LMBs that are already assigned to
> the partition. As the code is written now, we walk through the entire list
> of LMBs possible. For large systems this can be tens, or hundreds of thousands
> of LMBs.
> 
> Without this check the code will try to acquire the drc for (this
> is done via rtas-set-indicator calls) which will fail for LMBs that are
> already assigned to the partition.
> 
> As an example on a large system that has 100,000 LMBs where the first
> 50,000 are assigned to the partition. When we get an add request to add
> X more LMBs we do see that there are 50,000 available to add. We then try
> to add X LMBs but start at the beginning of the LMB list when trying to
> add them. This results in 50,000 attempts to acquire the drc for LMbs
> that we already own. Instead we should just skip trying to acquire them.
> 
> Hopefully that helps clarify things.
> 
> -Nathan
> 
>>
>> cheers
>>
>