| Message ID | 16929569199445cd09d9142505b2349620328b38.1540488386.git.naveen.n.rao@linux.vnet.ibm.com (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | Add dtl_entry tracepoint | expand |
| Context | Check | Description |
|---|---|---|
| snowpatch_ozlabs/apply_patch | success | next/apply_patch Successfully applied |
| snowpatch_ozlabs/checkpatch | success | Test checkpatch on branch next |
On Fri, Oct 26, 2018 at 01:55:44AM +0530, Naveen N. Rao wrote: > When the dtl debugfs interface is used, we usually set the > dtl_enable_mask to 0x7 (DTL_LOG_ALL). When this happens, we start seeing > DTL entries for all preempt reasons, including CEDE. In > scan_dispatch_log(), we add up the times from all entries and account > those towards stolen time. However, we should only be accounting stolen > time when the preemption was due to HDEC at the end of our time slice. It's always been the case that stolen time when idle has been accounted as idle time, not stolen time. That's why we didn't check for this in the past. Do you have a test that shows different results (as in reported idle and stolen times) with this patch compared to without? Paul.
Paul Mackerras wrote: > On Fri, Oct 26, 2018 at 01:55:44AM +0530, Naveen N. Rao wrote: >> When the dtl debugfs interface is used, we usually set the >> dtl_enable_mask to 0x7 (DTL_LOG_ALL). When this happens, we start seeing >> DTL entries for all preempt reasons, including CEDE. In >> scan_dispatch_log(), we add up the times from all entries and account >> those towards stolen time. However, we should only be accounting stolen >> time when the preemption was due to HDEC at the end of our time slice. > > It's always been the case that stolen time when idle has been > accounted as idle time, not stolen time. That's why we didn't check > for this in the past. > > Do you have a test that shows different results (as in reported idle > and stolen times) with this patch compared to without? Ah ok, that makes sense now and explains why I couldn't observe much of a difference in practice. However, I also went by the fact that there are 7 other preemption reasons, which could impact our calculation. Looking at the list again, it looks like H_CONFER/H_PROD and some faults can also have an impact here, though they may be rare? Thanks, Naveen
diff --git a/arch/powerpc/kernel/time.c b/arch/powerpc/kernel/time.c index 40868f3ee113..923abc3e555d 100644 --- a/arch/powerpc/kernel/time.c +++ b/arch/powerpc/kernel/time.c @@ -199,7 +199,7 @@ static u64 scan_dispatch_log(u64 stop_tb) struct lppaca *vpa = local_paca->lppaca_ptr; u64 tb_delta; u64 stolen = 0; - u64 dtb; + u64 dtb, dispatch_reason; if (!dtl) return 0; @@ -210,6 +210,7 @@ static u64 scan_dispatch_log(u64 stop_tb) dtb = be64_to_cpu(dtl->timebase); tb_delta = be32_to_cpu(dtl->enqueue_to_dispatch_time) + be32_to_cpu(dtl->ready_to_enqueue_time); + dispatch_reason = dtl->dispatch_reason; barrier(); if (i + N_DISPATCH_LOG < be64_to_cpu(vpa->dtl_idx)) { /* buffer has overflowed */ @@ -221,7 +222,9 @@ static u64 scan_dispatch_log(u64 stop_tb) break; if (dtl_consumer) dtl_consumer(dtl, i); - stolen += tb_delta; + /* 7 indicates that this dispatch follows a time slice preempt */ + if (dispatch_reason == 7) + stolen += tb_delta; ++i; ++dtl; if (dtl == dtl_end)
When the dtl debugfs interface is used, we usually set the dtl_enable_mask to 0x7 (DTL_LOG_ALL). When this happens, we start seeing DTL entries for all preempt reasons, including CEDE. In scan_dispatch_log(), we add up the times from all entries and account those towards stolen time. However, we should only be accounting stolen time when the preemption was due to HDEC at the end of our time slice. Fix this by checking for the dispatch reason in the DTL entry before adding to the stolen time. Fixes: cf9efce0ce313 ("powerpc: Account time using timebase rather than PURR") Signed-off-by: Naveen N. Rao <naveen.n.rao@linux.vnet.ibm.com> --- arch/powerpc/kernel/time.c | 7 +++++-- 1 file changed, 5 insertions(+), 2 deletions(-)