This patch introduces invalid address macro and uses it to replace dummy value '0xdeadbeefdeadbeefUL'.
Signed-off-by: Leo Yan leo.yan@linaro.org --- tools/perf/util/cs-etm-decoder/cs-etm-decoder.c | 8 ++++---- tools/perf/util/cs-etm-decoder/cs-etm-decoder.h | 2 ++ 2 files changed, 6 insertions(+), 4 deletions(-)
diff --git a/tools/perf/util/cs-etm-decoder/cs-etm-decoder.c b/tools/perf/util/cs-etm-decoder/cs-etm-decoder.c index 4d5fc37..24aabf0 100644 --- a/tools/perf/util/cs-etm-decoder/cs-etm-decoder.c +++ b/tools/perf/util/cs-etm-decoder/cs-etm-decoder.c @@ -261,8 +261,8 @@ static void cs_etm_decoder__clear_buffer(struct cs_etm_decoder *decoder) decoder->tail = 0; decoder->packet_count = 0; for (i = 0; i < MAX_BUFFER; i++) { - decoder->packet_buffer[i].start_addr = 0xdeadbeefdeadbeefUL; - decoder->packet_buffer[i].end_addr = 0xdeadbeefdeadbeefUL; + decoder->packet_buffer[i].start_addr = CS_ETM_INVAL_ADDR; + decoder->packet_buffer[i].end_addr = CS_ETM_INVAL_ADDR; decoder->packet_buffer[i].last_instr_taken_branch = false; decoder->packet_buffer[i].exc = false; decoder->packet_buffer[i].exc_ret = false; @@ -295,8 +295,8 @@ cs_etm_decoder__buffer_packet(struct cs_etm_decoder *decoder, decoder->packet_buffer[et].exc = false; decoder->packet_buffer[et].exc_ret = false; decoder->packet_buffer[et].cpu = *((int *)inode->priv); - decoder->packet_buffer[et].start_addr = 0xdeadbeefdeadbeefUL; - decoder->packet_buffer[et].end_addr = 0xdeadbeefdeadbeefUL; + decoder->packet_buffer[et].start_addr = CS_ETM_INVAL_ADDR; + decoder->packet_buffer[et].end_addr = CS_ETM_INVAL_ADDR;
if (decoder->packet_count == MAX_BUFFER - 1) return OCSD_RESP_WAIT; diff --git a/tools/perf/util/cs-etm-decoder/cs-etm-decoder.h b/tools/perf/util/cs-etm-decoder/cs-etm-decoder.h index 743f5f4..cb31b19 100644 --- a/tools/perf/util/cs-etm-decoder/cs-etm-decoder.h +++ b/tools/perf/util/cs-etm-decoder/cs-etm-decoder.h @@ -13,6 +13,8 @@ #include <linux/types.h> #include <stdio.h>
+#define CS_ETM_INVAL_ADDR 0xdeadbeefdeadbeefUL + struct cs_etm_decoder;
struct cs_etm_buffer {
If the instruction sample failure has happened, it isn't necessary to execute to the end of the function cs_etm__flush(). This commit is to bail out immediately and return the error code.
Signed-off-by: Leo Yan leo.yan@linaro.org --- tools/perf/util/cs-etm.c | 3 +++ 1 file changed, 3 insertions(+)
diff --git a/tools/perf/util/cs-etm.c b/tools/perf/util/cs-etm.c index 822ba91..8b2c099 100644 --- a/tools/perf/util/cs-etm.c +++ b/tools/perf/util/cs-etm.c @@ -939,6 +939,9 @@ static int cs_etm__flush(struct cs_etm_queue *etmq) err = cs_etm__synth_instruction_sample( etmq, addr, etmq->period_instructions); + if (err) + return err; + etmq->period_instructions = 0;
/*
Usually the start tracing packet is a CS_ETM_TRACE_ON packet, this packet is passed to cs_etm__flush(); cs_etm__flush() will check the condition 'prev_packet->sample_type == CS_ETM_RANGE' but 'prev_packet' is allocated by zalloc() so 'prev_packet->sample_type' is zero in initialization and this condition is false. So cs_etm__flush() will directly bail out without handling the start tracing packet.
This patch is to introduce a new sample type CS_ETM_EMPTY, which is used to indicate the packet is an empty packet. cs_etm__flush() will swap packets when it finds the previous packet is empty, so this can record the start tracing packet into 'etmq->prev_packet'.
Signed-off-by: Leo Yan leo.yan@linaro.org --- tools/perf/util/cs-etm-decoder/cs-etm-decoder.h | 1 + tools/perf/util/cs-etm.c | 19 ++++++++++++++++++- 2 files changed, 19 insertions(+), 1 deletion(-)
diff --git a/tools/perf/util/cs-etm-decoder/cs-etm-decoder.h b/tools/perf/util/cs-etm-decoder/cs-etm-decoder.h index cb31b19..108dc9d 100644 --- a/tools/perf/util/cs-etm-decoder/cs-etm-decoder.h +++ b/tools/perf/util/cs-etm-decoder/cs-etm-decoder.h @@ -25,6 +25,7 @@ struct cs_etm_buffer { };
enum cs_etm_sample_type { + CS_ETM_EMPTY = 0, CS_ETM_RANGE = 1 << 0, CS_ETM_TRACE_ON = 1 << 1, }; diff --git a/tools/perf/util/cs-etm.c b/tools/perf/util/cs-etm.c index 8b2c099..ae7c9c88 100644 --- a/tools/perf/util/cs-etm.c +++ b/tools/perf/util/cs-etm.c @@ -924,8 +924,14 @@ static int cs_etm__flush(struct cs_etm_queue *etmq) int err = 0; struct cs_etm_packet *tmp;
+ if (!etmq->prev_packet) + return 0; + + /* Handle start tracing packet */ + if (etmq->prev_packet->sample_type == CS_ETM_EMPTY) + goto swap_packet; + if (etmq->etm->synth_opts.last_branch && - etmq->prev_packet && etmq->prev_packet->sample_type == CS_ETM_RANGE) { /* * Generate a last branch event for the branches left in the @@ -944,6 +950,10 @@ static int cs_etm__flush(struct cs_etm_queue *etmq)
etmq->period_instructions = 0;
+ } + +swap_packet: + if (etmq->etm->synth_opts.last_branch) { /* * Swap PACKET with PREV_PACKET: PACKET becomes PREV_PACKET for * the next incoming packet. @@ -1023,6 +1033,13 @@ static int cs_etm__run_decoder(struct cs_etm_queue *etmq) */ cs_etm__flush(etmq); break; + case CS_ETM_EMPTY: + /* + * Should not receive empty packet, + * report error. + */ + pr_err("CS ETM Trace: empty packet\n"); + return -EINVAL; default: break; }
For CS_ETM_TRACE_ON packet, its fields 'packet->start_addr' and 'packet->end_addr' equal to 0xdeadbeefdeadbeefUL which are emitted in the decoder layer as dummy value, but the dummy value is pointless for branch sample when we use 'perf script' command to check program flow.
This patch is a preparation to support CS_ETM_TRACE_ON packet for branch sample, it converts the dummy address value to zero for more readable; this is accomplished by cs_etm__last_executed_instr() and cs_etm__first_executed_instr(). The later one is a new function introduced by this patch.
Signed-off-by: Leo Yan leo.yan@linaro.org --- tools/perf/util/cs-etm.c | 17 +++++++++++++++-- 1 file changed, 15 insertions(+), 2 deletions(-)
diff --git a/tools/perf/util/cs-etm.c b/tools/perf/util/cs-etm.c index ae7c9c88..976db84 100644 --- a/tools/perf/util/cs-etm.c +++ b/tools/perf/util/cs-etm.c @@ -494,6 +494,10 @@ static inline void cs_etm__reset_last_branch_rb(struct cs_etm_queue *etmq)
static inline u64 cs_etm__last_executed_instr(struct cs_etm_packet *packet) { + /* Returns 0 for the CS_ETM_TRACE_ON packet */ + if (packet->sample_type == CS_ETM_TRACE_ON) + return 0; + /* * The packet records the execution range with an exclusive end address * @@ -505,6 +509,15 @@ static inline u64 cs_etm__last_executed_instr(struct cs_etm_packet *packet) return packet->end_addr - A64_INSTR_SIZE; }
+static inline u64 cs_etm__first_executed_instr(struct cs_etm_packet *packet) +{ + /* Returns 0 for the CS_ETM_TRACE_ON packet */ + if (packet->sample_type == CS_ETM_TRACE_ON) + return 0; + + return packet->start_addr; +} + static inline u64 cs_etm__instr_count(const struct cs_etm_packet *packet) { /* @@ -546,7 +559,7 @@ static void cs_etm__update_last_branch_rb(struct cs_etm_queue *etmq)
be = &bs->entries[etmq->last_branch_pos]; be->from = cs_etm__last_executed_instr(etmq->prev_packet); - be->to = etmq->packet->start_addr; + be->to = cs_etm__first_executed_instr(etmq->packet); /* No support for mispredict */ be->flags.mispred = 0; be->flags.predicted = 1; @@ -701,7 +714,7 @@ static int cs_etm__synth_branch_sample(struct cs_etm_queue *etmq) sample.ip = cs_etm__last_executed_instr(etmq->prev_packet); sample.pid = etmq->pid; sample.tid = etmq->tid; - sample.addr = etmq->packet->start_addr; + sample.addr = cs_etm__first_executed_instr(etmq->packet); sample.id = etmq->etm->branches_id; sample.stream_id = etmq->etm->branches_id; sample.period = 1;
If one CS_ETM_TRACE_ON packet is inserted, we miss to generate branch sample for the previous CS_ETM_RANGE packet.
This patch is to generate branch sample when receiving a CS_ETM_TRACE_ON packet, so this can save complete info for the previous CS_ETM_RANGE packet just before CS_ETM_TRACE_ON packet.
Signed-off-by: Leo Yan leo.yan@linaro.org --- tools/perf/util/cs-etm.c | 8 ++++++++ 1 file changed, 8 insertions(+)
diff --git a/tools/perf/util/cs-etm.c b/tools/perf/util/cs-etm.c index 976db84..d3b7942 100644 --- a/tools/perf/util/cs-etm.c +++ b/tools/perf/util/cs-etm.c @@ -935,6 +935,7 @@ static int cs_etm__sample(struct cs_etm_queue *etmq) static int cs_etm__flush(struct cs_etm_queue *etmq) { int err = 0; + struct cs_etm_auxtrace *etm = etmq->etm; struct cs_etm_packet *tmp;
if (!etmq->prev_packet) @@ -965,6 +966,13 @@ static int cs_etm__flush(struct cs_etm_queue *etmq)
}
+ if (etm->sample_branches && + etmq->prev_packet->sample_type == CS_ETM_RANGE) { + err = cs_etm__synth_branch_sample(etmq); + if (err) + return err; + } + swap_packet: if (etmq->etm->synth_opts.last_branch) { /*
CS_ETM_TRACE_ON packet itself can give the info that there have a discontinuity in the trace, this patch is to add branch sample for CS_ETM_TRACE_ON packet if it is inserted in the middle of CS_ETM_RANGE packets; as result we can have hint for the trace discontinuity.
Signed-off-by: Leo Yan leo.yan@linaro.org --- tools/perf/util/cs-etm.c | 24 +++++++++++++++++------- 1 file changed, 17 insertions(+), 7 deletions(-)
diff --git a/tools/perf/util/cs-etm.c b/tools/perf/util/cs-etm.c index d3b7942..2ae6402 100644 --- a/tools/perf/util/cs-etm.c +++ b/tools/perf/util/cs-etm.c @@ -910,13 +910,23 @@ static int cs_etm__sample(struct cs_etm_queue *etmq) etmq->period_instructions = instrs_over; }
- if (etm->sample_branches && - etmq->prev_packet && - etmq->prev_packet->sample_type == CS_ETM_RANGE && - etmq->prev_packet->last_instr_taken_branch) { - ret = cs_etm__synth_branch_sample(etmq); - if (ret) - return ret; + if (etm->sample_branches && etmq->prev_packet) { + bool generate_sample = false; + + /* Generate sample for tracing on packet */ + if (etmq->prev_packet->sample_type == CS_ETM_TRACE_ON) + generate_sample = true; + + /* Generate sample for branch taken packet */ + if (etmq->prev_packet->sample_type == CS_ETM_RANGE && + etmq->prev_packet->last_instr_taken_branch) + generate_sample = true; + + if (generate_sample) { + ret = cs_etm__synth_branch_sample(etmq); + if (ret) + return ret; + } }
if (etm->sample_branches || etm->synth_opts.last_branch) {
Hi Mathieu,
On Sun, Jun 10, 2018 at 04:19:05PM +0800, Leo Yan wrote:
CS_ETM_TRACE_ON packet itself can give the info that there have a discontinuity in the trace, this patch is to add branch sample for CS_ETM_TRACE_ON packet if it is inserted in the middle of CS_ETM_RANGE packets; as result we can have hint for the trace discontinuity.
I followed your suggestion to prepare the cs-etm fixing patch series (if I didn't miss anything), but I am struggling to add this patch into the fixing patch series or not, though I understand I should keep the patch series as simple as possible for easier reviewing and merging.
Seems to me, the patch 5/6 'perf cs-etm: Generate branch sample when receiving a CS_ETM_TRACE_ON packet' has inserted partial info of CS_ETM_TRACE_ON for samples, e.g. the CS_ETM_TRACE_ON packet::addr has been used to generate branch sample, if without this patch we will miss the branch sample for CS_ETM_TRACE_ON::ip. So I decided to add this patch into fixing series. So my main reason for adding patch 6/6 is to make the fixing to be more complete.
Please let me know if you agree with this? Or have other suggestion. Thanks in advance.
Signed-off-by: Leo Yan leo.yan@linaro.org
tools/perf/util/cs-etm.c | 24 +++++++++++++++++------- 1 file changed, 17 insertions(+), 7 deletions(-)
diff --git a/tools/perf/util/cs-etm.c b/tools/perf/util/cs-etm.c index d3b7942..2ae6402 100644 --- a/tools/perf/util/cs-etm.c +++ b/tools/perf/util/cs-etm.c @@ -910,13 +910,23 @@ static int cs_etm__sample(struct cs_etm_queue *etmq) etmq->period_instructions = instrs_over; }
- if (etm->sample_branches &&
etmq->prev_packet &&etmq->prev_packet->sample_type == CS_ETM_RANGE &&etmq->prev_packet->last_instr_taken_branch) {ret = cs_etm__synth_branch_sample(etmq);if (ret)return ret;
- if (etm->sample_branches && etmq->prev_packet) {
bool generate_sample = false;/* Generate sample for tracing on packet */if (etmq->prev_packet->sample_type == CS_ETM_TRACE_ON)generate_sample = true;/* Generate sample for branch taken packet */if (etmq->prev_packet->sample_type == CS_ETM_RANGE &&etmq->prev_packet->last_instr_taken_branch)generate_sample = true;if (generate_sample) {ret = cs_etm__synth_branch_sample(etmq);if (ret)return ret;}if (etm->sample_branches || etm->synth_opts.last_branch) { -- 2.7.4
On 10 June 2018 at 02:28, Leo Yan leo.yan@linaro.org wrote:
Hi Mathieu,
On Sun, Jun 10, 2018 at 04:19:05PM +0800, Leo Yan wrote:
CS_ETM_TRACE_ON packet itself can give the info that there have a discontinuity in the trace, this patch is to add branch sample for CS_ETM_TRACE_ON packet if it is inserted in the middle of CS_ETM_RANGE packets; as result we can have hint for the trace discontinuity.
I followed your suggestion to prepare the cs-etm fixing patch series (if I didn't miss anything), but I am struggling to add this patch into the fixing patch series or not, though I understand I should keep the patch series as simple as possible for easier reviewing and merging.
Seems to me, the patch 5/6 'perf cs-etm: Generate branch sample when receiving a CS_ETM_TRACE_ON packet' has inserted partial info of CS_ETM_TRACE_ON for samples, e.g. the CS_ETM_TRACE_ON packet::addr has been used to generate branch sample, if without this patch we will miss the branch sample for CS_ETM_TRACE_ON::ip. So I decided to add this patch into fixing series. So my main reason for adding patch 6/6 is to make the fixing to be more complete.
Please let me know if you agree with this? Or have other suggestion. Thanks in advance.
Good day Leo,
Aside from the comment in 1/6 I think the compartmentalisation of this series is just fine as it is. I am personally fine with it and think it is ready for action on the public mailing list again. As I said earlier, you will need to wait for the merge window to close.
In the mean time, Mike and Rob - please review this set and see if anything is missing.
Thanks, Mathieu
Signed-off-by: Leo Yan leo.yan@linaro.org
tools/perf/util/cs-etm.c | 24 +++++++++++++++++------- 1 file changed, 17 insertions(+), 7 deletions(-)
diff --git a/tools/perf/util/cs-etm.c b/tools/perf/util/cs-etm.c index d3b7942..2ae6402 100644 --- a/tools/perf/util/cs-etm.c +++ b/tools/perf/util/cs-etm.c @@ -910,13 +910,23 @@ static int cs_etm__sample(struct cs_etm_queue *etmq) etmq->period_instructions = instrs_over; }
if (etm->sample_branches &&etmq->prev_packet &&etmq->prev_packet->sample_type == CS_ETM_RANGE &&etmq->prev_packet->last_instr_taken_branch) {ret = cs_etm__synth_branch_sample(etmq);if (ret)return ret;
if (etm->sample_branches && etmq->prev_packet) {bool generate_sample = false;/* Generate sample for tracing on packet */if (etmq->prev_packet->sample_type == CS_ETM_TRACE_ON)generate_sample = true;/* Generate sample for branch taken packet */if (etmq->prev_packet->sample_type == CS_ETM_RANGE &&etmq->prev_packet->last_instr_taken_branch)generate_sample = true;if (generate_sample) {ret = cs_etm__synth_branch_sample(etmq);if (ret)return ret;} } if (etm->sample_branches || etm->synth_opts.last_branch) {-- 2.7.4
On Mon, Jun 11, 2018 at 11:13:48AM -0600, Mathieu Poirier wrote:
On 10 June 2018 at 02:28, Leo Yan leo.yan@linaro.org wrote:
Hi Mathieu,
On Sun, Jun 10, 2018 at 04:19:05PM +0800, Leo Yan wrote:
CS_ETM_TRACE_ON packet itself can give the info that there have a discontinuity in the trace, this patch is to add branch sample for CS_ETM_TRACE_ON packet if it is inserted in the middle of CS_ETM_RANGE packets; as result we can have hint for the trace discontinuity.
I followed your suggestion to prepare the cs-etm fixing patch series (if I didn't miss anything), but I am struggling to add this patch into the fixing patch series or not, though I understand I should keep the patch series as simple as possible for easier reviewing and merging.
Seems to me, the patch 5/6 'perf cs-etm: Generate branch sample when receiving a CS_ETM_TRACE_ON packet' has inserted partial info of CS_ETM_TRACE_ON for samples, e.g. the CS_ETM_TRACE_ON packet::addr has been used to generate branch sample, if without this patch we will miss the branch sample for CS_ETM_TRACE_ON::ip. So I decided to add this patch into fixing series. So my main reason for adding patch 6/6 is to make the fixing to be more complete.
Please let me know if you agree with this? Or have other suggestion. Thanks in advance.
Good day Leo,
Aside from the comment in 1/6 I think the compartmentalisation of this series is just fine as it is. I am personally fine with it and think it is ready for action on the public mailing list again. As I said earlier, you will need to wait for the merge window to close.
Thanks for reviewing, Mathieu. Will fix 1/6 with suggestion.
In the mean time, Mike and Rob - please review this set and see if anything is missing.
Yeah, It's good to get green light or suggestions from Mike and Rob before I send patch series to public mailing list.
BTW, from LWN article [1] I get to know the 4.18 merge window seems to be delayed; I will check with you offline for when will be good time to send to LKML.
[1] https://lwn.net/Articles/756898/
Thanks, Leo Yan
On 11 June 2018 at 19:02, Leo Yan leo.yan@linaro.org wrote:
On Mon, Jun 11, 2018 at 11:13:48AM -0600, Mathieu Poirier wrote:
On 10 June 2018 at 02:28, Leo Yan leo.yan@linaro.org wrote:
Hi Mathieu,
On Sun, Jun 10, 2018 at 04:19:05PM +0800, Leo Yan wrote:
CS_ETM_TRACE_ON packet itself can give the info that there have a discontinuity in the trace, this patch is to add branch sample for CS_ETM_TRACE_ON packet if it is inserted in the middle of CS_ETM_RANGE packets; as result we can have hint for the trace discontinuity.
I followed your suggestion to prepare the cs-etm fixing patch series (if I didn't miss anything), but I am struggling to add this patch into the fixing patch series or not, though I understand I should keep the patch series as simple as possible for easier reviewing and merging.
Seems to me, the patch 5/6 'perf cs-etm: Generate branch sample when receiving a CS_ETM_TRACE_ON packet' has inserted partial info of CS_ETM_TRACE_ON for samples, e.g. the CS_ETM_TRACE_ON packet::addr has been used to generate branch sample, if without this patch we will miss the branch sample for CS_ETM_TRACE_ON::ip. So I decided to add this patch into fixing series. So my main reason for adding patch 6/6 is to make the fixing to be more complete.
Please let me know if you agree with this? Or have other suggestion. Thanks in advance.
Good day Leo,
Aside from the comment in 1/6 I think the compartmentalisation of this series is just fine as it is. I am personally fine with it and think it is ready for action on the public mailing list again. As I said earlier, you will need to wait for the merge window to close.
Thanks for reviewing, Mathieu. Will fix 1/6 with suggestion.
In the mean time, Mike and Rob - please review this set and see if anything is missing.
Yeah, It's good to get green light or suggestions from Mike and Rob before I send patch series to public mailing list.
BTW, from LWN article [1] I get to know the 4.18 merge window seems to be delayed; I will check with you offline for when will be good time to send to LKML.
The merge window hasn't been delayed. It opens on the day the official kernel release happens and lasts for two weeks. In this case 4.17 was released on the 3rd of June, which means the merge window will close on the 16th and a new 4.18-rc1 released on the 17th. You can send you patches as soon as you have a 4.18-rc1 to base your work on.
[1] https://lwn.net/Articles/756898/
Thanks, Leo Yan
On Tue, Jun 12, 2018 at 08:36:08AM -0600, Mathieu Poirier wrote:
[...]
BTW, from LWN article [1] I get to know the 4.18 merge window seems to be delayed; I will check with you offline for when will be good time to send to LKML.
The merge window hasn't been delayed. It opens on the day the official kernel release happens and lasts for two weeks. In this case 4.17 was released on the 3rd of June, which means the merge window will close on the 16th and a new 4.18-rc1 released on the 17th. You can send you patches as soon as you have a 4.18-rc1 to base your work on.
Yeah, will monitor the 4.18-rc1 releasing in next a few days. Thanks for detailed explaination.
[1] https://lwn.net/Articles/756898/
Thanks, Leo Yan
On 12/06/18 02:02, Leo Yan wrote:
On Mon, Jun 11, 2018 at 11:13:48AM -0600, Mathieu Poirier wrote:
On 10 June 2018 at 02:28, Leo Yan leo.yan@linaro.org wrote:
Hi Mathieu,
On Sun, Jun 10, 2018 at 04:19:05PM +0800, Leo Yan wrote:
CS_ETM_TRACE_ON packet itself can give the info that there have a discontinuity in the trace, this patch is to add branch sample for CS_ETM_TRACE_ON packet if it is inserted in the middle of CS_ETM_RANGE packets; as result we can have hint for the trace discontinuity.
I followed your suggestion to prepare the cs-etm fixing patch series (if I didn't miss anything), but I am struggling to add this patch into the fixing patch series or not, though I understand I should keep the patch series as simple as possible for easier reviewing and merging.
Seems to me, the patch 5/6 'perf cs-etm: Generate branch sample when receiving a CS_ETM_TRACE_ON packet' has inserted partial info of CS_ETM_TRACE_ON for samples, e.g. the CS_ETM_TRACE_ON packet::addr has been used to generate branch sample, if without this patch we will miss the branch sample for CS_ETM_TRACE_ON::ip. So I decided to add this patch into fixing series. So my main reason for adding patch 6/6 is to make the fixing to be more complete.
Please let me know if you agree with this? Or have other suggestion. Thanks in advance.
Good day Leo,
Aside from the comment in 1/6 I think the compartmentalisation of this series is just fine as it is. I am personally fine with it and think it is ready for action on the public mailing list again. As I said earlier, you will need to wait for the merge window to close.
Thanks for reviewing, Mathieu. Will fix 1/6 with suggestion.
In the mean time, Mike and Rob - please review this set and see if anything is missing.
Yeah, It's good to get green light or suggestions from Mike and Rob before I send patch series to public mailing list.
The patches look good to me and the instruction sample / branch stack output for AutoFDO is unchanged, so I'm happy.
Are there any automated regression tests run for perf? It would be useful to have a set of trace captures that can be run through perf inject/report/script and the output checked for expected behaviours.
BTW, from LWN article [1] I get to know the 4.18 merge window seems to be delayed; I will check with you offline for when will be good time to send to LKML.
[1] https://lwn.net/Articles/756898/
Thanks, Leo Yan
Regards
Rob
On Wed, Jun 13, 2018 at 12:11:40PM +0100, Robert Walker wrote:
On 12/06/18 02:02, Leo Yan wrote:
On Mon, Jun 11, 2018 at 11:13:48AM -0600, Mathieu Poirier wrote:
On 10 June 2018 at 02:28, Leo Yan leo.yan@linaro.org wrote:
Hi Mathieu,
On Sun, Jun 10, 2018 at 04:19:05PM +0800, Leo Yan wrote:
CS_ETM_TRACE_ON packet itself can give the info that there have a discontinuity in the trace, this patch is to add branch sample for CS_ETM_TRACE_ON packet if it is inserted in the middle of CS_ETM_RANGE packets; as result we can have hint for the trace discontinuity.
I followed your suggestion to prepare the cs-etm fixing patch series (if I didn't miss anything), but I am struggling to add this patch into the fixing patch series or not, though I understand I should keep the patch series as simple as possible for easier reviewing and merging.
Seems to me, the patch 5/6 'perf cs-etm: Generate branch sample when receiving a CS_ETM_TRACE_ON packet' has inserted partial info of CS_ETM_TRACE_ON for samples, e.g. the CS_ETM_TRACE_ON packet::addr has been used to generate branch sample, if without this patch we will miss the branch sample for CS_ETM_TRACE_ON::ip. So I decided to add this patch into fixing series. So my main reason for adding patch 6/6 is to make the fixing to be more complete.
Please let me know if you agree with this? Or have other suggestion. Thanks in advance.
Good day Leo,
Aside from the comment in 1/6 I think the compartmentalisation of this series is just fine as it is. I am personally fine with it and think it is ready for action on the public mailing list again. As I said earlier, you will need to wait for the merge window to close.
Thanks for reviewing, Mathieu. Will fix 1/6 with suggestion.
In the mean time, Mike and Rob - please review this set and see if anything is missing.
Yeah, It's good to get green light or suggestions from Mike and Rob before I send patch series to public mailing list.
The patches look good to me and the instruction sample / branch stack output for AutoFDO is unchanged, so I'm happy.
Thanks a lot for reviewing and testing, Rob.
Are there any automated regression tests run for perf? It would be useful to have a set of trace captures that can be run through perf inject/report/script and the output checked for expected behaviours.
Good question and I also have same question for this, usually I need to manually run 'perf script' command at my side.
I search a bit, perf command has one sub command 'perf test' for sanity testing; and in the kernel source code tools/perf/tests there have some testing cases. But I don't really dig into the testing code. Let's see if others have better suggestion for this.
Thanks, Leo Yan
On Wed, 13 Jun 2018 20:15:54 +0800 Leo Yan leo.yan@linaro.org wrote:
On Wed, Jun 13, 2018 at 12:11:40PM +0100, Robert Walker wrote:
Are there any automated regression tests run for perf? It would be useful to have a set of trace captures that can be run through perf inject/report/script and the output checked for expected behaviours.
Adding record to that list would be good, too.
Good question and I also have same question for this, usually I need to manually run 'perf script' command at my side.
I search a bit, perf command has one sub command 'perf test' for sanity testing; and in the kernel source code tools/perf/tests there have some testing cases. But I don't really dig into the testing code. Let's see if others have better suggestion for this.
There aren't any existing AUXTRACE driver tests, AFAICS. OTOH, tools/perf/tests/shell/record+script_probe_vfs_getname.sh does a record and script. I'm not sure how acceptable it would be to add an ARCH-and-PMU-specific test there. It would at least have to 'skip' itself if the appropriate CoreSight resources weren't available.
Kim
On 10 June 2018 at 02:19, Leo Yan leo.yan@linaro.org wrote:
This patch introduces invalid address macro and uses it to replace dummy value '0xdeadbeefdeadbeefUL'.
Signed-off-by: Leo Yan leo.yan@linaro.org
tools/perf/util/cs-etm-decoder/cs-etm-decoder.c | 8 ++++---- tools/perf/util/cs-etm-decoder/cs-etm-decoder.h | 2 ++ 2 files changed, 6 insertions(+), 4 deletions(-)
diff --git a/tools/perf/util/cs-etm-decoder/cs-etm-decoder.c b/tools/perf/util/cs-etm-decoder/cs-etm-decoder.c index 4d5fc37..24aabf0 100644 --- a/tools/perf/util/cs-etm-decoder/cs-etm-decoder.c +++ b/tools/perf/util/cs-etm-decoder/cs-etm-decoder.c @@ -261,8 +261,8 @@ static void cs_etm_decoder__clear_buffer(struct cs_etm_decoder *decoder) decoder->tail = 0; decoder->packet_count = 0; for (i = 0; i < MAX_BUFFER; i++) {
decoder->packet_buffer[i].start_addr = 0xdeadbeefdeadbeefUL;decoder->packet_buffer[i].end_addr = 0xdeadbeefdeadbeefUL;
decoder->packet_buffer[i].start_addr = CS_ETM_INVAL_ADDR;decoder->packet_buffer[i].end_addr = CS_ETM_INVAL_ADDR; decoder->packet_buffer[i].last_instr_taken_branch = false; decoder->packet_buffer[i].exc = false; decoder->packet_buffer[i].exc_ret = false;@@ -295,8 +295,8 @@ cs_etm_decoder__buffer_packet(struct cs_etm_decoder *decoder, decoder->packet_buffer[et].exc = false; decoder->packet_buffer[et].exc_ret = false; decoder->packet_buffer[et].cpu = *((int *)inode->priv);
decoder->packet_buffer[et].start_addr = 0xdeadbeefdeadbeefUL;decoder->packet_buffer[et].end_addr = 0xdeadbeefdeadbeefUL;
decoder->packet_buffer[et].start_addr = CS_ETM_INVAL_ADDR;decoder->packet_buffer[et].end_addr = CS_ETM_INVAL_ADDR; if (decoder->packet_count == MAX_BUFFER - 1) return OCSD_RESP_WAIT;diff --git a/tools/perf/util/cs-etm-decoder/cs-etm-decoder.h b/tools/perf/util/cs-etm-decoder/cs-etm-decoder.h index 743f5f4..cb31b19 100644 --- a/tools/perf/util/cs-etm-decoder/cs-etm-decoder.h +++ b/tools/perf/util/cs-etm-decoder/cs-etm-decoder.h @@ -13,6 +13,8 @@ #include <linux/types.h> #include <stdio.h>
+#define CS_ETM_INVAL_ADDR 0xdeadbeefdeadbeefUL
As far as I can tell this isn't needed outside of cs-etm-decoder.c and as such shouldn't be in the .h file. Please move to the top of cs-etm-decoder.c.
struct cs_etm_decoder;
struct cs_etm_buffer {
2.7.4
-
Kim Phillips -
Leo Yan -
Mathieu Poirier -
Robert Walker