* [PATCH v2 2/2] perf lock: Don't free "lock_seq_stat" if read_count isn't zero
2020-10-21 0:39 [PATCH v2 1/2] perf lock: Correct field name "flags" Leo Yan
@ 2020-10-21 0:39 ` Leo Yan
2020-11-02 16:56 ` Jiri Olsa
2020-11-02 10:24 ` [PATCH v2 1/2] perf lock: Correct field name "flags" Leo Yan
2020-11-02 16:42 ` Jiri Olsa
2 siblings, 1 reply; 8+ messages in thread
From: Leo Yan @ 2020-10-21 0:39 UTC (permalink / raw)
To: Arnaldo Carvalho de Melo, Peter Zijlstra, Ingo Molnar,
Mark Rutland, Alexander Shishkin, Jiri Olsa, Namhyung Kim,
Hitoshi Mitake, Frederic Weisbecker, linux-kernel
Cc: Leo Yan
When execute command "perf lock report", it hits failure and outputs log
as follows:
perf: builtin-lock.c:623: report_lock_release_event: Assertion `!(seq->read_count < 0)' failed.
Aborted
This is an imbalance issue. The locking sequence structure
"lock_seq_stat" contains the reader counter and it is used to check if
the locking sequence is balance or not between acquiring and releasing.
If the tool wrongly frees "lock_seq_stat" when "read_count" isn't zero,
the "read_count" will be reset to zero when allocate a new structure at
the next time; thus it causes the wrong counting for reader and finally
results in imbalance issue.
To fix this issue, if detects "read_count" is not zero (means still
have read user in the locking sequence), goto the "end" tag to skip
freeing structure "lock_seq_stat".
Fixes: e4cef1f65061 ("perf lock: Fix state machine to recognize lock sequence")
Signed-off-by: Leo Yan <leo.yan@linaro.org>
---
tools/perf/builtin-lock.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/tools/perf/builtin-lock.c b/tools/perf/builtin-lock.c
index 5cecc1ad78e1..a2f1e53f37a7 100644
--- a/tools/perf/builtin-lock.c
+++ b/tools/perf/builtin-lock.c
@@ -621,7 +621,7 @@ static int report_lock_release_event(struct evsel *evsel,
case SEQ_STATE_READ_ACQUIRED:
seq->read_count--;
BUG_ON(seq->read_count < 0);
- if (!seq->read_count) {
+ if (seq->read_count) {
ls->nr_release++;
goto end;
}
--
2.17.1
^ permalink raw reply related [flat|nested] 8+ messages in thread* Re: [PATCH v2 2/2] perf lock: Don't free "lock_seq_stat" if read_count isn't zero
2020-10-21 0:39 ` [PATCH v2 2/2] perf lock: Don't free "lock_seq_stat" if read_count isn't zero Leo Yan
@ 2020-11-02 16:56 ` Jiri Olsa
2020-11-03 2:29 ` Leo Yan
0 siblings, 1 reply; 8+ messages in thread
From: Jiri Olsa @ 2020-11-02 16:56 UTC (permalink / raw)
To: Leo Yan
Cc: Arnaldo Carvalho de Melo, Peter Zijlstra, Ingo Molnar,
Mark Rutland, Alexander Shishkin, Namhyung Kim, Hitoshi Mitake,
Frederic Weisbecker, linux-kernel
On Wed, Oct 21, 2020 at 08:39:48AM +0800, Leo Yan wrote:
> When execute command "perf lock report", it hits failure and outputs log
> as follows:
>
> perf: builtin-lock.c:623: report_lock_release_event: Assertion `!(seq->read_count < 0)' failed.
> Aborted
>
> This is an imbalance issue. The locking sequence structure
> "lock_seq_stat" contains the reader counter and it is used to check if
> the locking sequence is balance or not between acquiring and releasing.
>
> If the tool wrongly frees "lock_seq_stat" when "read_count" isn't zero,
> the "read_count" will be reset to zero when allocate a new structure at
> the next time; thus it causes the wrong counting for reader and finally
> results in imbalance issue.
>
> To fix this issue, if detects "read_count" is not zero (means still
> have read user in the locking sequence), goto the "end" tag to skip
> freeing structure "lock_seq_stat".
>
> Fixes: e4cef1f65061 ("perf lock: Fix state machine to recognize lock sequence")
> Signed-off-by: Leo Yan <leo.yan@linaro.org>
> ---
> tools/perf/builtin-lock.c | 2 +-
> 1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/tools/perf/builtin-lock.c b/tools/perf/builtin-lock.c
> index 5cecc1ad78e1..a2f1e53f37a7 100644
> --- a/tools/perf/builtin-lock.c
> +++ b/tools/perf/builtin-lock.c
> @@ -621,7 +621,7 @@ static int report_lock_release_event(struct evsel *evsel,
> case SEQ_STATE_READ_ACQUIRED:
> seq->read_count--;
> BUG_ON(seq->read_count < 0);
> - if (!seq->read_count) {
> + if (seq->read_count) {
> ls->nr_release++;
it seems ok, but I fail to see what's nr_release for
the point is just to skip the removal of seq right?
jirka
> goto end;
> }
> --
> 2.17.1
>
^ permalink raw reply [flat|nested] 8+ messages in thread* Re: [PATCH v2 2/2] perf lock: Don't free "lock_seq_stat" if read_count isn't zero
2020-11-02 16:56 ` Jiri Olsa
@ 2020-11-03 2:29 ` Leo Yan
2020-11-03 11:53 ` Jiri Olsa
0 siblings, 1 reply; 8+ messages in thread
From: Leo Yan @ 2020-11-03 2:29 UTC (permalink / raw)
To: Jiri Olsa
Cc: Arnaldo Carvalho de Melo, Peter Zijlstra, Ingo Molnar,
Mark Rutland, Alexander Shishkin, Namhyung Kim, Hitoshi Mitake,
Frederic Weisbecker, linux-kernel
On Mon, Nov 02, 2020 at 05:56:26PM +0100, Jiri Olsa wrote:
> On Wed, Oct 21, 2020 at 08:39:48AM +0800, Leo Yan wrote:
> > When execute command "perf lock report", it hits failure and outputs log
> > as follows:
> >
> > perf: builtin-lock.c:623: report_lock_release_event: Assertion `!(seq->read_count < 0)' failed.
> > Aborted
> >
> > This is an imbalance issue. The locking sequence structure
> > "lock_seq_stat" contains the reader counter and it is used to check if
> > the locking sequence is balance or not between acquiring and releasing.
> >
> > If the tool wrongly frees "lock_seq_stat" when "read_count" isn't zero,
> > the "read_count" will be reset to zero when allocate a new structure at
> > the next time; thus it causes the wrong counting for reader and finally
> > results in imbalance issue.
> >
> > To fix this issue, if detects "read_count" is not zero (means still
> > have read user in the locking sequence), goto the "end" tag to skip
> > freeing structure "lock_seq_stat".
> >
> > Fixes: e4cef1f65061 ("perf lock: Fix state machine to recognize lock sequence")
> > Signed-off-by: Leo Yan <leo.yan@linaro.org>
> > ---
> > tools/perf/builtin-lock.c | 2 +-
> > 1 file changed, 1 insertion(+), 1 deletion(-)
> >
> > diff --git a/tools/perf/builtin-lock.c b/tools/perf/builtin-lock.c
> > index 5cecc1ad78e1..a2f1e53f37a7 100644
> > --- a/tools/perf/builtin-lock.c
> > +++ b/tools/perf/builtin-lock.c
> > @@ -621,7 +621,7 @@ static int report_lock_release_event(struct evsel *evsel,
> > case SEQ_STATE_READ_ACQUIRED:
> > seq->read_count--;
> > BUG_ON(seq->read_count < 0);
> > - if (!seq->read_count) {
> > + if (seq->read_count) {
> > ls->nr_release++;
>
> it seems ok, but I fail to see what's nr_release for
> the point is just to skip the removal of seq right?
To be honest, I'm not sure if I understand your question :)
Either remove "seq" or not, "nr_release" will be increased. When remove
"seq", the code line [1] will increase '1' for "nr_release"; when skip
to remove "seq", "nr_release" is also increased 1 [2]. So I don't see
the logic issue for "nr_release", do I miss anything?
Another side topic is the four metrics "nr_acquire", "nr_release",
"nr_readlock", "nr_trylock" have been accounted, but they are not really
used for output final result. I'd like to defer this later as a task
for refine the output metrics.
Thanks,
Leo
[1] https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/tools/perf/builtin-lock.c#n641
[2] https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/tools/perf/builtin-lock.c#n625
^ permalink raw reply [flat|nested] 8+ messages in thread* Re: [PATCH v2 2/2] perf lock: Don't free "lock_seq_stat" if read_count isn't zero
2020-11-03 2:29 ` Leo Yan
@ 2020-11-03 11:53 ` Jiri Olsa
0 siblings, 0 replies; 8+ messages in thread
From: Jiri Olsa @ 2020-11-03 11:53 UTC (permalink / raw)
To: Leo Yan
Cc: Arnaldo Carvalho de Melo, Peter Zijlstra, Ingo Molnar,
Mark Rutland, Alexander Shishkin, Namhyung Kim, Hitoshi Mitake,
Frederic Weisbecker, linux-kernel
On Tue, Nov 03, 2020 at 10:29:44AM +0800, Leo Yan wrote:
> On Mon, Nov 02, 2020 at 05:56:26PM +0100, Jiri Olsa wrote:
> > On Wed, Oct 21, 2020 at 08:39:48AM +0800, Leo Yan wrote:
> > > When execute command "perf lock report", it hits failure and outputs log
> > > as follows:
> > >
> > > perf: builtin-lock.c:623: report_lock_release_event: Assertion `!(seq->read_count < 0)' failed.
> > > Aborted
> > >
> > > This is an imbalance issue. The locking sequence structure
> > > "lock_seq_stat" contains the reader counter and it is used to check if
> > > the locking sequence is balance or not between acquiring and releasing.
> > >
> > > If the tool wrongly frees "lock_seq_stat" when "read_count" isn't zero,
> > > the "read_count" will be reset to zero when allocate a new structure at
> > > the next time; thus it causes the wrong counting for reader and finally
> > > results in imbalance issue.
> > >
> > > To fix this issue, if detects "read_count" is not zero (means still
> > > have read user in the locking sequence), goto the "end" tag to skip
> > > freeing structure "lock_seq_stat".
> > >
> > > Fixes: e4cef1f65061 ("perf lock: Fix state machine to recognize lock sequence")
> > > Signed-off-by: Leo Yan <leo.yan@linaro.org>
> > > ---
> > > tools/perf/builtin-lock.c | 2 +-
> > > 1 file changed, 1 insertion(+), 1 deletion(-)
> > >
> > > diff --git a/tools/perf/builtin-lock.c b/tools/perf/builtin-lock.c
> > > index 5cecc1ad78e1..a2f1e53f37a7 100644
> > > --- a/tools/perf/builtin-lock.c
> > > +++ b/tools/perf/builtin-lock.c
> > > @@ -621,7 +621,7 @@ static int report_lock_release_event(struct evsel *evsel,
> > > case SEQ_STATE_READ_ACQUIRED:
> > > seq->read_count--;
> > > BUG_ON(seq->read_count < 0);
> > > - if (!seq->read_count) {
> > > + if (seq->read_count) {
> > > ls->nr_release++;
> >
> > it seems ok, but I fail to see what's nr_release for
> > the point is just to skip the removal of seq right?
>
> To be honest, I'm not sure if I understand your question :)
>
> Either remove "seq" or not, "nr_release" will be increased. When remove
> "seq", the code line [1] will increase '1' for "nr_release"; when skip
> to remove "seq", "nr_release" is also increased 1 [2]. So I don't see
> the logic issue for "nr_release", do I miss anything?
>
> Another side topic is the four metrics "nr_acquire", "nr_release",
> "nr_readlock", "nr_trylock" have been accounted, but they are not really
> used for output final result. I'd like to defer this later as a task
> for refine the output metrics.
yes, that was my point, that I don't see nr_release being
used for anything
Acked-by: Jiri Olsa <jolsa@redhat.com>
thanks,
jirka
>
> Thanks,
> Leo
>
> [1] https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/tools/perf/builtin-lock.c#n641
> [2] https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/tools/perf/builtin-lock.c#n625
>
^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH v2 1/2] perf lock: Correct field name "flags"
2020-10-21 0:39 [PATCH v2 1/2] perf lock: Correct field name "flags" Leo Yan
2020-10-21 0:39 ` [PATCH v2 2/2] perf lock: Don't free "lock_seq_stat" if read_count isn't zero Leo Yan
@ 2020-11-02 10:24 ` Leo Yan
2020-11-02 16:42 ` Jiri Olsa
2 siblings, 0 replies; 8+ messages in thread
From: Leo Yan @ 2020-11-02 10:24 UTC (permalink / raw)
To: Arnaldo Carvalho de Melo, Peter Zijlstra, Ingo Molnar,
Mark Rutland, Alexander Shishkin, Jiri Olsa, Namhyung Kim,
Hitoshi Mitake, Frederic Weisbecker, linux-kernel
Hi Jiri,
On Wed, Oct 21, 2020 at 08:39:47AM +0800, Leo Yan wrote:
> The tracepoint "lock:lock_acquire" contains field "flags" but not
> "flag". Current code wrongly retrieves value from field "flag" and it
> always gets zero for the value, thus "perf lock" doesn't report the
> correct result.
>
> This patch replaces the field name "flag" with "flags", so can read out
> the correct flags for locking.
Could you take a look for the two fixings in this patch set (v2)?
Thanks,
Leo
> Fixes: 746f16ec6ae3 ("perf lock: Use perf_evsel__intval and perf_session__set_tracepoints_handlers")
> Signed-off-by: Leo Yan <leo.yan@linaro.org>
> ---
> tools/perf/builtin-lock.c | 2 +-
> 1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/tools/perf/builtin-lock.c b/tools/perf/builtin-lock.c
> index f0a1dbacb46c..5cecc1ad78e1 100644
> --- a/tools/perf/builtin-lock.c
> +++ b/tools/perf/builtin-lock.c
> @@ -406,7 +406,7 @@ static int report_lock_acquire_event(struct evsel *evsel,
> struct lock_seq_stat *seq;
> const char *name = evsel__strval(evsel, sample, "name");
> u64 tmp = evsel__intval(evsel, sample, "lockdep_addr");
> - int flag = evsel__intval(evsel, sample, "flag");
> + int flag = evsel__intval(evsel, sample, "flags");
>
> memcpy(&addr, &tmp, sizeof(void *));
>
> --
> 2.17.1
>
^ permalink raw reply [flat|nested] 8+ messages in thread* Re: [PATCH v2 1/2] perf lock: Correct field name "flags"
2020-10-21 0:39 [PATCH v2 1/2] perf lock: Correct field name "flags" Leo Yan
2020-10-21 0:39 ` [PATCH v2 2/2] perf lock: Don't free "lock_seq_stat" if read_count isn't zero Leo Yan
2020-11-02 10:24 ` [PATCH v2 1/2] perf lock: Correct field name "flags" Leo Yan
@ 2020-11-02 16:42 ` Jiri Olsa
2020-11-03 1:52 ` Leo Yan
2 siblings, 1 reply; 8+ messages in thread
From: Jiri Olsa @ 2020-11-02 16:42 UTC (permalink / raw)
To: Leo Yan
Cc: Arnaldo Carvalho de Melo, Peter Zijlstra, Ingo Molnar,
Mark Rutland, Alexander Shishkin, Namhyung Kim, Hitoshi Mitake,
Frederic Weisbecker, linux-kernel
On Wed, Oct 21, 2020 at 08:39:47AM +0800, Leo Yan wrote:
> The tracepoint "lock:lock_acquire" contains field "flags" but not
> "flag". Current code wrongly retrieves value from field "flag" and it
> always gets zero for the value, thus "perf lock" doesn't report the
> correct result.
>
> This patch replaces the field name "flag" with "flags", so can read out
> the correct flags for locking.
>
> Fixes: 746f16ec6ae3 ("perf lock: Use perf_evsel__intval and perf_session__set_tracepoints_handlers")
Acked-by: Jiri Olsa <jolsa@redhat.com>
btw it seems the issue was there event before that commit:
acquire_event.flag = (int)raw_field_value(event, "flag", data);
thanks,
jirka
> Signed-off-by: Leo Yan <leo.yan@linaro.org>
> ---
> tools/perf/builtin-lock.c | 2 +-
> 1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/tools/perf/builtin-lock.c b/tools/perf/builtin-lock.c
> index f0a1dbacb46c..5cecc1ad78e1 100644
> --- a/tools/perf/builtin-lock.c
> +++ b/tools/perf/builtin-lock.c
> @@ -406,7 +406,7 @@ static int report_lock_acquire_event(struct evsel *evsel,
> struct lock_seq_stat *seq;
> const char *name = evsel__strval(evsel, sample, "name");
> u64 tmp = evsel__intval(evsel, sample, "lockdep_addr");
> - int flag = evsel__intval(evsel, sample, "flag");
> + int flag = evsel__intval(evsel, sample, "flags");
>
> memcpy(&addr, &tmp, sizeof(void *));
>
> --
> 2.17.1
>
^ permalink raw reply [flat|nested] 8+ messages in thread* Re: [PATCH v2 1/2] perf lock: Correct field name "flags"
2020-11-02 16:42 ` Jiri Olsa
@ 2020-11-03 1:52 ` Leo Yan
0 siblings, 0 replies; 8+ messages in thread
From: Leo Yan @ 2020-11-03 1:52 UTC (permalink / raw)
To: Jiri Olsa
Cc: Arnaldo Carvalho de Melo, Peter Zijlstra, Ingo Molnar,
Mark Rutland, Alexander Shishkin, Namhyung Kim, Hitoshi Mitake,
Frederic Weisbecker, linux-kernel
On Mon, Nov 02, 2020 at 05:42:13PM +0100, Jiri Olsa wrote:
> On Wed, Oct 21, 2020 at 08:39:47AM +0800, Leo Yan wrote:
> > The tracepoint "lock:lock_acquire" contains field "flags" but not
> > "flag". Current code wrongly retrieves value from field "flag" and it
> > always gets zero for the value, thus "perf lock" doesn't report the
> > correct result.
> >
> > This patch replaces the field name "flag" with "flags", so can read out
> > the correct flags for locking.
> >
> > Fixes: 746f16ec6ae3 ("perf lock: Use perf_evsel__intval and perf_session__set_tracepoints_handlers")
>
> Acked-by: Jiri Olsa <jolsa@redhat.com>
>
> btw it seems the issue was there event before that commit:
> acquire_event.flag = (int)raw_field_value(event, "flag", data);
Thanks for pointing out this, I will change the fix tag as:
Fixes: e4cef1f65061 ("perf lock: Fix state machine to recognize lock sequence")
Leo
> > Signed-off-by: Leo Yan <leo.yan@linaro.org>
> > ---
> > tools/perf/builtin-lock.c | 2 +-
> > 1 file changed, 1 insertion(+), 1 deletion(-)
> >
> > diff --git a/tools/perf/builtin-lock.c b/tools/perf/builtin-lock.c
> > index f0a1dbacb46c..5cecc1ad78e1 100644
> > --- a/tools/perf/builtin-lock.c
> > +++ b/tools/perf/builtin-lock.c
> > @@ -406,7 +406,7 @@ static int report_lock_acquire_event(struct evsel *evsel,
> > struct lock_seq_stat *seq;
> > const char *name = evsel__strval(evsel, sample, "name");
> > u64 tmp = evsel__intval(evsel, sample, "lockdep_addr");
> > - int flag = evsel__intval(evsel, sample, "flag");
> > + int flag = evsel__intval(evsel, sample, "flags");
> >
> > memcpy(&addr, &tmp, sizeof(void *));
> >
> > --
> > 2.17.1
> >
>
^ permalink raw reply [flat|nested] 8+ messages in thread