Message-ID: <aCYOEwgBLWyK7g3q@x1>
Date: Thu, 15 May 2025 12:53: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 04:11:55PM -0300, Arnaldo Carvalho de Melo wrote:
> 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>

I was expecting some reaction from you or Thomas, but since I got a ping
from Thomas for this not being processed, I'll process it according to
my assessment of this thread.

Thanks,

- Arnaldo


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