| Message ID | 20200221061022.3047-2-kjain@linux.ibm.com (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | powerpc/perf: Add json file metric support for the hv_24x7 socket/chip level events | expand |
| Context | Check | Description |
|---|---|---|
| snowpatch_ozlabs/apply_patch | success | Successfully applied on branch powerpc/merge (65b2623f395a4e25ab3ff4cff1c9c7623619a22d) |
| snowpatch_ozlabs/checkpatch | warning | total: 0 errors, 1 warnings, 0 checks, 8 lines checked |
| snowpatch_ozlabs/needsstable | success | Patch has no Fixes tags |
Kajol Jain [kjain@linux.ibm.com] wrote: > Commit 2b206ee6b0df ("powerpc/perf/hv-24x7: Display change in counter > values")' added to print _change_ in the counter value rather then raw > value for 24x7 counters. Incase of transactions, the event count > is set to 0 at the beginning of the transaction. It also sets > the event's prev_count to the raw value at the time of initialization. > Because of setting event count to 0, we are seeing some weird behaviour, > whenever we run multiple 24x7 events at a time. Interesting. Are we taking delta of a delta and ending up with large negative values in the -I case? However... <snip> > > Signed-off-by: Kajol Jain <kjain@linux.ibm.com> > --- > arch/powerpc/perf/hv-24x7.c | 2 +- > 1 file changed, 1 insertion(+), 1 deletion(-) > > diff --git a/arch/powerpc/perf/hv-24x7.c b/arch/powerpc/perf/hv-24x7.c > index 573e0b309c0c..6dbbf70232aa 100644 > --- a/arch/powerpc/perf/hv-24x7.c > +++ b/arch/powerpc/perf/hv-24x7.c > @@ -1409,7 +1409,7 @@ static void h_24x7_event_read(struct perf_event *event) > * that would require issuing a hcall, which would then > * defeat the purpose of using the txn interface. > */ > - local64_set(&event->count, 0); > + local64_add(0, &event->count); ... not sure, how adding zero to the count helps. Should we just remove the line (and the comment block above it)? Or does it help to clear the event count in ->start_txn() rather than on read()? How does the change impact the counts when run without the -I? Thanks for chasing this down. Sukadev
On 2/23/20 8:21 AM, Sukadev Bhattiprolu wrote: > Kajol Jain [kjain@linux.ibm.com] wrote: >> Commit 2b206ee6b0df ("powerpc/perf/hv-24x7: Display change in counter >> values")' added to print _change_ in the counter value rather then raw >> value for 24x7 counters. Incase of transactions, the event count >> is set to 0 at the beginning of the transaction. It also sets >> the event's prev_count to the raw value at the time of initialization. >> Because of setting event count to 0, we are seeing some weird behaviour, >> whenever we run multiple 24x7 events at a time. > > Interesting. Are we taking delta of a delta and ending up with large > negative values in the -I case? However... > Hi Sukadev, That's right, we are ending up in calculating delta of delta which may give us negative values because of which we are getting these large values in -I case. > <snip> > >> >> Signed-off-by: Kajol Jain <kjain@linux.ibm.com> >> --- >> arch/powerpc/perf/hv-24x7.c | 2 +- >> 1 file changed, 1 insertion(+), 1 deletion(-) >> >> diff --git a/arch/powerpc/perf/hv-24x7.c b/arch/powerpc/perf/hv-24x7.c >> index 573e0b309c0c..6dbbf70232aa 100644 >> --- a/arch/powerpc/perf/hv-24x7.c >> +++ b/arch/powerpc/perf/hv-24x7.c >> @@ -1409,7 +1409,7 @@ static void h_24x7_event_read(struct perf_event *event) >> * that would require issuing a hcall, which would then >> * defeat the purpose of using the txn interface. >> */ >> - local64_set(&event->count, 0); >> + local64_add(0, &event->count); > > ... not sure, how adding zero to the count helps. Should we just remove the > line (and the comment block above it)? Or does it help to clear the event > count in ->start_txn() rather than on read()? Its not impacting much as we are just adding and not setting event count.I think we can remove that line with the comment added. Will it be ok if I remove that whole part? > > How does the change impact the counts when run without the -I? There won't be much impact because from my understanding you did add `(void)local64_xchg(&event->hw.prev_count, ct);`, to print change value in your Commit 2b206ee6b0df in function 'h_24x7_event_init()'. And we will go through this path in case of 'without -I'. Please let me know if my understanding is correct. Thanks, Kajol > > Thanks for chasing this down. > > Sukadev >
diff --git a/arch/powerpc/perf/hv-24x7.c b/arch/powerpc/perf/hv-24x7.c index 573e0b309c0c..6dbbf70232aa 100644 --- a/arch/powerpc/perf/hv-24x7.c +++ b/arch/powerpc/perf/hv-24x7.c @@ -1409,7 +1409,7 @@ static void h_24x7_event_read(struct perf_event *event) * that would require issuing a hcall, which would then * defeat the purpose of using the txn interface. */ - local64_set(&event->count, 0); + local64_add(0, &event->count); } put_cpu_var(hv_24x7_reqb);
Commit 2b206ee6b0df ("powerpc/perf/hv-24x7: Display change in counter values")' added to print _change_ in the counter value rather then raw value for 24x7 counters. Incase of transactions, the event count is set to 0 at the beginning of the transaction. It also sets the event's prev_count to the raw value at the time of initialization. Because of setting event count to 0, we are seeing some weird behaviour, whenever we run multiple 24x7 events at a time. For example: command#: ./perf stat -e "{hv_24x7/PM_MCS01_128B_RD_DISP_PORT01,chip=0/, hv_24x7/PM_MCS01_128B_RD_DISP_PORT01,chip=1/}" -C 0 -I 1000 sleep 100 1.000121704 120 hv_24x7/PM_MCS01_128B_RD_DISP_PORT01,chip=0/ 1.000121704 5 hv_24x7/PM_MCS01_128B_RD_DISP_PORT01,chip=1/ 2.000357733 8 hv_24x7/PM_MCS01_128B_RD_DISP_PORT01,chip=0/ 2.000357733 10 hv_24x7/PM_MCS01_128B_RD_DISP_PORT01,chip=1/ 3.000495215 18,446,744,073,709,551,616 hv_24x7/PM_MCS01_128B_RD_DISP_PORT01,chip=0/ 3.000495215 18,446,744,073,709,551,616 hv_24x7/PM_MCS01_128B_RD_DISP_PORT01,chip=1/ 4.000641884 56 hv_24x7/PM_MCS01_128B_RD_DISP_PORT01,chip=0/ 4.000641884 18,446,744,073,709,551,616 hv_24x7/PM_MCS01_128B_RD_DISP_PORT01,chip=1/ 5.000791887 18,446,744,073,709,551,616 hv_24x7/PM_MCS01_128B_RD_DISP_PORT01,chip=0/ Getting these large values in case we do -I. As we are setting event_count to 0, for interval case, overall event_count is not coming in incremental order. As we may can get new delta lesser then previous count. Because of which when we print intervals, we are getting negative value which create these large values. This patch rather then setting event_count to 0, it change local64_set to local64_add in function 'h_24x7_event_read'. With this patch In power9 platform command#: ./perf stat -e "{hv_24x7/PM_MCS01_128B_RD_DISP_PORT01,chip=0/, hv_24x7/PM_MCS01_128B_RD_DISP_PORT01,chip=1/}" -C 0 -I 1000 sleep 100 1.000117685 93 hv_24x7/PM_MCS01_128B_RD_DISP_PORT01,chip=0/ 1.000117685 1 hv_24x7/PM_MCS01_128B_RD_DISP_PORT01,chip=1/ 2.000349331 98 hv_24x7/PM_MCS01_128B_RD_DISP_PORT01,chip=0/ 2.000349331 2 hv_24x7/PM_MCS01_128B_RD_DISP_PORT01,chip=1/ 3.000495900 131 hv_24x7/PM_MCS01_128B_RD_DISP_PORT01,chip=0/ 3.000495900 4 hv_24x7/PM_MCS01_128B_RD_DISP_PORT01,chip=1/ 4.000645920 204 hv_24x7/PM_MCS01_128B_RD_DISP_PORT01,chip=0/ 4.000645920 61 hv_24x7/PM_MCS01_128B_RD_DISP_PORT01,chip=1/ 4.284169997 22 hv_24x7/PM_MCS01_128B_RD_DISP_PORT01,chip=0/ Signed-off-by: Kajol Jain <kjain@linux.ibm.com> --- arch/powerpc/perf/hv-24x7.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)