linux-kernel - Re: [PATCH] tools:perf:scripts:python:mem-phys-addr fix performance

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [day] [month] [year] [list]

Date:   Tue, 10 Jan 2023 00:44:20 +0400
From:   Alexander Pantyukhin <apantykhin@...il.com>
To:     mark.rutland@....com
Cc:     peterz@...radead.org, mingo@...hat.com, acme@...nel.org,
        alexander.shishkin@...ux.intel.com, jolsa@...nel.org,
        namhyung@...nel.org, linux-perf-users@...r.kernel.org,
        linux-kernel@...r.kernel.org
Subject: Re: [PATCH] tools:perf:scripts:python:mem-phys-addr fix performance

Hi!
There are some things which I would like to discuss there.

1) I found that if there is for example the following system_ram:
system_ram = [int('00000000', 16) , int('0000ffff', 16)]

and the func

def is_system_ram(phys_addr):
    #/proc/iomem is sorted
    position = bisect.bisect(system_ram, phys_addr)
    if position % 2 == 0:
    return False
    return True

The value:
is_system_ram(int('0000ffff', 16)) is False.
It seems to be a mistake. This value is the end of the range and it
should be a True.
(There is another check on the 88 line: if int(m[0], 16) <= phys_addr
<= int(m[1], 16)
where both ranges are included in the check.)

2) The script reads the whole file when parse_iomem()
and uses this information for pmem and system_ram. What if to store
whole info about addresses
for all memory types?
In This case it's possible to store the data structure like the
following example:

/proc/iomem
00000000-0000ffff : reserved
00010000-0009f3ff : System RAM

Result: [(00000000, 'reserved'), (0000ffff, 'reserved'), (00010000,
'System RAM'), (0009f3ff, 'System RAM')....]

This case the searching would be O(log(n)) for all types not just for
"System RAM" and "Persistent Memory".

But the behavior of the script would be changed: the current version
re-read the file and scan the fresh data
when neither "System RAM" nor "Persistent Memory". But the suggested
fix requires to call parse_iomem() when
fresh data is required.

If these changes are approved, I would like to fix it.

Best, Alex.

пн, 9 янв. 2023 г. в 13:33, Alexander Pantyukhin <apantykhin@...il.com>:
>
> Avoid double strip() calls for getting memory type.
>
> Signed-off-by: Alexander Pantyukhin <apantykhin@...il.com>
> ---
>  tools/perf/scripts/python/mem-phys-addr.py | 10 ++++++----
>  1 file changed, 6 insertions(+), 4 deletions(-)
>
> diff --git a/tools/perf/scripts/python/mem-phys-addr.py b/tools/perf/scripts/python/mem-phys-addr.py
> index 1f332e72b9b0..f267d8382eda 100644
> --- a/tools/perf/scripts/python/mem-phys-addr.py
> +++ b/tools/perf/scripts/python/mem-phys-addr.py
> @@ -8,7 +8,6 @@ from __future__ import print_function
>
>  import os
>  import sys
> -import struct
>  import re
>  import bisect
>  import collections
> @@ -30,12 +29,14 @@ event_name = None
>  def parse_iomem():
>         global f
>         f = open('/proc/iomem', 'r')
> -       for i, j in enumerate(f):
> +       for _, j in enumerate(f):
>                 m = re.split('-|:',j,2)
> -               if m[2].strip() == 'System RAM':
> +               memory_type = m[2].strip()
> +
> +               if memory_type == 'System RAM':
>                         system_ram.append(int(m[0], 16))
>                         system_ram.append(int(m[1], 16))
> -               if m[2].strip() == 'Persistent Memory':
> +               elif memory_type == 'Persistent Memory':
>                         pmem.append(int(m[0], 16))
>                         pmem.append(int(m[1], 16))
>
> @@ -75,6 +76,7 @@ def is_persistent_mem(phys_addr):
>  def find_memory_type(phys_addr):
>         if phys_addr == 0:
>                 return "N/A"
> +
>         if is_system_ram(phys_addr):
>                 return "System RAM"
>
> --
> 2.25.1
>