[PATCH v2 1/2] perf lock: Correct field name "flags"

[PATCH v2 1/2] perf lock: Correct field name "flags"

[Leo Yan]

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")
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

[PATCH v2 2/2] perf lock: Don't free "lock_seq_stat" if read_count isn't zero

[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

Re: [PATCH v2 1/2] perf lock: Correct field name "flags"

[Leo Yan]

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
> 

Re: [PATCH v2 1/2] perf lock: Correct field name "flags"

[Jiri Olsa]

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
> 

Re: [PATCH v2 2/2] perf lock: Don't free "lock_seq_stat" if read_count isn't zero

[Jiri Olsa]

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
> 

Re: [PATCH v2 1/2] perf lock: Correct field name "flags"

[Leo Yan]

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
> > 
> 

Re: [PATCH v2 2/2] perf lock: Don't free "lock_seq_stat" if read_count isn't zero

[Leo Yan]

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

Re: [PATCH v2 2/2] perf lock: Don't free "lock_seq_stat" if read_count isn't zero

[Jiri Olsa]

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
>