Re: [RFC v2 3/3] postmigration/memory: Associativity & ibm,dynamic-memory-v2

[Nathan Fontenot]

On 04/24/2018 04:35 PM, Michael Bringmann wrote:
> See below.
> 
> On 04/24/2018 12:17 PM, Nathan Fontenot wrote:
>> On 02/26/2018 02:53 PM, Michael Bringmann wrote:
>>> postmigration/memory: Now apply changes to the associativity of memory
>>> blocks described by the 'ibm,dynamic-memory-v2' property regarding
>>> the topology of LPARS in Post Migration events.
>>>
>>> * Extend the previous work done for the 'ibm,associativity-lookup-array'
>>>   to apply to either property 'ibm,dynamic-memory' or
>>>   'ibm,dynamic-memory-v2', whichever is present.
>>> * Add new code to parse the 'ibm,dynamic-memory-v2' property looking
>>>   for differences in block 'assignment', associativity indexes per
>>>   block, and any other difference currently known.
>>> * Rewrite some of the original code to parse the 'ibm,dynamic-memory'
>>>   property to take advantage of LMB parsing code.
>>>
>>> When block differences are recognized, the memory block may be removed,
>>> added, or updated depending upon the state of the new device tree
>>> property and differences from the migrated value of the property.
>>>
>>
>> The only thing we need to check during LPM is affinity updates, memory
>> is not added or removed as part of LPM.
>>
>> I think a slightly different approach to this may be worth considering.
>> One of the goals of the drmem.c code was to remove the need to parse the
>> device tree for memory directly. For this update, I think we could modify
>> the code that builds the drmem_info data so that it can return a drmem_info
>> struct instead of assuming to set the global one.
>>
>> This change would allow you to do a straight compare on the global vs. the
>> new info from the updated device tree property. I think this would be cleaner
>> and may be able to use the same routine for V1 and V2 properties.
> 
> The code dealing with the 'ibm,associativity' array updated cleanly to use
> the same function to scan the LMBs regardless of the version of the 
> properties.
> 
> The code dealing with changes to 'ibm,dynamic-memory-v2' is a mirror of the
> code in 'pseries_update_drconf_memory' that deals with changes to the property
> 'ibm,dynamic-memory', so it should also be updated.  On the other hand, do we
> need to consider the memory requirements of creating/cloning the drmem_info
> structure to provide a copy based on the new 'dynamic-memory' property?
> Or is this not an issue?

If done correctly, using the drmem code to create a drmem_info struct from
the new memory property would allow us to use the same comparison routine
for v1 and v2 versions of the property.

The size of the drmem_info data is not big enough to be concerned about
memory requirements when making a copy. 

-Nathan

> 
>>
>>> Signed-off-by: Michael Bringmann 
>>> ---
>>> Changes in RFC v2:
>>>   -- Reuse existing parser code from 'drmem.c' in parsing property
>>>  'imb,dynamic-memory-v2' for migration.
>>>   -- Fix crash during migration that occurs on non-VPHN systems
>>>  when attempting to reset topology timer.
>>>   -- Change section of a support function + variable from __init 
>>>  to normal runtime to make them visible to migration code.
>>> ---
>>>  arch/powerpc/include/asm/drmem.h|8 +
>>>  arch/powerpc/mm/drmem.c |   23 ++-
>>>  arch/powerpc/mm/numa.c  |3 
>>>  arch/powerpc/platforms/pseries/hotplug-memory.c |  175 
>>> +++
>>>  drivers/of/fdt.c|4 -
>>>  include/linux/of_fdt.h  |2 
>>>  6 files changed, 170 insertions(+), 45 deletions(-)
>>>
>>> diff --git a/arch/powerpc/include/asm/drmem.h 
>>> b/arch/powerpc/include/asm/drmem.h
>>> index 47a7012..e4773c9 100644
>>> --- a/arch/powerpc/include/asm/drmem.h
>>> +++ b/arch/powerpc/include/asm/drmem.h
>>> @@ -92,6 +92,14 @@ void __init walk_drmem_lmbs(struct device_node *dn,
>>> void (*func)(struct drmem_lmb *, const __be32 **));
>>>  int drmem_update_dt(void);
>>>
>>> +void walk_drmem_v1_lmbs(const __be32 *prop, const __be32 *data,
>>> +   void (*func)(struct drmem_lmb *, const __be32 **));
>>> +
>>> +void read_drconf_v2_cell(struct of_drconf_cell_v2 *dr_cell,
>>> +   const __be32 **prop);
>>> +void walk_drmem_v2_lmbs(const __be32 *prop, const __be32 *data,
>>> +   void (*func)(struct drmem_lmb *, const __be32 **));
>>> +
>>>  #ifdef CONFIG_PPC_PSERIES
>>>  void __init walk_drmem_lmbs_early(unsigned long node,
>>> void (*func)(struct drmem_lmb *, const __be32 **));
>>> diff --git a/arch/powerpc/mm/drmem.c b/arch/powerpc/mm/drmem.c
>>> index 31dbe14..e47a6e0 100644
>>> --- a/arch/powerpc/mm/drmem.c
>>> +++ b/arch/powerpc/mm/drmem.c
>>> @@ -192,7 +192,7 @@ int drmem_update_dt(void)
>>> return rc;
>>>  }
>>>
>>> -static void __init read_drconf_v1_cell(struct drmem_lmb *lmb,
>>> +static void read_drconf_v1_cell(struct 

Re: [RFC v2 3/3] postmigration/memory: Associativity & ibm,dynamic-memory-v2

[Michael Bringmann]

See below.

On 04/24/2018 12:17 PM, Nathan Fontenot wrote:
> On 02/26/2018 02:53 PM, Michael Bringmann wrote:
>> postmigration/memory: Now apply changes to the associativity of memory
>> blocks described by the 'ibm,dynamic-memory-v2' property regarding
>> the topology of LPARS in Post Migration events.
>>
>> * Extend the previous work done for the 'ibm,associativity-lookup-array'
>>   to apply to either property 'ibm,dynamic-memory' or
>>   'ibm,dynamic-memory-v2', whichever is present.
>> * Add new code to parse the 'ibm,dynamic-memory-v2' property looking
>>   for differences in block 'assignment', associativity indexes per
>>   block, and any other difference currently known.
>> * Rewrite some of the original code to parse the 'ibm,dynamic-memory'
>>   property to take advantage of LMB parsing code.
>>
>> When block differences are recognized, the memory block may be removed,
>> added, or updated depending upon the state of the new device tree
>> property and differences from the migrated value of the property.
>>
> 
> The only thing we need to check during LPM is affinity updates, memory
> is not added or removed as part of LPM.
> 
> I think a slightly different approach to this may be worth considering.
> One of the goals of the drmem.c code was to remove the need to parse the
> device tree for memory directly. For this update, I think we could modify
> the code that builds the drmem_info data so that it can return a drmem_info
> struct instead of assuming to set the global one.
> 
> This change would allow you to do a straight compare on the global vs. the
> new info from the updated device tree property. I think this would be cleaner
> and may be able to use the same routine for V1 and V2 properties.

The code dealing with the 'ibm,associativity' array updated cleanly to use
the same function to scan the LMBs regardless of the version of the properties.

The code dealing with changes to 'ibm,dynamic-memory-v2' is a mirror of the
code in 'pseries_update_drconf_memory' that deals with changes to the property
'ibm,dynamic-memory', so it should also be updated.  On the other hand, do we
need to consider the memory requirements of creating/cloning the drmem_info
structure to provide a copy based on the new 'dynamic-memory' property?
Or is this not an issue?

> 
>> Signed-off-by: Michael Bringmann 
>> ---
>> Changes in RFC v2:
>>   -- Reuse existing parser code from 'drmem.c' in parsing property
>>  'imb,dynamic-memory-v2' for migration.
>>   -- Fix crash during migration that occurs on non-VPHN systems
>>  when attempting to reset topology timer.
>>   -- Change section of a support function + variable from __init 
>>  to normal runtime to make them visible to migration code.
>> ---
>>  arch/powerpc/include/asm/drmem.h|8 +
>>  arch/powerpc/mm/drmem.c |   23 ++-
>>  arch/powerpc/mm/numa.c  |3 
>>  arch/powerpc/platforms/pseries/hotplug-memory.c |  175 
>> +++
>>  drivers/of/fdt.c|4 -
>>  include/linux/of_fdt.h  |2 
>>  6 files changed, 170 insertions(+), 45 deletions(-)
>>
>> diff --git a/arch/powerpc/include/asm/drmem.h 
>> b/arch/powerpc/include/asm/drmem.h
>> index 47a7012..e4773c9 100644
>> --- a/arch/powerpc/include/asm/drmem.h
>> +++ b/arch/powerpc/include/asm/drmem.h
>> @@ -92,6 +92,14 @@ void __init walk_drmem_lmbs(struct device_node *dn,
>>  void (*func)(struct drmem_lmb *, const __be32 **));
>>  int drmem_update_dt(void);
>>
>> +void walk_drmem_v1_lmbs(const __be32 *prop, const __be32 *data,
>> +void (*func)(struct drmem_lmb *, const __be32 **));
>> +
>> +void read_drconf_v2_cell(struct of_drconf_cell_v2 *dr_cell,
>> +const __be32 **prop);
>> +void walk_drmem_v2_lmbs(const __be32 *prop, const __be32 *data,
>> +void (*func)(struct drmem_lmb *, const __be32 **));
>> +
>>  #ifdef CONFIG_PPC_PSERIES
>>  void __init walk_drmem_lmbs_early(unsigned long node,
>>  void (*func)(struct drmem_lmb *, const __be32 **));
>> diff --git a/arch/powerpc/mm/drmem.c b/arch/powerpc/mm/drmem.c
>> index 31dbe14..e47a6e0 100644
>> --- a/arch/powerpc/mm/drmem.c
>> +++ b/arch/powerpc/mm/drmem.c
>> @@ -192,7 +192,7 @@ int drmem_update_dt(void)
>>  return rc;
>>  }
>>
>> -static void __init read_drconf_v1_cell(struct drmem_lmb *lmb,
>> +static void read_drconf_v1_cell(struct drmem_lmb *lmb,
>> const __be32 **prop)
>>  {
>>  const __be32 *p = *prop;
>> @@ -208,7 +208,7 @@ static void __init read_drconf_v1_cell(struct drmem_lmb 
>> *lmb,
>>  *prop = p;
>>  }
>>
>> -static void __init __walk_drmem_v1_lmbs(const __be32 *prop, const __be32 
>> *usm,
>> +void walk_drmem_v1_lmbs(const __be32 *prop, const __be32 *data,
>>  void (*func)(struct drmem_lmb *, const __be32 **))
>> 

Re: [RFC v2 3/3] postmigration/memory: Associativity & ibm,dynamic-memory-v2

[Nathan Fontenot]

On 02/26/2018 02:53 PM, Michael Bringmann wrote:
> postmigration/memory: Now apply changes to the associativity of memory
> blocks described by the 'ibm,dynamic-memory-v2' property regarding
> the topology of LPARS in Post Migration events.
> 
> * Extend the previous work done for the 'ibm,associativity-lookup-array'
>   to apply to either property 'ibm,dynamic-memory' or
>   'ibm,dynamic-memory-v2', whichever is present.
> * Add new code to parse the 'ibm,dynamic-memory-v2' property looking
>   for differences in block 'assignment', associativity indexes per
>   block, and any other difference currently known.
> * Rewrite some of the original code to parse the 'ibm,dynamic-memory'
>   property to take advantage of LMB parsing code.
> 
> When block differences are recognized, the memory block may be removed,
> added, or updated depending upon the state of the new device tree
> property and differences from the migrated value of the property.
> 

The only thing we need to check during LPM is affinity updates, memory
is not added or removed as part of LPM.

I think a slightly different approach to this may be worth considering.
One of the goals of the drmem.c code was to remove the need to parse the
device tree for memory directly. For this update, I think we could modify
the code that builds the drmem_info data so that it can return a drmem_info
struct instead of assuming to set the global one.

This change would allow you to do a straight compare on the global vs. the
new info from the updated device tree property. I think this would be cleaner
and may be able to use the same routine for V1 and V2 properties.

> Signed-off-by: Michael Bringmann 
> ---
> Changes in RFC v2:
>   -- Reuse existing parser code from 'drmem.c' in parsing property
>  'imb,dynamic-memory-v2' for migration.
>   -- Fix crash during migration that occurs on non-VPHN systems
>  when attempting to reset topology timer.
>   -- Change section of a support function + variable from __init 
>  to normal runtime to make them visible to migration code.
> ---
>  arch/powerpc/include/asm/drmem.h|8 +
>  arch/powerpc/mm/drmem.c |   23 ++-
>  arch/powerpc/mm/numa.c  |3 
>  arch/powerpc/platforms/pseries/hotplug-memory.c |  175 
> +++
>  drivers/of/fdt.c|4 -
>  include/linux/of_fdt.h  |2 
>  6 files changed, 170 insertions(+), 45 deletions(-)
> 
> diff --git a/arch/powerpc/include/asm/drmem.h 
> b/arch/powerpc/include/asm/drmem.h
> index 47a7012..e4773c9 100644
> --- a/arch/powerpc/include/asm/drmem.h
> +++ b/arch/powerpc/include/asm/drmem.h
> @@ -92,6 +92,14 @@ void __init walk_drmem_lmbs(struct device_node *dn,
>   void (*func)(struct drmem_lmb *, const __be32 **));
>  int drmem_update_dt(void);
> 
> +void walk_drmem_v1_lmbs(const __be32 *prop, const __be32 *data,
> + void (*func)(struct drmem_lmb *, const __be32 **));
> +
> +void read_drconf_v2_cell(struct of_drconf_cell_v2 *dr_cell,
> + const __be32 **prop);
> +void walk_drmem_v2_lmbs(const __be32 *prop, const __be32 *data,
> + void (*func)(struct drmem_lmb *, const __be32 **));
> +
>  #ifdef CONFIG_PPC_PSERIES
>  void __init walk_drmem_lmbs_early(unsigned long node,
>   void (*func)(struct drmem_lmb *, const __be32 **));
> diff --git a/arch/powerpc/mm/drmem.c b/arch/powerpc/mm/drmem.c
> index 31dbe14..e47a6e0 100644
> --- a/arch/powerpc/mm/drmem.c
> +++ b/arch/powerpc/mm/drmem.c
> @@ -192,7 +192,7 @@ int drmem_update_dt(void)
>   return rc;
>  }
> 
> -static void __init read_drconf_v1_cell(struct drmem_lmb *lmb,
> +static void read_drconf_v1_cell(struct drmem_lmb *lmb,
>  const __be32 **prop)
>  {
>   const __be32 *p = *prop;
> @@ -208,7 +208,7 @@ static void __init read_drconf_v1_cell(struct drmem_lmb 
> *lmb,
>   *prop = p;
>  }
> 
> -static void __init __walk_drmem_v1_lmbs(const __be32 *prop, const __be32 
> *usm,
> +void walk_drmem_v1_lmbs(const __be32 *prop, const __be32 *data,
>   void (*func)(struct drmem_lmb *, const __be32 **))
>  {
>   struct drmem_lmb lmb;
> @@ -218,11 +218,12 @@ static void __init __walk_drmem_v1_lmbs(const __be32 
> *prop, const __be32 *usm,
> 
>   for (i = 0; i < n_lmbs; i++) {
>   read_drconf_v1_cell(, );
> - func(, );
> + func(, );
>   }
>  }
> +EXPORT_SYMBOL(walk_drmem_v1_lmbs);
> 
> -static void __init read_drconf_v2_cell(struct of_drconf_cell_v2 *dr_cell,
> +void read_drconf_v2_cell(struct of_drconf_cell_v2 *dr_cell,
>  const __be32 **prop)
>  {
>   const __be32 *p = *prop;
> @@ -235,8 +236,9 @@ static void __init read_drconf_v2_cell(struct 
> of_drconf_cell_v2 *dr_cell,
> 
>   *prop = p;
>  }
>