Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 23.128.96.18 as permitted sender) client-ip=23.128.96.18;
Subject: Re: [PATCH v3] pseries/drmem: update LMBs after LPM
To:     Tyrel Datwyler <tyreld@linux.ibm.com>,
        "Aneesh Kumar K.V" <aneesh.kumar@linux.ibm.com>,
        mpe@ellerman.id.au, benh@kernel.crashing.org, paulus@samba.org
Cc:     nathanl@linux.ibm.com, linuxppc-dev@lists.ozlabs.org,
        linux-kernel@vger.kernel.org
References: <20210428094758.28665-1-ldufour@linux.ibm.com>
 <87fsz95qso.fsf@linux.ibm.com>
 <9d29bf8c-9e97-c179-6897-8e25fa4eb516@linux.ibm.com>
From:   Laurent Dufour <ldufour@linux.ibm.com>
Message-ID: <271ef351-b89c-ba68-3b6d-baa24cc0021b@linux.ibm.com>
Date:   Fri, 30 Apr 2021 18:13:28 +0200
User-Agent: Mozilla/5.0 (Macintosh; Intel Mac OS X 10.15; rv:78.0)
 Gecko/20100101 Thunderbird/78.10.0
MIME-Version: 1.0
In-Reply-To: <9d29bf8c-9e97-c179-6897-8e25fa4eb516@linux.ibm.com>
Content-Type: text/plain; charset=utf-8; format=flowed
Content-Language: en-US
Content-Transfer-Encoding: 8bit
Precedence: bulk

Le 29/04/2021 à 21:12, Tyrel Datwyler a écrit :
> On 4/29/21 3:27 AM, Aneesh Kumar K.V wrote:
>> Laurent Dufour <ldufour@linux.ibm.com> writes:
>>
>>> After a LPM, the device tree node ibm,dynamic-reconfiguration-memory may be
>>> updated by the hypervisor in the case the NUMA topology of the LPAR's
>>> memory is updated.
>>>
>>> This is caught by the kernel, but the memory's node is updated because
>>> there is no way to move a memory block between nodes.
>>>
>>> If later a memory block is added or removed, drmem_update_dt() is called
>>> and it is overwriting the DT node to match the added or removed LMB. But
>>> the LMB's associativity node has not been updated after the DT node update
>>> and thus the node is overwritten by the Linux's topology instead of the
>>> hypervisor one.
>>>
>>> Introduce a hook called when the ibm,dynamic-reconfiguration-memory node is
>>> updated to force an update of the LMB's associativity.
>>>
>>> Cc: Tyrel Datwyler <tyreld@linux.ibm.com>
>>> Signed-off-by: Laurent Dufour <ldufour@linux.ibm.com>
>>> ---
>>>
>>> V3:
>>>   - Check rd->dn->name instead of rd->dn->full_name
>>> V2:
>>>   - Take Tyrel's idea to rely on OF_RECONFIG_UPDATE_PROPERTY instead of
>>>   introducing a new hook mechanism.
>>> ---
>>>   arch/powerpc/include/asm/drmem.h              |  1 +
>>>   arch/powerpc/mm/drmem.c                       | 35 +++++++++++++++++++
>>>   .../platforms/pseries/hotplug-memory.c        |  4 +++
>>>   3 files changed, 40 insertions(+)
>>>
>>> diff --git a/arch/powerpc/include/asm/drmem.h b/arch/powerpc/include/asm/drmem.h
>>> index bf2402fed3e0..4265d5e95c2c 100644
>>> --- a/arch/powerpc/include/asm/drmem.h
>>> +++ b/arch/powerpc/include/asm/drmem.h
>>> @@ -111,6 +111,7 @@ int drmem_update_dt(void);
>>>   int __init
>>>   walk_drmem_lmbs_early(unsigned long node, void *data,
>>>   		      int (*func)(struct drmem_lmb *, const __be32 **, void *));
>>> +void drmem_update_lmbs(struct property *prop);
>>>   #endif
>>>   
>>>   static inline void invalidate_lmb_associativity_index(struct drmem_lmb *lmb)
>>> diff --git a/arch/powerpc/mm/drmem.c b/arch/powerpc/mm/drmem.c
>>> index 9af3832c9d8d..f0a6633132af 100644
>>> --- a/arch/powerpc/mm/drmem.c
>>> +++ b/arch/powerpc/mm/drmem.c
>>> @@ -307,6 +307,41 @@ int __init walk_drmem_lmbs_early(unsigned long node, void *data,
>>>   	return ret;
>>>   }
>>>   
>>> +/*
>>> + * Update the LMB associativity index.
>>> + */
>>> +static int update_lmb(struct drmem_lmb *updated_lmb,
>>> +		      __maybe_unused const __be32 **usm,
>>> +		      __maybe_unused void *data)
>>> +{
>>> +	struct drmem_lmb *lmb;
>>> +
>>> +	/*
>>> +	 * Brut force there may be better way to fetch the LMB
>>> +	 */
>>> +	for_each_drmem_lmb(lmb) {
>>> +		if (lmb->drc_index != updated_lmb->drc_index)
>>> +			continue;
>>> +
>>> +		lmb->aa_index = updated_lmb->aa_index;
>>> +		break;
>>> +	}
>>> +	return 0;
>>> +}
>>> +
>>> +/*
>>> + * Update the LMB associativity index.
>>> + *
>>> + * This needs to be called when the hypervisor is updating the
>>> + * dynamic-reconfiguration-memory node property.
>>> + */
>>> +void drmem_update_lmbs(struct property *prop)
>>> +{
>>> +	if (!strcmp(prop->name, "ibm,dynamic-memory"))
>>> +		__walk_drmem_v1_lmbs(prop->value, NULL, NULL, update_lmb);
>>> +	else if (!strcmp(prop->name, "ibm,dynamic-memory-v2"))
>>> +		__walk_drmem_v2_lmbs(prop->value, NULL, NULL, update_lmb);
>>> +}
>>>   #endif
>>>   
>>>   static int init_drmem_lmb_size(struct device_node *dn)
>>> diff --git a/arch/powerpc/platforms/pseries/hotplug-memory.c b/arch/powerpc/platforms/pseries/hotplug-memory.c
>>> index 8377f1f7c78e..672ffbee2e78 100644
>>> --- a/arch/powerpc/platforms/pseries/hotplug-memory.c
>>> +++ b/arch/powerpc/platforms/pseries/hotplug-memory.c
>>> @@ -949,6 +949,10 @@ static int pseries_memory_notifier(struct notifier_block *nb,
>>>   	case OF_RECONFIG_DETACH_NODE:
>>>   		err = pseries_remove_mem_node(rd->dn);
>>>   		break;
>>> +	case OF_RECONFIG_UPDATE_PROPERTY:
>>> +		if (!strcmp(rd->dn->name,
>>> +			    "ibm,dynamic-reconfiguration-memory"))
>>> +			drmem_update_lmbs(rd->prop);
>>>   	}
>>>   	return notifier_from_errno(err);
>>
>> How will this interact with DLPAR memory? When we dlpar memory,
>> ibm,configure-connector is used to fetch the new associativity details
>> and set drmem_lmb->aa_index correctly there. Once that is done kernel
>> then call drmem_update_dt() which will result in the above notifier
>> callback?
>>
>> IIUC, the call back then will update drmem_lmb->aa_index again?
> 
> After digging through some of this code I'm a bit concerned about all the kernel
> device tree manipulation around memory DLPAR both with the assoc-lookup-array
> prop update and post dynamic-memory prop updating. We build a drmem_info array
> of the LMBs from the device-tree at boot. I don't really understand why we are
> manipulating the device tree property every time we add/remove an LMB. Not sure
> the reasoning was to write back in particular the aa_index and flags for each
> LMB into the device tree when we already have them in the drmem_info array. On
> the other hand the assoc-lookup-array I suppose would need to have an in kernel
> representation to avoid updating the device tree property every time.

I think the reason is to keep the device tree in sync with the current set of LMBs.

My understanding is that the kernel is not really using the 
'ibm,dynamic-memory*' DT property once the boot is done. But user space tools 
(like lsslot and drmgr) read it to built the LMB tree and get the DRC index for 
each LMBs as it is not available in SYSFS.

> Changes to the device tree should be things reported to the system from the
> hypervisor through the proper interfaces, and as a result any code that cares
> can register an of_reconfig_notifier to resepond to device tree updates. The
> memory dlpar code seems to be needlessly manipulating the device-tree which
> leads to the problem here where a notifier callback is now duplicating work.

I don't think the hypervisor is expected to update the 'ibm,dynamic-memory' each 
time a LMB is added, this is not design this way AFAIK.

Laurent.

> Just my two cents FWIW.
> 
> -Tyrel
> 
>>
>> -aneesh
>>
>