diff mbox series

tools/rtla: Enhance Error Handling and Readability for timerlat

Message ID HAxOlk1o3vhrAUlyVMtrSLGP39PFS1TLfGT9C9pB6cXgQpGB_fiNeGSuWS_4-4zeMxunluuBnkc4Y-k_c0qrlstVpYQvgfkJN2p_VnfMO-k=@protonmail.com (mailing list archive)
State Changes Requested
Headers show
Series tools/rtla: Enhance Error Handling and Readability for timerlat | expand

Commit Message

furkanonder Oct. 5, 2024, 5:20 p.m. UTC 
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 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 <furkanonder@protonmail.com>
---
 tools/tracing/rtla/sample/timerlat_load.py | 56 +++++++++++++++++++++++--------------------
 1 file changed, 30 insertions(+), 26 deletions(-)


--
2.46.2

Comments

Steven Rostedt Oct. 11, 2024, 12:29 a.m. UTC | #1 
Tomas,

Can you review this patch?

Thanks,

-- Steve


On Sat, 05 Oct 2024 17:20:37 +0000
furkanonder <furkanonder@protonmail.com> wrote:

> 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 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 <furkanonder@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
Tomas Glozar Oct. 11, 2024, 1:35 p.m. UTC | #2 
pá 11. 10. 2024 v 2:34 odesílatel Steven Rostedt <rostedt@goodmis.org> napsal:
>
>
> Tomas,
>
> Can you review this patch?
>
> Thanks,
>
> -- Steve
>

Yeah, I will review it.

Tomas
Tomas Glozar Oct. 14, 2024, 8:51 a.m. UTC | #3 
so 5. 10. 2024 v 19:21 odesílatel furkanonder
<furkanonder@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 <furkanonder@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
diff mbox series

Patch

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