Message-ID: <aBUY-zaI6BxRvWWS@x1>
Date: Fri, 2 May 2025 16:11:55 -0300
From: Arnaldo Carvalho de Melo <acme@...nel.org>
To: Chun-Tse Shao <ctshao@...gle.com>
Cc: Thomas Richter <tmricht@...ux.ibm.com>, linux-kernel@...r.kernel.org,
	linux-s390@...r.kernel.org, linux-perf-users@...r.kernel.org,
	namhyung@...nel.org, agordeev@...ux.ibm.com, gor@...ux.ibm.com,
	sumanthk@...ux.ibm.com, hca@...ux.ibm.com, irogers@...gle.com
Subject: Re: [PATCH V4] perf test: Allow tolerance for leader sampling test

On Fri, May 02, 2025 at 11:21:07AM -0700, Chun-Tse Shao wrote:
> Hi Arnaldo,
> 
> I submitted the patch v1 and Thomas helped me to modify and submit v2
> and v3 while I was OOO. In this case I am not sure which one should be
> the author, maybe just keep it as Thomas.

>From the tags provided, I think it would be best to list you as the
author and Thomas a a Co-developer, like mentioned in:

Documentation/process/submitting-patches.rst

Co-developed-by: states that the patch was co-created by multiple developers;
it is used to give attribution to co-authors (in addition to the author
attributed by the From: tag) when several people work on a single patch.  Since
Co-developed-by: denotes authorship, every Co-developed-by: must be immediately
followed by a Signed-off-by: of the associated co-author.  Standard sign-off
procedure applies, i.e. the ordering of Signed-off-by: tags should reflect the
chronological history of the patch insofar as possible, regardless of whether
the author is attributed via From: or Co-developed-by:.  Notably, the last
Signed-off-by: must always be that of the developer submitting the patch.

Note, the From: tag is optional when the From: author is also the person (and
email) listed in the From: line of the email header.

Example of a patch submitted by the From: author::

        <changelog>

        Co-developed-by: First Co-Author <first@...uthor.example.org>
        Signed-off-by: First Co-Author <first@...uthor.example.org>
        Co-developed-by: Second Co-Author <second@...uthor.example.org>
        Signed-off-by: Second Co-Author <second@...uthor.example.org>
        Signed-off-by: From Author <from@...hor.example.org>

Example of a patch submitted by a Co-developed-by: author::

        From: From Author <from@...hor.example.org>

        <changelog>

        Co-developed-by: Random Co-Author <random@...uthor.example.org>
        Signed-off-by: Random Co-Author <random@...uthor.example.org>
        Signed-off-by: From Author <from@...hor.example.org>
        Co-developed-by: Submitting Co-Author <sub@...uthor.example.org>
        Signed-off-by: Submitting Co-Author <sub@...uthor.example.org>
 
> Thanks,
> CT
> 
> On Fri, May 2, 2025 at 9:35 AM Arnaldo Carvalho de Melo <acme@...nel.org> wrote:
> >
> > On Wed, Apr 30, 2025 at 04:06:11PM +0200, Thomas Richter wrote:
> > > V4: Update to be applied onto linux-next
> > > V3: Added check for missing samples as suggested by Chun-Tse.
> > > V2: Changed bc invocation to return 0 on success and 1 on error.
> > >
> > > There is a known issue that the leader sampling is inconsistent, since
> > > throttle only affect leader, not the slave. The detail is in [1]. To
> > > maintain test coverage, this patch sets a tolerance rate of 80% to
> > > accommodate the throttled samples and prevent test failures due to
> > > throttling.
> > >
> > > [1] lore.kernel.org/20250328182752.769662-1-ctshao@...gle.com
> > >
> > > Signed-off-by: Chun-Tse Shao <ctshao@...gle.com>
> > > Suggested-by: Ian Rogers <irogers@...gle.com>
> > > Suggested-by: Thomas Richter <tmricht@...ux.ibm.com>
> >
> > But who is the author? As-is this patch states Thomas Richter as the
> > author, but since there is also a Suggested-by and Tested-by Thomas
> > Richter, it makes me believe the author is Chun-Tse Shao, is that the
> > case?
> >
> > - Arnaldo
> >
> > > Tested-by: Thomas Richter <tmricht@...ux.ibm.com>
> > > Signed-off-by: Thomas Richter <tmricht@...ux.ibm.com>
> > > ---
> > >  tools/perf/tests/shell/record.sh | 33 ++++++++++++++++++++++++++------
> > >  1 file changed, 27 insertions(+), 6 deletions(-)
> > >
> > > diff --git a/tools/perf/tests/shell/record.sh b/tools/perf/tests/shell/record.sh
> > > index 05d91a663fda..587f62e34414 100755
> > > --- a/tools/perf/tests/shell/record.sh
> > > +++ b/tools/perf/tests/shell/record.sh
> > > @@ -240,22 +240,43 @@ test_leader_sampling() {
> > >      err=1
> > >      return
> > >    fi
> > > +  perf script -i "${perfdata}" | grep brstack > $script_output
> > > +  # Check if the two instruction counts are equal in each record.
> > > +  # However, the throttling code doesn't consider event grouping. During throttling, only the
> > > +  # leader is stopped, causing the slave's counts significantly higher. To temporarily solve this,
> > > +  # let's set the tolerance rate to 80%.
> > > +  # TODO: Revert the code for tolerance once the throttling mechanism is fixed.
> > >    index=0
> > > -  perf script -i "${perfdata}" > "${script_output}"
> > > +  valid_counts=0
> > > +  invalid_counts=0
> > > +  tolerance_rate=0.8
> > >    while IFS= read -r line
> > >    do
> > > -    # Check if the two instruction counts are equal in each record
> > >      cycles=$(echo $line | awk '{for(i=1;i<=NF;i++) if($i=="cycles:") print $(i-1)}')
> > >      if [ $(($index%2)) -ne 0 ] && [ ${cycles}x != ${prev_cycles}x ]
> > >      then
> > > -      echo "Leader sampling [Failed inconsistent cycles count]"
> > > -      err=1
> > > -      return
> > > +      invalid_counts=$(($invalid_counts+1))
> > > +    else
> > > +      valid_counts=$(($valid_counts+1))
> > >      fi
> > >      index=$(($index+1))
> > >      prev_cycles=$cycles
> > >    done < "${script_output}"
> > > -  echo "Basic leader sampling test [Success]"
> > > +  total_counts=$(bc <<< "$invalid_counts+$valid_counts")
> > > +  if (( $(bc <<< "$total_counts <= 0") ))
> > > +  then
> > > +    echo "Leader sampling [No sample generated]"
> > > +    err=1
> > > +    return
> > > +  fi
> > > +  isok=$(bc <<< "scale=2; if (($invalid_counts/$total_counts) < (1-$tolerance_rate)) { 0 } else { 1 };")
> > > +  if [ $isok -eq 1 ]
> > > +  then
> > > +     echo "Leader sampling [Failed inconsistent cycles count]"
> > > +     err=1
> > > +  else
> > > +    echo "Basic leader sampling test [Success]"
> > > +  fi
> > >  }
> > >
> > >  test_topdown_leader_sampling() {
> > > --
> > > 2.45.2

Powered by blists - more mailing lists