<html><head><meta http-equiv="Content-Type" content="text/html; charset=utf-8"></head><body style="word-wrap: break-word; -webkit-nbsp-mode: space; line-break: after-white-space;" class=""><br class=""><div><br class=""><blockquote type="cite" class=""><div class="">On 29-Oct-2021, at 6:45 PM, Michael Ellerman <<a href="mailto:mpe@ellerman.id.au" class="">mpe@ellerman.id.au</a>> wrote:</div><br class="Apple-interchange-newline"><div class=""><meta charset="UTF-8" class=""><span style="caret-color: rgb(0, 0, 0); font-family: Helvetica; font-size: 12px; font-style: normal; font-variant-caps: normal; font-weight: normal; letter-spacing: normal; text-align: start; text-indent: 0px; text-transform: none; white-space: normal; word-spacing: 0px; -webkit-text-stroke-width: 0px; text-decoration: none; float: none; display: inline !important;" class="">Nicholas Piggin <</span><a href="mailto:npiggin@gmail.com" style="font-family: Helvetica; font-size: 12px; font-style: normal; font-variant-caps: normal; font-weight: normal; letter-spacing: normal; orphans: auto; text-align: start; text-indent: 0px; text-transform: none; white-space: normal; widows: auto; word-spacing: 0px; -webkit-text-size-adjust: auto; -webkit-text-stroke-width: 0px;" class="">npiggin@gmail.com</a><span style="caret-color: rgb(0, 0, 0); font-family: Helvetica; font-size: 12px; font-style: normal; font-variant-caps: normal; font-weight: normal; letter-spacing: normal; text-align: start; text-indent: 0px; text-transform: none; white-space: normal; word-spacing: 0px; -webkit-text-stroke-width: 0px; text-decoration: none; float: none; display: inline !important;" class="">> writes:</span><br style="caret-color: rgb(0, 0, 0); font-family: Helvetica; font-size: 12px; font-style: normal; font-variant-caps: normal; font-weight: normal; letter-spacing: normal; text-align: start; text-indent: 0px; text-transform: none; white-space: normal; word-spacing: 0px; -webkit-text-stroke-width: 0px; text-decoration: none;" class=""><blockquote type="cite" style="font-family: Helvetica; font-size: 12px; font-style: normal; font-variant-caps: normal; font-weight: normal; letter-spacing: normal; orphans: auto; text-align: start; text-indent: 0px; text-transform: none; white-space: normal; widows: auto; word-spacing: 0px; -webkit-text-size-adjust: auto; -webkit-text-stroke-width: 0px; text-decoration: none;" class="">Excerpts from Athira Rajeev's message of October 29, 2021 1:05 pm:<br class=""><blockquote type="cite" class="">During Live Partition Migration (LPM), it is observed that perf<br class="">counter values reports zero post migration completion. However<br class="">'perf stat' with workload continues to show counts post migration<br class="">since PMU gets disabled/enabled during sched switches. But incase<br class="">of system/cpu wide monitoring, zero counts were reported with 'perf<br class="">stat' after migration completion.<br class=""><br class="">Example:<br class="">./perf stat -e r1001e -I 1000<br class=""> time counts unit events<br class=""> 1.001010437 22,137,414 r1001e<br class=""> 2.002495447 15,455,821 r1001e<br class=""><<>> As seen in next below logs, the counter values shows zero<br class=""> after migration is completed.<br class=""><<>><br class=""> 86.142535370 129,392,333,440 r1001e<br class=""> 87.144714617 0 r1001e<br class=""> 88.146526636 0 r1001e<br class=""> 89.148085029 0 r1001e<br class=""></blockquote><br class="">This is the output without the patch? After the patch it keeps counting<span class="Apple-converted-space"> </span><br class="">I suppose? And does the very large count go away too?<br class=""><br class=""><blockquote type="cite" class=""><br class="">Here PMU is enabled during start of perf session and counter<br class="">values are read at intervals. Counters are only disabled at the<br class="">end of session. The powerpc mobility code presently does not handle<br class="">disabling and enabling back of PMU counters during partition<br class="">migration. Also since the PMU register values are not saved/restored<br class="">during migration, PMU registers like Monitor Mode Control Register 0<br class="">(MMCR0), Monitor Mode Control Register 1 (MMCR1) will not contain<br class="">the value it was programmed with. Hence PMU counters will not be<br class="">enabled correctly post migration.<br class=""><br class="">Fix this in mobility code by handling disabling and enabling of<br class="">PMU in all cpu's before and after migration. Patch introduces two<br class="">functions 'mobility_pmu_disable' and 'mobility_pmu_enable'.<br class="">mobility_pmu_disable() is called before the processor threads goes<br class="">to suspend state so as to disable the PMU counters. And disable is<br class="">done only if there are any active events running on that cpu.<br class="">mobility_pmu_enable() is called after the migrate is done to enable<br class="">back the PMU counters.<br class=""><br class="">Since the performance Monitor counters ( PMCs) are not<br class="">saved/restored during LPM, results in PMC value being zero and the<br class="">'event->hw.prev_count' being non-zero value. This causes problem<br class="">during updation of event->count since we always accumulate<br class="">(event->hw.prev_count - PMC value) in event->count. If<br class="">event->hw.prev_count is greater PMC value, event->count becomes<br class="">negative. To fix this, 'prev_count' also needs to be re-initialised<br class="">for all events while enabling back the events. Hence read the<br class="">existing events and clear the PMC index (stored in event->hw.idx)<br class="">for all events im mobility_pmu_disable. By this way, event count<br class="">settings will get re-initialised correctly in power_pmu_enable.<br class=""><br class="">Signed-off-by: Athira Rajeev <<a href="mailto:atrajeev@linux.vnet.ibm.com" class="">atrajeev@linux.vnet.ibm.com</a>><br class="">[ Fixed compilation error reported by kernel test robot ]<br class="">Reported-by: kernel test robot <<a href="mailto:lkp@intel.com" class="">lkp@intel.com</a>><br class="">---<br class="">Changelog:<br class="">Change from v2 -> v3:<br class="">Addressed review comments from Nicholas Piggin.<br class="">- Removed the "migrate" field which was added in initial<br class=""> patch to address updation of event count settings correctly<br class=""> in power_pmu_enable. Instead read off existing events in<br class=""> mobility_pmu_disable before power_pmu_enable.<br class="">- Moved the mobility_pmu_disable/enable declaration from<br class=""> rtas.h to perf event header file.<br class=""><br class="">Addressed review comments from Nathan.<br class="">- Moved the mobility function calls from stop_machine<br class=""> context out to pseries_migrate_partition. Also now this<br class=""> is a per cpu invocation.<br class=""><br class="">Change from v1 -> v2:<br class="">- Moved the mobility_pmu_enable and mobility_pmu_disable<br class=""> declarations under CONFIG_PPC_PERF_CTRS in rtas.h.<br class=""> Also included 'asm/rtas.h' in core-book3s to fix the<br class=""> compilation warning reported by kernel test robot.<br class=""><br class="">arch/powerpc/include/asm/perf_event.h | 8 +++++<br class="">arch/powerpc/perf/core-book3s.c | 39 +++++++++++++++++++++++<br class="">arch/powerpc/platforms/pseries/mobility.c | 7 ++++<br class="">3 files changed, 54 insertions(+)<br class=""><br class="">diff --git a/arch/powerpc/include/asm/perf_event.h b/arch/powerpc/include/asm/perf_event.h<br class="">index 164e910bf654..88aab6cf840c 100644<br class="">--- a/arch/powerpc/include/asm/perf_event.h<br class="">+++ b/arch/powerpc/include/asm/perf_event.h<br class="">@@ -17,6 +17,14 @@ static inline bool is_sier_available(void) { return false; }<br class="">static inline unsigned long get_pmcs_ext_regs(int idx) { return 0; }<br class="">#endif<br class=""><br class="">+#ifdef CONFIG_PPC_PERF_CTRS<br class="">+void mobility_pmu_disable(void *unused);<br class="">+void mobility_pmu_enable(void *unused);<br class="">+#else<br class="">+static inline void mobility_pmu_disable(void *unused) { }<br class="">+static inline void mobility_pmu_enable(void *unused) { }<br class="">+#endif<br class="">+<br class="">#ifdef CONFIG_FSL_EMB_PERF_EVENT<br class="">#include <asm/perf_event_fsl_emb.h><br class="">#endif<br class="">diff --git a/arch/powerpc/perf/core-book3s.c b/arch/powerpc/perf/core-book3s.c<br class="">index 73e62e9b179b..2e8c4c668fa3 100644<br class="">--- a/arch/powerpc/perf/core-book3s.c<br class="">+++ b/arch/powerpc/perf/core-book3s.c<br class="">@@ -1343,6 +1343,33 @@ static void power_pmu_disable(struct pmu *pmu)<br class=""><span class="Apple-tab-span" style="white-space: pre;"> </span>local_irq_restore(flags);<br class="">}<br class=""><br class="">+/*<br class="">+ * Called from pseries_migrate_partition() function<br class="">+ * before migration, from powerpc/mobility code.<br class="">+ */<br class=""></blockquote></blockquote><br style="caret-color: rgb(0, 0, 0); font-family: Helvetica; font-size: 12px; font-style: normal; font-variant-caps: normal; font-weight: normal; letter-spacing: normal; text-align: start; text-indent: 0px; text-transform: none; white-space: normal; word-spacing: 0px; -webkit-text-stroke-width: 0px; text-decoration: none;" class=""><span style="caret-color: rgb(0, 0, 0); font-family: Helvetica; font-size: 12px; font-style: normal; font-variant-caps: normal; font-weight: normal; letter-spacing: normal; text-align: start; text-indent: 0px; text-transform: none; white-space: normal; word-spacing: 0px; -webkit-text-stroke-width: 0px; text-decoration: none; float: none; display: inline !important;" class="">These are only needed if pseries is built, so should be inside a PSERIES</span><br style="caret-color: rgb(0, 0, 0); font-family: Helvetica; font-size: 12px; font-style: normal; font-variant-caps: normal; font-weight: normal; letter-spacing: normal; text-align: start; text-indent: 0px; text-transform: none; white-space: normal; word-spacing: 0px; -webkit-text-stroke-width: 0px; text-decoration: none;" class=""><span style="caret-color: rgb(0, 0, 0); font-family: Helvetica; font-size: 12px; font-style: normal; font-variant-caps: normal; font-weight: normal; letter-spacing: normal; text-align: start; text-indent: 0px; text-transform: none; white-space: normal; word-spacing: 0px; -webkit-text-stroke-width: 0px; text-decoration: none; float: none; display: inline !important;" class="">ifdef.</span><br style="caret-color: rgb(0, 0, 0); font-family: Helvetica; font-size: 12px; font-style: normal; font-variant-caps: normal; font-weight: normal; letter-spacing: normal; text-align: start; text-indent: 0px; text-transform: none; white-space: normal; word-spacing: 0px; -webkit-text-stroke-width: 0px; text-decoration: none;" class=""></div></blockquote><div><br class=""></div>Sure mpe, will address this change in next version<br class=""><blockquote type="cite" class=""><div class=""><br style="caret-color: rgb(0, 0, 0); font-family: Helvetica; font-size: 12px; font-style: normal; font-variant-caps: normal; font-weight: normal; letter-spacing: normal; text-align: start; text-indent: 0px; text-transform: none; white-space: normal; word-spacing: 0px; -webkit-text-stroke-width: 0px; text-decoration: none;" class=""><span style="caret-color: rgb(0, 0, 0); font-family: Helvetica; font-size: 12px; font-style: normal; font-variant-caps: normal; font-weight: normal; letter-spacing: normal; text-align: start; text-indent: 0px; text-transform: none; white-space: normal; word-spacing: 0px; -webkit-text-stroke-width: 0px; text-decoration: none; float: none; display: inline !important;" class="">This function should handle iterating over CPUs, that shouldn't be left</span><br style="caret-color: rgb(0, 0, 0); font-family: Helvetica; font-size: 12px; font-style: normal; font-variant-caps: normal; font-weight: normal; letter-spacing: normal; text-align: start; text-indent: 0px; text-transform: none; white-space: normal; word-spacing: 0px; -webkit-text-stroke-width: 0px; text-decoration: none;" class=""><span style="caret-color: rgb(0, 0, 0); font-family: Helvetica; font-size: 12px; font-style: normal; font-variant-caps: normal; font-weight: normal; letter-spacing: normal; text-align: start; text-indent: 0px; text-transform: none; white-space: normal; word-spacing: 0px; -webkit-text-stroke-width: 0px; text-decoration: none; float: none; display: inline !important;" class="">up to the mobility.c code.</span><br style="caret-color: rgb(0, 0, 0); font-family: Helvetica; font-size: 12px; font-style: normal; font-variant-caps: normal; font-weight: normal; letter-spacing: normal; text-align: start; text-indent: 0px; text-transform: none; white-space: normal; word-spacing: 0px; -webkit-text-stroke-width: 0px; text-decoration: none;" class=""><br style="caret-color: rgb(0, 0, 0); font-family: Helvetica; font-size: 12px; font-style: normal; font-variant-caps: normal; font-weight: normal; letter-spacing: normal; text-align: start; text-indent: 0px; text-transform: none; white-space: normal; word-spacing: 0px; -webkit-text-stroke-width: 0px; text-decoration: none;" class=""><span style="caret-color: rgb(0, 0, 0); font-family: Helvetica; font-size: 12px; font-style: normal; font-variant-caps: normal; font-weight: normal; letter-spacing: normal; text-align: start; text-indent: 0px; text-transform: none; white-space: normal; word-spacing: 0px; -webkit-text-stroke-width: 0px; text-decoration: none; float: none; display: inline !important;" class="">And the names should be something like pmu_start_migration(),</span><br style="caret-color: rgb(0, 0, 0); font-family: Helvetica; font-size: 12px; font-style: normal; font-variant-caps: normal; font-weight: normal; letter-spacing: normal; text-align: start; text-indent: 0px; text-transform: none; white-space: normal; word-spacing: 0px; -webkit-text-stroke-width: 0px; text-decoration: none;" class=""><span style="caret-color: rgb(0, 0, 0); font-family: Helvetica; font-size: 12px; font-style: normal; font-variant-caps: normal; font-weight: normal; letter-spacing: normal; text-align: start; text-indent: 0px; text-transform: none; white-space: normal; word-spacing: 0px; -webkit-text-stroke-width: 0px; text-decoration: none; float: none; display: inline !important;" class="">pmu_finish_migration().</span><br style="caret-color: rgb(0, 0, 0); font-family: Helvetica; font-size: 12px; font-style: normal; font-variant-caps: normal; font-weight: normal; letter-spacing: normal; text-align: start; text-indent: 0px; text-transform: none; white-space: normal; word-spacing: 0px; -webkit-text-stroke-width: 0px; text-decoration: none;" class=""></div></blockquote><div><br class=""></div>Ok, will change<br class=""><blockquote type="cite" class=""><div class=""><br style="caret-color: rgb(0, 0, 0); font-family: Helvetica; font-size: 12px; font-style: normal; font-variant-caps: normal; font-weight: normal; letter-spacing: normal; text-align: start; text-indent: 0px; text-transform: none; white-space: normal; word-spacing: 0px; -webkit-text-stroke-width: 0px; text-decoration: none;" class=""><blockquote type="cite" style="font-family: Helvetica; font-size: 12px; font-style: normal; font-variant-caps: normal; font-weight: normal; letter-spacing: normal; orphans: auto; text-align: start; text-indent: 0px; text-transform: none; white-space: normal; widows: auto; word-spacing: 0px; -webkit-text-size-adjust: auto; -webkit-text-stroke-width: 0px; text-decoration: none;" class=""><blockquote type="cite" class="">+void mobility_pmu_disable(void *unused)<br class="">+{<br class="">+<span class="Apple-tab-span" style="white-space: pre;"> </span>struct cpu_hw_events *cpuhw = this_cpu_ptr(&cpu_hw_events);<br class="">+<span class="Apple-tab-span" style="white-space: pre;"> </span>struct perf_event *event;<br class="">+<br class="">+<span class="Apple-tab-span" style="white-space: pre;"> </span>if (cpuhw->n_events != 0) {<br class="">+<span class="Apple-tab-span" style="white-space: pre;"> </span><span class="Apple-tab-span" style="white-space: pre;"> </span>int i;<br class="">+<br class="">+<span class="Apple-tab-span" style="white-space: pre;"> </span><span class="Apple-tab-span" style="white-space: pre;"> </span>power_pmu_disable(NULL);<br class="">+<span class="Apple-tab-span" style="white-space: pre;"> </span><span class="Apple-tab-span" style="white-space: pre;"> </span>/*<br class="">+<span class="Apple-tab-span" style="white-space: pre;"> </span><span class="Apple-tab-span" style="white-space: pre;"> </span><span class="Apple-converted-space"> </span>* Read off any pre-existing events because the register<br class="">+<span class="Apple-tab-span" style="white-space: pre;"> </span><span class="Apple-tab-span" style="white-space: pre;"> </span><span class="Apple-converted-space"> </span>* values may not be migrated.<br class="">+<span class="Apple-tab-span" style="white-space: pre;"> </span><span class="Apple-tab-span" style="white-space: pre;"> </span><span class="Apple-converted-space"> </span>*/<br class="">+<span class="Apple-tab-span" style="white-space: pre;"> </span><span class="Apple-tab-span" style="white-space: pre;"> </span>for (i = 0; i < cpuhw->n_events; ++i) {<br class="">+<span class="Apple-tab-span" style="white-space: pre;"> </span><span class="Apple-tab-span" style="white-space: pre;"> </span><span class="Apple-tab-span" style="white-space: pre;"> </span>event = cpuhw->event[i];<br class="">+<span class="Apple-tab-span" style="white-space: pre;"> </span><span class="Apple-tab-span" style="white-space: pre;"> </span><span class="Apple-tab-span" style="white-space: pre;"> </span>if (event->hw.idx) {<br class="">+<span class="Apple-tab-span" style="white-space: pre;"> </span><span class="Apple-tab-span" style="white-space: pre;"> </span><span class="Apple-tab-span" style="white-space: pre;"> </span><span class="Apple-tab-span" style="white-space: pre;"> </span>power_pmu_read(event);<br class="">+<span class="Apple-tab-span" style="white-space: pre;"> </span><span class="Apple-tab-span" style="white-space: pre;"> </span><span class="Apple-tab-span" style="white-space: pre;"> </span><span class="Apple-tab-span" style="white-space: pre;"> </span>event->hw.idx = 0;<br class="">+<span class="Apple-tab-span" style="white-space: pre;"> </span><span class="Apple-tab-span" style="white-space: pre;"> </span><span class="Apple-tab-span" style="white-space: pre;"> </span>}<br class="">+<span class="Apple-tab-span" style="white-space: pre;"> </span><span class="Apple-tab-span" style="white-space: pre;"> </span>}<br class="">+<span class="Apple-tab-span" style="white-space: pre;"> </span>}<br class="">+}<br class="">+<br class="">/*<br class=""> * Re-enable all events if disable == 0.<br class=""> * If we were previously disabled and events were added, then<br class="">@@ -1515,6 +1542,18 @@ static void power_pmu_enable(struct pmu *pmu)<br class=""><span class="Apple-tab-span" style="white-space: pre;"> </span>local_irq_restore(flags);<br class="">}<br class=""><br class="">+/*<br class="">+ * Called from pseries_migrate_partition() function<br class="">+ * after migration, from powerpc/mobility code.<br class="">+ */<br class="">+void mobility_pmu_enable(void *unused)<br class="">+{<br class="">+<span class="Apple-tab-span" style="white-space: pre;"> </span>struct cpu_hw_events *cpuhw = this_cpu_ptr(&cpu_hw_events);<br class="">+<br class="">+<span class="Apple-tab-span" style="white-space: pre;"> </span>cpuhw->n_added = cpuhw->n_events;<br class="">+<span class="Apple-tab-span" style="white-space: pre;"> </span>power_pmu_enable(NULL);<br class="">+}<br class="">+<br class="">static int collect_events(struct perf_event *group, int max_count,<br class=""><span class="Apple-tab-span" style="white-space: pre;"> </span><span class="Apple-tab-span" style="white-space: pre;"> </span><span class="Apple-tab-span" style="white-space: pre;"> </span><span class="Apple-converted-space"> </span> struct perf_event *ctrs[], u64 *events,<br class=""><span class="Apple-tab-span" style="white-space: pre;"> </span><span class="Apple-tab-span" style="white-space: pre;"> </span><span class="Apple-tab-span" style="white-space: pre;"> </span><span class="Apple-converted-space"> </span> unsigned int *flags)<br class="">diff --git a/arch/powerpc/platforms/pseries/mobility.c b/arch/powerpc/platforms/pseries/mobility.c<br class="">index e83e0891272d..3e96485ccba4 100644<br class="">--- a/arch/powerpc/platforms/pseries/mobility.c<br class="">+++ b/arch/powerpc/platforms/pseries/mobility.c<br class="">@@ -22,6 +22,7 @@<br class="">#include <linux/delay.h><br class="">#include <linux/slab.h><br class="">#include <linux/stringify.h><br class="">+#include <linux/perf_event.h><br class=""><br class="">#include <asm/machdep.h><br class="">#include <asm/rtas.h><br class="">@@ -631,12 +632,18 @@ static int pseries_migrate_partition(u64 handle)<br class=""><span class="Apple-tab-span" style="white-space: pre;"> </span>if (ret)<br class=""><span class="Apple-tab-span" style="white-space: pre;"> </span><span class="Apple-tab-span" style="white-space: pre;"> </span>return ret;<br class=""><br class="">+<span class="Apple-tab-span" style="white-space: pre;"> </span>/* Disable PMU before suspend */<br class="">+<span class="Apple-tab-span" style="white-space: pre;"> </span>on_each_cpu(&mobility_pmu_disable, NULL, 0);<br class=""></blockquote><br class="">Why was this moved out of stop machine and to an IPI?<br class=""><br class="">My concern would be, what are the other CPUs doing at this time? Is it<span class="Apple-converted-space"> </span><br class="">possible they could take interrupts and schedule? Could that mess up the<br class="">perf state here?<br class=""></blockquote><br style="caret-color: rgb(0, 0, 0); font-family: Helvetica; font-size: 12px; font-style: normal; font-variant-caps: normal; font-weight: normal; letter-spacing: normal; text-align: start; text-indent: 0px; text-transform: none; white-space: normal; word-spacing: 0px; -webkit-text-stroke-width: 0px; text-decoration: none;" class=""><span style="caret-color: rgb(0, 0, 0); font-family: Helvetica; font-size: 12px; font-style: normal; font-variant-caps: normal; font-weight: normal; letter-spacing: normal; text-align: start; text-indent: 0px; text-transform: none; white-space: normal; word-spacing: 0px; -webkit-text-stroke-width: 0px; text-decoration: none; float: none; display: inline !important;" class="">pseries_migrate_partition() is called directly from migration_store(),</span><br style="caret-color: rgb(0, 0, 0); font-family: Helvetica; font-size: 12px; font-style: normal; font-variant-caps: normal; font-weight: normal; letter-spacing: normal; text-align: start; text-indent: 0px; text-transform: none; white-space: normal; word-spacing: 0px; -webkit-text-stroke-width: 0px; text-decoration: none;" class=""><span style="caret-color: rgb(0, 0, 0); font-family: Helvetica; font-size: 12px; font-style: normal; font-variant-caps: normal; font-weight: normal; letter-spacing: normal; text-align: start; text-indent: 0px; text-transform: none; white-space: normal; word-spacing: 0px; -webkit-text-stroke-width: 0px; text-decoration: none; float: none; display: inline !important;" class="">which is the sysfs store function, which can be called concurrently by</span><br style="caret-color: rgb(0, 0, 0); font-family: Helvetica; font-size: 12px; font-style: normal; font-variant-caps: normal; font-weight: normal; letter-spacing: normal; text-align: start; text-indent: 0px; text-transform: none; white-space: normal; word-spacing: 0px; -webkit-text-stroke-width: 0px; text-decoration: none;" class=""><span style="caret-color: rgb(0, 0, 0); font-family: Helvetica; font-size: 12px; font-style: normal; font-variant-caps: normal; font-weight: normal; letter-spacing: normal; text-align: start; text-indent: 0px; text-transform: none; white-space: normal; word-spacing: 0px; -webkit-text-stroke-width: 0px; text-decoration: none; float: none; display: inline !important;" class="">different CPUs.</span><br style="caret-color: rgb(0, 0, 0); font-family: Helvetica; font-size: 12px; font-style: normal; font-variant-caps: normal; font-weight: normal; letter-spacing: normal; text-align: start; text-indent: 0px; text-transform: none; white-space: normal; word-spacing: 0px; -webkit-text-stroke-width: 0px; text-decoration: none;" class=""><br style="caret-color: rgb(0, 0, 0); font-family: Helvetica; font-size: 12px; font-style: normal; font-variant-caps: normal; font-weight: normal; letter-spacing: normal; text-align: start; text-indent: 0px; text-transform: none; white-space: normal; word-spacing: 0px; -webkit-text-stroke-width: 0px; text-decoration: none;" class=""><span style="caret-color: rgb(0, 0, 0); font-family: Helvetica; font-size: 12px; font-style: normal; font-variant-caps: normal; font-weight: normal; letter-spacing: normal; text-align: start; text-indent: 0px; text-transform: none; white-space: normal; word-spacing: 0px; -webkit-text-stroke-width: 0px; text-decoration: none; float: none; display: inline !important;" class="">It's also potentially called from rtas_syscall_dispatch_ibm_suspend_me(),</span><br style="caret-color: rgb(0, 0, 0); font-family: Helvetica; font-size: 12px; font-style: normal; font-variant-caps: normal; font-weight: normal; letter-spacing: normal; text-align: start; text-indent: 0px; text-transform: none; white-space: normal; word-spacing: 0px; -webkit-text-stroke-width: 0px; text-decoration: none;" class=""><span style="caret-color: rgb(0, 0, 0); font-family: Helvetica; font-size: 12px; font-style: normal; font-variant-caps: normal; font-weight: normal; letter-spacing: normal; text-align: start; text-indent: 0px; text-transform: none; white-space: normal; word-spacing: 0px; -webkit-text-stroke-width: 0px; text-decoration: none; float: none; display: inline !important;" class="">from sys_rtas(), again with no locking.</span><br style="caret-color: rgb(0, 0, 0); font-family: Helvetica; font-size: 12px; font-style: normal; font-variant-caps: normal; font-weight: normal; letter-spacing: normal; text-align: start; text-indent: 0px; text-transform: none; white-space: normal; word-spacing: 0px; -webkit-text-stroke-width: 0px; text-decoration: none;" class=""><br style="caret-color: rgb(0, 0, 0); font-family: Helvetica; font-size: 12px; font-style: normal; font-variant-caps: normal; font-weight: normal; letter-spacing: normal; text-align: start; text-indent: 0px; text-transform: none; white-space: normal; word-spacing: 0px; -webkit-text-stroke-width: 0px; text-decoration: none;" class=""><span style="caret-color: rgb(0, 0, 0); font-family: Helvetica; font-size: 12px; font-style: normal; font-variant-caps: normal; font-weight: normal; letter-spacing: normal; text-align: start; text-indent: 0px; text-transform: none; white-space: normal; word-spacing: 0px; -webkit-text-stroke-width: 0px; text-decoration: none; float: none; display: inline !important;" class="">So we could have two CPUs calling into here at the same time, which</span><br style="caret-color: rgb(0, 0, 0); font-family: Helvetica; font-size: 12px; font-style: normal; font-variant-caps: normal; font-weight: normal; letter-spacing: normal; text-align: start; text-indent: 0px; text-transform: none; white-space: normal; word-spacing: 0px; -webkit-text-stroke-width: 0px; text-decoration: none;" class=""><span style="caret-color: rgb(0, 0, 0); font-family: Helvetica; font-size: 12px; font-style: normal; font-variant-caps: normal; font-weight: normal; letter-spacing: normal; text-align: start; text-indent: 0px; text-transform: none; white-space: normal; word-spacing: 0px; -webkit-text-stroke-width: 0px; text-decoration: none; float: none; display: inline !important;" class="">might not crash, but is unlikely to work well.</span><br style="caret-color: rgb(0, 0, 0); font-family: Helvetica; font-size: 12px; font-style: normal; font-variant-caps: normal; font-weight: normal; letter-spacing: normal; text-align: start; text-indent: 0px; text-transform: none; white-space: normal; word-spacing: 0px; -webkit-text-stroke-width: 0px; text-decoration: none;" class=""><br style="caret-color: rgb(0, 0, 0); font-family: Helvetica; font-size: 12px; font-style: normal; font-variant-caps: normal; font-weight: normal; letter-spacing: normal; text-align: start; text-indent: 0px; text-transform: none; white-space: normal; word-spacing: 0px; -webkit-text-stroke-width: 0px; text-decoration: none;" class=""><span style="caret-color: rgb(0, 0, 0); font-family: Helvetica; font-size: 12px; font-style: normal; font-variant-caps: normal; font-weight: normal; letter-spacing: normal; text-align: start; text-indent: 0px; text-transform: none; white-space: normal; word-spacing: 0px; -webkit-text-stroke-width: 0px; text-decoration: none; float: none; display: inline !important;" class="">I think the lack of locking might have been OK in the past because only</span><br style="caret-color: rgb(0, 0, 0); font-family: Helvetica; font-size: 12px; font-style: normal; font-variant-caps: normal; font-weight: normal; letter-spacing: normal; text-align: start; text-indent: 0px; text-transform: none; white-space: normal; word-spacing: 0px; -webkit-text-stroke-width: 0px; text-decoration: none;" class=""><span style="caret-color: rgb(0, 0, 0); font-family: Helvetica; font-size: 12px; font-style: normal; font-variant-caps: normal; font-weight: normal; letter-spacing: normal; text-align: start; text-indent: 0px; text-transform: none; white-space: normal; word-spacing: 0px; -webkit-text-stroke-width: 0px; text-decoration: none; float: none; display: inline !important;" class="">one CPU will successfully get the other CPUs to call do_join() in</span><br style="caret-color: rgb(0, 0, 0); font-family: Helvetica; font-size: 12px; font-style: normal; font-variant-caps: normal; font-weight: normal; letter-spacing: normal; text-align: start; text-indent: 0px; text-transform: none; white-space: normal; word-spacing: 0px; -webkit-text-stroke-width: 0px; text-decoration: none;" class=""><span style="caret-color: rgb(0, 0, 0); font-family: Helvetica; font-size: 12px; font-style: normal; font-variant-caps: normal; font-weight: normal; letter-spacing: normal; text-align: start; text-indent: 0px; text-transform: none; white-space: normal; word-spacing: 0px; -webkit-text-stroke-width: 0px; text-decoration: none; float: none; display: inline !important;" class="">pseries_suspend(). But I could be wrong.</span><br style="caret-color: rgb(0, 0, 0); font-family: Helvetica; font-size: 12px; font-style: normal; font-variant-caps: normal; font-weight: normal; letter-spacing: normal; text-align: start; text-indent: 0px; text-transform: none; white-space: normal; word-spacing: 0px; -webkit-text-stroke-width: 0px; text-decoration: none;" class=""><br style="caret-color: rgb(0, 0, 0); font-family: Helvetica; font-size: 12px; font-style: normal; font-variant-caps: normal; font-weight: normal; letter-spacing: normal; text-align: start; text-indent: 0px; text-transform: none; white-space: normal; word-spacing: 0px; -webkit-text-stroke-width: 0px; text-decoration: none;" class=""><span style="caret-color: rgb(0, 0, 0); font-family: Helvetica; font-size: 12px; font-style: normal; font-variant-caps: normal; font-weight: normal; letter-spacing: normal; text-align: start; text-indent: 0px; text-transform: none; white-space: normal; word-spacing: 0px; -webkit-text-stroke-width: 0px; text-decoration: none; float: none; display: inline !important;" class="">Anyway, now that we're mutating the PMU state before suspending we need</span><br style="caret-color: rgb(0, 0, 0); font-family: Helvetica; font-size: 12px; font-style: normal; font-variant-caps: normal; font-weight: normal; letter-spacing: normal; text-align: start; text-indent: 0px; text-transform: none; white-space: normal; word-spacing: 0px; -webkit-text-stroke-width: 0px; text-decoration: none;" class=""><span style="caret-color: rgb(0, 0, 0); font-family: Helvetica; font-size: 12px; font-style: normal; font-variant-caps: normal; font-weight: normal; letter-spacing: normal; text-align: start; text-indent: 0px; text-transform: none; white-space: normal; word-spacing: 0px; -webkit-text-stroke-width: 0px; text-decoration: none; float: none; display: inline !important;" class="">to be more careful. So I think we need a lock around the whole sequence.</span><br style="caret-color: rgb(0, 0, 0); font-family: Helvetica; font-size: 12px; font-style: normal; font-variant-caps: normal; font-weight: normal; letter-spacing: normal; text-align: start; text-indent: 0px; text-transform: none; white-space: normal; word-spacing: 0px; -webkit-text-stroke-width: 0px; text-decoration: none;" class=""></div></blockquote><div><br class=""></div><div>Thanks for the review comments.</div><div><br class=""></div><div>The PMU callback invocations were from inside stop machine ( inside “do_join” ) in initial version.</div><div>Moved these out to pseries_migrate_partition as an IPI so as to minimise calls to other sub-system from “do_join” context, as pointed by Nathan.</div><div>But if it is not safe to have per-cpu invocation from pseries_migrate_partition, should we handle this under the interrupt disable path in do_join itself ( as in the initial version ) ? </div><div><br class=""></div><div>Please share your thoughts/suggestions.</div><div><br class=""></div><div>Thanks</div><div>Athira</div><br class=""><blockquote type="cite" class=""><div class=""><br style="caret-color: rgb(0, 0, 0); font-family: Helvetica; font-size: 12px; font-style: normal; font-variant-caps: normal; font-weight: normal; letter-spacing: normal; text-align: start; text-indent: 0px; text-transform: none; white-space: normal; word-spacing: 0px; -webkit-text-stroke-width: 0px; text-decoration: none;" class=""><span style="caret-color: rgb(0, 0, 0); font-family: Helvetica; font-size: 12px; font-style: normal; font-variant-caps: normal; font-weight: normal; letter-spacing: normal; text-align: start; text-indent: 0px; text-transform: none; white-space: normal; word-spacing: 0px; -webkit-text-stroke-width: 0px; text-decoration: none; float: none; display: inline !important;" class="">cheers</span></div></blockquote></div><br class=""></body></html>