On 08/30/2017 09:35 AM, Nathan Fontenot wrote:
> On 08/29/2017 09:35 PM, Michael Ellerman wrote:
>> John Allen <jal...@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 <jal...@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
>>
>
