On 05/18/2018 02:28 PM, Nathan Fontenot wrote: > On 05/17/2018 05:26 PM, Michael Bringmann wrote: >> powerpc migration/cpu: Now apply changes to the associativity of cpus >> for the topology of LPARS in Post Migration events. Recognize more >> changes to the associativity of memory blocks described by the >> 'cpu' properties when processing the topology of LPARS in Post Migration >> events. Previous efforts only recognized whether a memory block's >> assignment had changed in the property. Changes here include: >> >> * Provide hotplug CPU 'readd by index' operation >> * Checking for changes in cpu associativity and making 'readd' calls >> when differences are observed. >> * Queue up changes to CPU properties so that they may take place >> after all PowerPC device-tree changes have been applied i.e. after >> the device hotplug is released in the mobility code. > > This kinda feels like three different patches in one. any reason to not split > this into three patches? Perhaps at the least split the last item into it's > own patch.
E.g. 1) Conditional 'acquire_drc' 2) PSERIES_HP_ELOG_ACTION_READD call to dlpar_cpu_readd_by_index 3) Add lock/unlock device hotplug > >> >> Signed-off-by: Michael Bringmann <m...@linux.vnet.ibm.com> >> --- >> Changes include: >> -- Rearrange patches to co-locate CPU property-related changes. >> -- Modify dlpar_cpu_add & dlpar_cpu_remove to skip DRC index acquire >> or release operations during the CPU readd process. >> -- Correct a bug in DRC index selection for queued operation. >> -- Rebase to 4.17-rc5 kernel >> --- >> arch/powerpc/platforms/pseries/hotplug-cpu.c | 123 >> +++++++++++++++++++------- >> arch/powerpc/platforms/pseries/mobility.c | 3 + >> 2 files changed, 95 insertions(+), 31 deletions(-) >> >> diff --git a/arch/powerpc/platforms/pseries/hotplug-cpu.c >> b/arch/powerpc/platforms/pseries/hotplug-cpu.c >> index a408217..23d4cb8 100644 >> --- a/arch/powerpc/platforms/pseries/hotplug-cpu.c >> +++ b/arch/powerpc/platforms/pseries/hotplug-cpu.c >> @@ -474,7 +474,7 @@ static bool valid_cpu_drc_index(struct device_node >> *parent, u32 drc_index) >> &cdata); >> } >> >> -static ssize_t dlpar_cpu_add(u32 drc_index) >> +static ssize_t dlpar_cpu_add(u32 drc_index, bool acquire_drc) >> { >> struct device_node *dn, *parent; >> int rc, saved_rc; >> @@ -499,19 +499,22 @@ static ssize_t dlpar_cpu_add(u32 drc_index) >> return -EINVAL; >> } >> >> - rc = dlpar_acquire_drc(drc_index); >> - if (rc) { >> - pr_warn("Failed to acquire DRC, rc: %d, drc index: %x\n", >> - rc, drc_index); >> - of_node_put(parent); >> - return -EINVAL; >> + if (acquire_drc) { >> + rc = dlpar_acquire_drc(drc_index); >> + if (rc) { >> + pr_warn("Failed to acquire DRC, rc: %d, drc index: >> %x\n", >> + rc, drc_index); >> + of_node_put(parent); >> + return -EINVAL; >> + } >> } >> >> dn = dlpar_configure_connector(cpu_to_be32(drc_index), parent); >> if (!dn) { >> pr_warn("Failed call to configure-connector, drc index: %x\n", >> drc_index); >> - dlpar_release_drc(drc_index); >> + if (acquire_drc) >> + dlpar_release_drc(drc_index); >> of_node_put(parent); >> return -EINVAL; >> } >> @@ -526,8 +529,9 @@ static ssize_t dlpar_cpu_add(u32 drc_index) >> pr_warn("Failed to attach node %s, rc: %d, drc index: %x\n", >> dn->name, rc, drc_index); >> >> - rc = dlpar_release_drc(drc_index); >> - if (!rc) >> + if (acquire_drc) >> + rc = dlpar_release_drc(drc_index); >> + if (!rc || acquire_drc) >> dlpar_free_cc_nodes(dn); > > This seems like it would be more readable if everything were inside the > if (acquire_drc) block. > >> >> return saved_rc; >> @@ -540,7 +544,7 @@ static ssize_t dlpar_cpu_add(u32 drc_index) >> dn->name, rc, drc_index); >> >> rc = dlpar_detach_node(dn); >> - if (!rc) >> + if (!rc && acquire_drc) >> dlpar_release_drc(drc_index); >> >> return saved_rc; >> @@ -608,12 +612,13 @@ static int dlpar_offline_cpu(struct device_node *dn) >> >> } >> >> -static ssize_t dlpar_cpu_remove(struct device_node *dn, u32 drc_index) >> +static ssize_t dlpar_cpu_remove(struct device_node *dn, u32 drc_index, >> + bool release_drc) >> { >> int rc; >> >> - pr_debug("Attempting to remove CPU %s, drc index: %x\n", >> - dn->name, drc_index); >> + pr_debug("Attempting to remove CPU %s, drc index: %x (%d)\n", >> + dn->name, drc_index, release_drc); >> >> rc = dlpar_offline_cpu(dn); >> if (rc) { >> @@ -621,12 +626,14 @@ static ssize_t dlpar_cpu_remove(struct device_node >> *dn, u32 drc_index) >> return -EINVAL; >> } >> >> - rc = dlpar_release_drc(drc_index); >> - if (rc) { >> - pr_warn("Failed to release drc (%x) for CPU %s, rc: %d\n", >> - drc_index, dn->name, rc); >> - dlpar_online_cpu(dn); >> - return rc; >> + if (release_drc) { >> + rc = dlpar_release_drc(drc_index); >> + if (rc) { >> + pr_warn("Failed to release drc (%x) for CPU %s, rc: >> %d\n", >> + drc_index, dn->name, rc); >> + dlpar_online_cpu(dn); >> + return rc; >> + } >> } >> >> rc = dlpar_detach_node(dn); >> @@ -635,7 +642,8 @@ static ssize_t dlpar_cpu_remove(struct device_node *dn, >> u32 drc_index) >> >> pr_warn("Failed to detach CPU %s, rc: %d", dn->name, rc); >> >> - rc = dlpar_acquire_drc(drc_index); >> + if (release_drc) >> + rc = dlpar_acquire_drc(drc_index); >> if (!rc) >> dlpar_online_cpu(dn); >> >> @@ -664,7 +672,7 @@ static struct device_node *cpu_drc_index_to_dn(u32 >> drc_index) >> return dn; >> } >> >> -static int dlpar_cpu_remove_by_index(u32 drc_index) >> +static int dlpar_cpu_remove_by_index(u32 drc_index, bool release_drc) >> { >> struct device_node *dn; >> int rc; >> @@ -676,10 +684,30 @@ static int dlpar_cpu_remove_by_index(u32 drc_index) >> return -ENODEV; >> } >> >> - rc = dlpar_cpu_remove(dn, drc_index); >> + rc = dlpar_cpu_remove(dn, drc_index, release_drc); >> of_node_put(dn); >> return rc; >> } >> + >> +static int dlpar_cpu_readd_by_index(u32 drc_index) >> +{ >> + int rc = 0; > > No need to initialize to 0 here. > >> + >> + pr_info("Attempting to re-add CPU, drc index %x\n", drc_index); >> + >> + rc = dlpar_cpu_remove_by_index(drc_index, false); >> + if (!rc) >> + rc = dlpar_cpu_add(drc_index, false); >> + >> + if (rc) >> + pr_info("Failed to update cpu at drc_index %lx\n", >> + (unsigned long int)drc_index); >> + else >> + pr_info("CPU at drc_index %lx was updated\n", >> + (unsigned long int)drc_index); >> + >> + return rc; >> +} >> >> static int find_dlpar_cpus_to_remove(u32 *cpu_drcs, int cpus_to_remove) >> { >> @@ -741,7 +769,7 @@ static int dlpar_cpu_remove_by_count(u32 cpus_to_remove) >> } >> >> for (i = 0; i < cpus_to_remove; i++) { >> - rc = dlpar_cpu_remove_by_index(cpu_drcs[i]); >> + rc = dlpar_cpu_remove_by_index(cpu_drcs[i], true); >> if (rc) >> break; >> >> @@ -752,7 +780,7 @@ static int dlpar_cpu_remove_by_count(u32 cpus_to_remove) >> pr_warn("CPU hot-remove failed, adding back removed CPUs\n"); >> >> for (i = 0; i < cpus_removed; i++) >> - dlpar_cpu_add(cpu_drcs[i]); >> + dlpar_cpu_add(cpu_drcs[i], true); >> >> rc = -EINVAL; >> } else { >> @@ -843,7 +871,7 @@ static int dlpar_cpu_add_by_count(u32 cpus_to_add) >> } >> >> for (i = 0; i < cpus_to_add; i++) { >> - rc = dlpar_cpu_add(cpu_drcs[i]); >> + rc = dlpar_cpu_add(cpu_drcs[i], true); >> if (rc) >> break; >> >> @@ -854,7 +882,7 @@ static int dlpar_cpu_add_by_count(u32 cpus_to_add) >> pr_warn("CPU hot-add failed, removing any added CPUs\n"); >> >> for (i = 0; i < cpus_added; i++) >> - dlpar_cpu_remove_by_index(cpu_drcs[i]); >> + dlpar_cpu_remove_by_index(cpu_drcs[i], true); >> >> rc = -EINVAL; >> } else { >> @@ -880,7 +908,7 @@ int dlpar_cpu(struct pseries_hp_errorlog *hp_elog) >> if (hp_elog->id_type == PSERIES_HP_ELOG_ID_DRC_COUNT) >> rc = dlpar_cpu_remove_by_count(count); >> else if (hp_elog->id_type == PSERIES_HP_ELOG_ID_DRC_INDEX) >> - rc = dlpar_cpu_remove_by_index(drc_index); >> + rc = dlpar_cpu_remove_by_index(drc_index, true); >> else >> rc = -EINVAL; >> break; >> @@ -888,10 +916,13 @@ int dlpar_cpu(struct pseries_hp_errorlog *hp_elog) >> if (hp_elog->id_type == PSERIES_HP_ELOG_ID_DRC_COUNT) >> rc = dlpar_cpu_add_by_count(count); >> else if (hp_elog->id_type == PSERIES_HP_ELOG_ID_DRC_INDEX) >> - rc = dlpar_cpu_add(drc_index); >> + rc = dlpar_cpu_add(drc_index, true); >> else >> rc = -EINVAL; >> break; >> + case PSERIES_HP_ELOG_ACTION_READD: >> + rc = dlpar_cpu_readd_by_index(drc_index); >> + break; >> default: >> pr_err("Invalid action (%d) specified\n", hp_elog->action); >> rc = -EINVAL; >> @@ -913,7 +944,7 @@ static ssize_t dlpar_cpu_probe(const char *buf, size_t >> count) >> if (rc) >> return -EINVAL; >> >> - rc = dlpar_cpu_add(drc_index); >> + rc = dlpar_cpu_add(drc_index, true); >> >> return rc ? rc : count; >> } >> @@ -934,7 +965,7 @@ static ssize_t dlpar_cpu_release(const char *buf, size_t >> count) >> return -EINVAL; >> } >> >> - rc = dlpar_cpu_remove(dn, drc_index); >> + rc = dlpar_cpu_remove(dn, drc_index, true); >> of_node_put(dn); >> >> return rc ? rc : count; >> @@ -942,12 +973,38 @@ static ssize_t dlpar_cpu_release(const char *buf, >> size_t count) >> >> #endif /* CONFIG_ARCH_CPU_PROBE_RELEASE */ >> >> +static int pseries_update_cpu(struct of_reconfig_data *pr) >> +{ >> + /* So far, we only handle the 'ibm,associativity' property, >> + * here. >> + */ >> + struct pseries_hp_errorlog *hp_elog; >> + >> + hp_elog = kzalloc(sizeof(*hp_elog), GFP_KERNEL); >> + if(!hp_elog) >> + return -ENOMEM; > > You seem to have some odd indentation here, may want to run this through > checkpatch Okay. I think it happened on one of my build systems which was setting 'ts=4'. > > Also, I think queue_hotplug_event() makes a copy of the hotplug event passed > in. If > so you could just use a hp_elog struct on the stack and avoid the allocation. Okay. > > -Nathan Thanks. Michael > >> + >> + hp_elog->resource = PSERIES_HP_ELOG_RESOURCE_CPU; >> + hp_elog->action = PSERIES_HP_ELOG_ACTION_READD; >> + hp_elog->id_type = PSERIES_HP_ELOG_ID_DRC_INDEX; >> + hp_elog->_drc_u.drc_index = be32_to_cpu(pr->dn->phandle); >> + >> + queue_hotplug_event(hp_elog, NULL, NULL); >> + >> + kfree(hp_elog); >> + >> + return 0; >> +} >> + >> static int pseries_smp_notifier(struct notifier_block *nb, >> unsigned long action, void *data) >> { >> struct of_reconfig_data *rd = data; >> int err = 0; >> >> + if (strcmp(rd->dn->type, "cpu")) >> + return notifier_from_errno(err); >> + >> switch (action) { >> case OF_RECONFIG_ATTACH_NODE: >> err = pseries_add_processor(rd->dn); >> @@ -955,6 +1012,10 @@ static int pseries_smp_notifier(struct notifier_block >> *nb, >> case OF_RECONFIG_DETACH_NODE: >> pseries_remove_processor(rd->dn); >> break; >> + case OF_RECONFIG_UPDATE_PROPERTY: >> + if (!strcmp(rd->prop->name, "ibm,associativity")) >> + err = pseries_update_cpu(rd); >> + break; >> } >> return notifier_from_errno(err); >> } >> diff --git a/arch/powerpc/platforms/pseries/mobility.c >> b/arch/powerpc/platforms/pseries/mobility.c >> index 8a8033a..6d98f84 100644 >> --- a/arch/powerpc/platforms/pseries/mobility.c >> +++ b/arch/powerpc/platforms/pseries/mobility.c >> @@ -283,6 +283,8 @@ int pseries_devicetree_update(s32 scope) >> if (!rtas_buf) >> return -ENOMEM; >> >> + lock_device_hotplug(); >> + >> do { >> rc = mobility_rtas_call(update_nodes_token, rtas_buf, scope); >> if (rc && rc != 1) >> @@ -321,6 +323,7 @@ int pseries_devicetree_update(s32 scope) >> } while (rc == 1); >> >> kfree(rtas_buf); >> + unlock_device_hotplug(); >> return rc; >> } >> > > -- Michael W. Bringmann Linux Technology Center IBM Corporation Tie-Line 363-5196 External: (512) 286-5196 Cell: (512) 466-0650 m...@linux.vnet.ibm.com