so 5. 10. 2024 v 19:21 odesÃlatel furkanonder
<furkanon...@protonmail.com> napsal:
>
> The enhancements made to timerlat_load.py focus on improving error
> handling, readability, and overall user experience. These changes aim to
> make the script more robust and easier to maintain while providing clearer
> feedback to users. Key modifications include:
>
> Type Declaration in Argument ParsingType Declaration in Argument Parsing
> Added type declarations for command-line arguments in the argument parser.
> This removes the need for manual type checks later in the code, improving
> clarity and safety.
>
> String Formatting
> Replaced string concatenation with an f-string to enhancing readability
> and conciseness.
>
> Enhanced Exception Handling and Consistent Error Reporting
> Specific exceptions are now caught and printed with clearer messages,
> providing context for errors when opening file descriptors. Added
> exception handling for the data file descriptor opening to ensure
> uniformity across the script.
>
> Before:
> $ sudo python timerlat_load.py 122
> Error setting affinity
> After:
> $ sudo python timerlat_load.py 122
> Error setting affinity: [Errno 22] Invalid argument
>
> Before:
> $ sudo python timerlat_load.py 1 -p 950
> Error setting priority
> After:
> $ sudo python timerlat_load.py 1 -p 950
> Error setting priority: [Errno 22] Invalid argument
>
> Before:
> $ python timerlat_load.py 1
> Error opening timerlat fd, did you run timerlat -U?
> After:
> $ python timerlat_load.py 1
> Permission denied. Please check your access rights.
>
> Changes for the read Infinite Loop
> The original generic exception clause has been replaced with more
> specific exception types to provide clearer feedback on errors.
>
> Signed-off-by: Furkan Onder <furkanon...@protonmail.com>
> ---
> tools/tracing/rtla/sample/timerlat_load.py | 56
> +++++++++++++++++++++++--------------------
> 1 file changed, 30 insertions(+), 26 deletions(-)
>
> diff --git a/tools/tracing/rtla/sample/timerlat_load.py
> b/tools/tracing/rtla/sample/timerlat_load.py
> index 8cc5eb2d2e69..a819c3588073 100644
> --- a/tools/tracing/rtla/sample/timerlat_load.py
> +++ b/tools/tracing/rtla/sample/timerlat_load.py
> @@ -25,50 +25,54 @@ import sys
> import os
>
> parser = argparse.ArgumentParser(description='user-space timerlat thread in
> Python')
> -parser.add_argument("cpu", help='CPU to run timerlat thread')
> -parser.add_argument("-p", "--prio", help='FIFO priority')
> -
> +parser.add_argument("cpu", type=int, help='CPU to run timerlat thread')
> +parser.add_argument("-p", "--prio", type=int, help='FIFO priority')
> args = parser.parse_args()
>
> try:
> - affinity_mask = { int(args.cpu) }
> -except:
> - print("Invalid cpu: " + args.cpu)
> - exit(1)
> -
> -try:
> - os.sched_setaffinity(0, affinity_mask);
> -except:
> - print("Error setting affinity")
> - exit(1)
> + affinity_mask = {args.cpu}
> + os.sched_setaffinity(0, affinity_mask)
> +except Exception as e:
> + print(f"Error setting affinity: {e}")
> + sys.exit(1)
>
> -if (args.prio):
> +if args.prio:
> try:
> - param = os.sched_param(int(args.prio))
> + param = os.sched_param(args.prio)
> os.sched_setscheduler(0, os.SCHED_FIFO, param)
> - except:
> - print("Error setting priority")
> - exit(1)
> + except Exception as e:
> + print(f"Error setting priority: {e}")
> + sys.exit(1)
>
> try:
> - timerlat_path = "/sys/kernel/tracing/osnoise/per_cpu/cpu" + args.cpu +
> "/timerlat_fd"
> + timerlat_path =
> f"/sys/kernel/tracing/osnoise/per_cpu/cpu{args.cpu}/timerlat_fd"
> timerlat_fd = open(timerlat_path, 'r')
> -except:
> +except PermissionError:
> + print("Permission denied. Please check your access rights.")
> + sys.exit(1)
> +except OSError:
> print("Error opening timerlat fd, did you run timerlat -U?")
> - exit(1)
> + sys.exit(1)
>
> try:
> - data_fd = open("/dev/full", 'r');
> -except:
> - print("Error opening data fd")
> + data_fd = open("/dev/full", 'r')
> +except Exception as e:
> + print(f"Error opening data fd: {e}")
> + sys.exit(1)
>
> while True:
> try:
> timerlat_fd.read(1)
> - data_fd.read(20*1024*1024)
> - except:
> + data_fd.read(20 * 1024 * 1024)
> + except KeyboardInterrupt:
> print("Leaving")
> break
> + except IOError as e:
> + print(f"I/O error occurred: {e}")
> + break
> + except Exception as e:
> + print(f"Unexpected error: {e}")
> + break
>
> timerlat_fd.close()
> data_fd.close()
>
> --
> 2.46.2
>
Could you please resend your fix with one patch per logical change?
That is one for the code style changes (string formatting and other
minor changes like the removal of redundant brackers), one for better
exception handling, and one for better argument parsing. This will
keep the part which improves code style separate from the parts which
do actual functional changes to the code, which will make future
references to the patches clearer. It will also make the commit
messages shorter and easier to read.
Other than that, LGTM. Note that due to a bug in fb9e90a67ee9
("rtla/timerlat: Make user-space threads the default"), rtla has to be
run with "rtla timerlat top -U -k ..." to work properly with
user-managed userspace workload threads; if -k is not specified, rtla
defaults to -u and will create the workload thread itself, leading to
timerlat_load.py erroring out on timerlat fd open with EBUSY. (I think
this was already mentioned somewhere?)
Tomas
