[PATCH v6 3/3] powerpc/pseries: Implement indexed-count hotplug memory remove

Nathan Fontenot nfont at linux.vnet.ibm.com
Wed Nov 16 07:09:16 AEDT 2016


On 11/14/2016 06:58 PM, Michael Roth wrote:
> Quoting Nathan Fontenot (2016-10-18 12:21:06)
>> From: Sahil Mehta <sahilmehta17 at gmail.com>
>>
>> Indexed-count remove for memory hotplug guarantees that a contiguous block
>> of <count> lmbs beginning at a specified <index> will be unassigned (NOT
>> that <count> lmbs will be removed). Because of Qemu's per-DIMM memory
>> management, the removal of a contiguous block of memory currently
>> requires a series of individual calls. Indexed-count remove reduces
>> this series into a single call.
>>
>> Signed-off-by: Sahil Mehta <sahilmehta17 at gmail.com>
>> Signed-off-by: Nathan Fontenot <nfont at linux.vnet.ibm.com>
>> ---
>> v2:     -use u32s drc_index and count instead of u32 ic[]
>>          in dlpar_memory
>> v3:     -add logic to handle invalid drc_index input
>> v4:     -none
>> v5:     -Update for() loop to start at start_index
>> v6:     -none
>>
>>  arch/powerpc/platforms/pseries/hotplug-memory.c |   90 +++++++++++++++++++++++
>>  1 file changed, 90 insertions(+)
>>
>> diff --git a/arch/powerpc/platforms/pseries/hotplug-memory.c b/arch/powerpc/platforms/pseries/hotplug-memory.c
>> index badc66d..19ad081 100644
>> --- a/arch/powerpc/platforms/pseries/hotplug-memory.c
>> +++ b/arch/powerpc/platforms/pseries/hotplug-memory.c
>> @@ -558,6 +558,92 @@ static int dlpar_memory_remove_by_index(u32 drc_index, struct property *prop)
>>         return rc;
>>  }
>>
>> +static int dlpar_memory_remove_by_ic(u32 lmbs_to_remove, u32 drc_index,
>> +                                    struct property *prop)
>> +{
>> +       struct of_drconf_cell *lmbs;
>> +       u32 num_lmbs, *p;
>> +       int i, rc, start_lmb_found;
>> +       int lmbs_available = 0, start_index = 0, end_index;
>> +
>> +       pr_info("Attempting to hot-remove %u LMB(s) at %x\n",
>> +               lmbs_to_remove, drc_index);
>> +
>> +       if (lmbs_to_remove == 0)
>> +               return -EINVAL;
>> +
>> +       p = prop->value;
>> +       num_lmbs = *p++;
>> +       lmbs = (struct of_drconf_cell *)p;
>> +       start_lmb_found = 0;
>> +
>> +       /* Navigate to drc_index */
>> +       while (start_index < num_lmbs) {
>> +               if (lmbs[start_index].drc_index == drc_index) {
>> +                       start_lmb_found = 1;
>> +                       break;
>> +               }
>> +
>> +               start_index++;
>> +       }
>> +
>> +       if (!start_lmb_found)
>> +               return -EINVAL;
>> +
>> +       end_index = start_index + lmbs_to_remove;
>> +
>> +       /* Validate that there are enough LMBs to satisfy the request */
>> +       for (i = start_index; i < end_index; i++) {
>> +               if (lmbs[i].flags & DRCONF_MEM_RESERVED)
>> +                       break;
>> +
>> +               lmbs_available++;
>> +       }
>> +
>> +       if (lmbs_available < lmbs_to_remove)
>> +               return -EINVAL;
>> +
>> +       for (i = start_index; i < end_index; i++) {
>> +               if (!(lmbs[i].flags & DRCONF_MEM_ASSIGNED))
>> +                       continue;
>> +
>> +               rc = dlpar_remove_lmb(&lmbs[i]);
> 
> dlpar_remove_lmb() currently does both offlining of the memory as well
> as releasing the LMB back to the platform, but the specification for
> hotplug notifications has the following verbage regarding
> indexed-count/count identifiers:
> 
> 'When using “drc count” or “drc count indexed” as the Hotplug Identifier,
> the OS should take steps to verify the entirety of the request can be
> satisfied before proceeding with the hotplug / unplug operations. If
> only a partial count can be satisfied, the OS should ignore the entirety
> of the request. If the OS cannot determine this beforehand, it should
> satisfy the hotplug / unplug request for as many of the requested
> resources as possible, and attempt to revert to the original OS / DRC
> state.'
> 
> So doing the dlpar_remove->dlapr_add in case of failure is in line with
> the spec, but it should only be done as a last-resort. To me that
> suggests that we should be attempting to offline all the LMBs
> beforehand, and only after that's successful should we begin attempting
> to release LMBs back to the platform. Should we consider introducing
> that logic in the patchset? Or maybe as a follow-up?

Introducing it as a pre-cursor to this patch set may be best.

I'll send patches that split the dlpar_remove_lmb into a remove routine
that does the hotplug remove of the memory from the kernel and a new
dlpare_release_lmb routine that releases the drc-index and updates the
device tree. Plus a patch to make dlpar_add_lmb symmetrical.

-Nathan 

> 
>> +               if (rc)
>> +                       break;
>> +
>> +               lmbs[i].reserved = 1;
>> +       }
>> +
>> +       if (rc) {
>> +               pr_err("Memory indexed-count-remove failed, adding any removed LMBs\n");
>> +
>> +               for (i = start_index; i < end_index; i++) {
>> +                       if (!lmbs[i].reserved)
>> +                               continue;
>> +
>> +                       rc = dlpar_add_lmb(&lmbs[i]);
>> +                       if (rc)
>> +                               pr_err("Failed to add LMB, drc index %x\n",
>> +                                      be32_to_cpu(lmbs[i].drc_index));
>> +
>> +                       lmbs[i].reserved = 0;
>> +               }
>> +               rc = -EINVAL;
>> +       } else {
>> +               for (i = start_index; i < end_index; i++) {
>> +                       if (!lmbs[i].reserved)
>> +                               continue;
>> +
>> +                       pr_info("Memory at %llx (drc index %x) was hot-removed\n",
>> +                               lmbs[i].base_addr, lmbs[i].drc_index);
>> +
>> +                       lmbs[i].reserved = 0;
>> +               }
>> +       }
>> +
>> +       return rc;
>> +}
>> +
>>  #else
>>  static inline int pseries_remove_memblock(unsigned long base,
>>                                           unsigned int memblock_size)
>> @@ -853,6 +939,10 @@ int dlpar_memory(struct pseries_hp_errorlog *hp_elog)
>>                 } else if (hp_elog->id_type == PSERIES_HP_ELOG_ID_DRC_INDEX) {
>>                         drc_index = hp_elog->_drc_u.drc_index;
>>                         rc = dlpar_memory_remove_by_index(drc_index, prop);
>> +               } else if (hp_elog->id_type == PSERIES_HP_ELOG_ID_DRC_IC) {
>> +                       count = hp_elog->_drc_u.indexed_count[0];
>> +                       drc_index = hp_elog->_drc_u.indexed_count[1];
>> +                       rc = dlpar_memory_remove_by_ic(count, drc_index, prop);
>>                 } else {
>>                         rc = -EINVAL;
>>                 }
>>



More information about the Linuxppc-dev mailing list