Re: [PATCH] perf/core: Fix small negative period being ignored

[Adrian Hunter]

On 16/01/24 10:39, Luo Gengkun wrote:
> In perf_adjust_period, we will first calculate period, and then use
> this period to calculate delta. However, when delta is less than 0,
> there will be a deviation compared to when delta is greater than or
> equal to 0. For example, when delta is in the range of [-14,-1], the
> range of delta = delta + 7 is between [-7,6], so the final value of
> delta/8 is 0. Therefore, the impact of -1 and -2 will be ignored.
> This is unacceptable when the target period is very short, because
> we will lose a lot of samples.
> 
> Here are some tests and analyzes:
> before:
>   # perf record -e cs -F 1000  ./a.out
>   [ perf record: Woken up 1 times to write data ]
>   [ perf record: Captured and wrote 0.022 MB perf.data (518 samples) ]
> 
>   # perf script
>   ...
>   a.out     396   257.956048:         23 cs:  ffffffff81f4eeec schedul>
>   a.out     396   257.957891:         23 cs:  ffffffff81f4eeec schedul>
>   a.out     396   257.959730:         23 cs:  ffffffff81f4eeec schedul>
>   a.out     396   257.961545:         23 cs:  ffffffff81f4eeec schedul>
>   a.out     396   257.963355:         23 cs:  ffffffff81f4eeec schedul>
>   a.out     396   257.965163:         23 cs:  ffffffff81f4eeec schedul>
>   a.out     396   257.966973:         23 cs:  ffffffff81f4eeec schedul>
>   a.out     396   257.968785:         23 cs:  ffffffff81f4eeec schedul>
>   a.out     396   257.970593:         23 cs:  ffffffff81f4eeec schedul>
>   ...
> 
> after:
>   # perf record -e cs -F 1000  ./a.out
>   [ perf record: Woken up 1 times to write data ]
>   [ perf record: Captured and wrote 0.058 MB perf.data (1466 samples) ]
> 
>   # perf script
>   ...
>   a.out     395    59.338813:         11 cs:  ffffffff81f4eeec schedul>
>   a.out     395    59.339707:         12 cs:  ffffffff81f4eeec schedul>
>   a.out     395    59.340682:         13 cs:  ffffffff81f4eeec schedul>
>   a.out     395    59.341751:         13 cs:  ffffffff81f4eeec schedul>
>   a.out     395    59.342799:         12 cs:  ffffffff81f4eeec schedul>
>   a.out     395    59.343765:         11 cs:  ffffffff81f4eeec schedul>
>   a.out     395    59.344651:         11 cs:  ffffffff81f4eeec schedul>
>   a.out     395    59.345539:         12 cs:  ffffffff81f4eeec schedul>
>   a.out     395    59.346502:         13 cs:  ffffffff81f4eeec schedul>
>   ...
> 
> test.c
> 
> int main() {
>         for (int i = 0; i < 20000; i++)
>                 usleep(10);
> 
>         return 0;
> }
> 
>   # time ./a.out
>   real    0m1.583s
>   user    0m0.040s
>   sys     0m0.298s
> 
> The above results were tested on x86-64 qemu with KVM enabled using
> test.c as test program. Ideally, we should have around 1500 samples,
> but the previous algorithm had only about 500, whereas the modified
> algorithm now has about 1400. Further more, the new version shows 1
> sample per 0.001s, while the previous one is 1 sample per 0.002s.This
> indicates that the new algorithm is more sensitive to small negative
> values compared to old algorithm.
> 
> Fixes: bd2b5b12849a ("perf_counter: More aggressive frequency adjustment")
> Signed-off-by: Luo Gengkun <luogengkun2@xxxxxxxxxx>

It seems better, and the maths makes sense, so:

Reviewed-by: Adrian Hunter <adrian.hunter@xxxxxxxxx>


But the test case still seems to give unexpected results. Usually:

  # time taskset --cpu 1 ./test
  real    0m 1.25s
  user    0m 0.03s
  sys     0m 0.00
  # taskset --cpu 0 perf record -F 1000 -e cs -- taskset --cpu 1 ./test
  [ perf record: Woken up 1 times to write data ]
  [ perf record: Captured and wrote 0.051 MB perf.data (1290 samples) ]

But occasionally:

  # taskset --cpu 0 perf record -F 1000 -e cs -- taskset --cpu 1 ./test
  [ perf record: Woken up 1 times to write data ]
  [ perf record: Captured and wrote 0.010 MB perf.data (204 samples) ]
  # perf script
  ...
  test   865   265.377846:         16 cs:  ffffffff832e927b schedule+0x2b
  test   865   265.378900:         15 cs:  ffffffff832e927b schedule+0x2b
  test   865   265.379845:         14 cs:  ffffffff832e927b schedule+0x2b
  test   865   265.380770:         14 cs:  ffffffff832e927b schedule+0x2b
  test   865   265.381647:         15 cs:  ffffffff832e927b schedule+0x2b
  test   865   265.382638:         16 cs:  ffffffff832e927b schedule+0x2b
  test   865   265.383647:         16 cs:  ffffffff832e927b schedule+0x2b
  test   865   265.384704:         15 cs:  ffffffff832e927b schedule+0x2b
  test   865   265.385649:         14 cs:  ffffffff832e927b schedule+0x2b
  test   865   265.386578:        152 cs:  ffffffff832e927b schedule+0x2b
  test   865   265.396383:        154 cs:  ffffffff832e927b schedule+0x2b
  test   865   265.406183:        154 cs:  ffffffff832e927b schedule+0x2b
  test   865   265.415839:        154 cs:  ffffffff832e927b schedule+0x2b
  test   865   265.425445:        154 cs:  ffffffff832e927b schedule+0x2b
  test   865   265.435052:        154 cs:  ffffffff832e927b schedule+0x2b
  test   865   265.444708:        154 cs:  ffffffff832e927b schedule+0x2b
  test   865   265.454314:        154 cs:  ffffffff832e927b schedule+0x2b
  test   865   265.463970:        154 cs:  ffffffff832e927b schedule+0x2b
  test   865   265.473577:        154 cs:  ffffffff832e927b schedule+0x2b
  ...



> ---
>  kernel/events/core.c | 6 +++++-
>  1 file changed, 5 insertions(+), 1 deletion(-)
> 
> diff --git a/kernel/events/core.c b/kernel/events/core.c
> index 683dc086ef10..cad50d3439f1 100644
> --- a/kernel/events/core.c
> +++ b/kernel/events/core.c
> @@ -4078,7 +4078,11 @@ static void perf_adjust_period(struct perf_event *event, u64 nsec, u64 count, bo
>  	period = perf_calculate_period(event, nsec, count);
>  
>  	delta = (s64)(period - hwc->sample_period);
> -	delta = (delta + 7) / 8; /* low pass filter */
> +	if (delta >= 0)
> +		delta += 7;
> +	else
> +		delta -= 7;
> +	delta /= 8; /* low pass filter */
>  
>  	sample_period = hwc->sample_period + delta;
>