On 17 Jan 2025, at 15:25, Adrian Moreno wrote:
> The upcall_cost.py script has a useful port mapping cache that can be
> very useful for other scripts.
>
> Move it into a library file (usdt_lib.py). While we're at it, refactor
> it into a class since setting attributes to functions and having it
> return different types depending on input is not very clean.
>
> This patch should not introduce any functional change.
>
> Signed-off-by: Adrian Moreno <[email protected]>
In general, this looks good to me. Some small comments below.
//Eelco
> ---
> utilities/automake.mk | 7 ++-
> utilities/usdt-scripts/upcall_cost.py | 72 ++++-----------------------
> utilities/usdt-scripts/usdt_lib.py | 70 ++++++++++++++++++++++++++
> 3 files changed, 86 insertions(+), 63 deletions(-)
> create mode 100644 utilities/usdt-scripts/usdt_lib.py
>
> diff --git a/utilities/automake.mk b/utilities/automake.mk
> index 22260b254..fcd353109 100644
> --- a/utilities/automake.mk
> +++ b/utilities/automake.mk
> @@ -28,7 +28,8 @@ usdt_SCRIPTS += \
> utilities/usdt-scripts/kernel_delay.rst \
> utilities/usdt-scripts/reval_monitor.py \
> utilities/usdt-scripts/upcall_cost.py \
> - utilities/usdt-scripts/upcall_monitor.py
> + utilities/usdt-scripts/upcall_monitor.py \
> + utilities/usdt-scripts/usdt_lib.py
>
> completion_SCRIPTS += \
> utilities/ovs-appctl-bashcomp.bash \
> @@ -78,7 +79,9 @@ EXTRA_DIST += \
> utilities/usdt-scripts/kernel_delay.rst \
> utilities/usdt-scripts/reval_monitor.py \
> utilities/usdt-scripts/upcall_cost.py \
> - utilities/usdt-scripts/upcall_monitor.py
> + utilities/usdt-scripts/upcall_monitor.py \
> + utilities/usdt-scripts/usdt_lib.py
> +
> MAN_ROOTS += \
> utilities/ovs-testcontroller.8.in \
> utilities/ovs-dpctl.8.in \
> diff --git a/utilities/usdt-scripts/upcall_cost.py
> b/utilities/usdt-scripts/upcall_cost.py
> index 765669585..47a1e30a6 100755
> --- a/utilities/usdt-scripts/upcall_cost.py
> +++ b/utilities/usdt-scripts/upcall_cost.py
> @@ -58,12 +58,13 @@ from strenum import StrEnum
> from text_histogram3 import histogram
> from time import process_time
>
> +from usdt_lib import DpPortMapping
> +
> import argparse
> import ast
> import psutil
> import re
> import struct
> -import subprocess
> import sys
> import time
>
> @@ -353,7 +354,7 @@ class DpUpcall(Event):
> pkt_frag_len):
> super(DpUpcall, self).__init__(ts, pid, comm, cpu,
> EventType.DP_UPCALL)
> self.dpif_name = dpif_name
> - self.dp_port = get_dp_mapping(dpif_name, port)
> + self.dp_port = dp_map.get(dpif_name, port)
> if self.dp_port is None:
> #
> # As we only identify interfaces at startup, new interfaces could
> @@ -446,13 +447,8 @@ class RecvUpcall(Event):
> self.pkt_len)
>
> def get_system_dp_port(dpif_name):
> - dp_map = get_dp_mapping(dpif_name, "ovs-system", return_map=True)
> - if dpif_name not in dp_map:
> - return None
> - try:
> - return dp_map[dpif_name]["ovs-system"]
> - except KeyError:
> - return None
> + dp_map = dp_map.get_map()
This looks confusing, as dp_map local is the same as the global variable. Maybe
rename it.
> + return dp_map.get(dpif_name, {}).get("ovs-system", None)
>
> def decode_nlm(msg, indent=4, dump=True):
> bytes_left = len(msg)
> @@ -621,54 +617,6 @@ class OpFlowExecute(Event):
> return event
>
>
> -#
> -# get_dp_mapping()
> -#
> -def get_dp_mapping(dp, port, return_map=False, dp_map=None):
> - if options.unit_test:
> - return port
> -
> - if dp_map is not None:
> - get_dp_mapping.dp_port_map_cache = dp_map
> -
> - #
> - # Build a cache, so we do not have to execue the ovs command each time.
> - #
> - if not hasattr(get_dp_mapping, "dp_port_map_cache"):
> - try:
> - output = subprocess.check_output(['ovs-appctl', 'dpctl/show'],
> - encoding='utf8').split("\n")
> - except subprocess.CalledProcessError:
> - output = ""
> - pass
> -
> - current_dp = None
> - get_dp_mapping.dp_port_map_cache = {}
> -
> - for line in output:
> - match = re.match("^system@(.*):$", line)
> - if match is not None:
> - current_dp = match.group(1)
> -
> - match = re.match("^ port ([0-9]+): ([^ /]*)", line)
> - if match is not None and current_dp is not None:
> - try:
> - get_dp_mapping.dp_port_map_cache[
> - current_dp][match.group(2)] = int(match.group(1))
> - except KeyError:
> - get_dp_mapping.dp_port_map_cache[current_dp] = \
> - {match.group(2): int(match.group(1))}
> -
> - if return_map:
> - return get_dp_mapping.dp_port_map_cache
> -
> - if dp not in get_dp_mapping.dp_port_map_cache or \
> - port not in get_dp_mapping.dp_port_map_cache[dp]:
> - return None
> -
> - return get_dp_mapping.dp_port_map_cache[dp][port]
> -
> -
> #
> # event_to_dict()
> #
> @@ -1429,6 +1377,9 @@ def main():
> global events_received
> global event_count
> global export_file
> + global dp_map
> +
> + dp_map = DpPortMapping()
>
> #
> # Argument parsing
> @@ -1531,9 +1482,9 @@ def main():
> event_count = {'total': {}, 'valid': {}, 'miss': {}}
> if options.read_events is None:
> #
> - # Call get_dp_mapping() to prepare the cache
> + # Prepare the datapath port mapping cache
> #
> - dp_port_map = get_dp_mapping("ovs-system", "eth0", return_map=True)
> + dp_port_map = dp_map.get_map()
> if export_file is not None:
> export_file.write("dp_port_map = {}\n".format(dp_port_map))
>
> @@ -1643,8 +1594,7 @@ def main():
> if entry.startswith('dp_port_map = {'):
> if not dp_port_mapping_valid:
> dp_port_mapping_valid = True
> - get_dp_mapping("", "",
> -
> dp_map=ast.literal_eval(entry[14:]))
> + dp_map.set_map(ast.literal_eval(entry[14:]))
> elif (entry.startswith('event = {') and
> dp_port_mapping_valid):
> event = ast.literal_eval(entry[8:])
> diff --git a/utilities/usdt-scripts/usdt_lib.py
> b/utilities/usdt-scripts/usdt_lib.py
> new file mode 100644
> index 000000000..da3fab2bf
> --- /dev/null
> +++ b/utilities/usdt-scripts/usdt_lib.py
> @@ -0,0 +1,70 @@
> +# Copyright (c) 2025 Red Hat, Inc.
As some of this code is copied from the 2021 licensed file, I think, this
should also be 2021.
> +#
> +# Licensed under the Apache License, Version 2.0 (the "License");
> +# you may not use this file except in compliance with the License.
> +# You may obtain a copy of the License at:
> +#
> +# http://www.apache.org/licenses/LICENSE-2.0
> +#
> +# Unless required by applicable law or agreed to in writing, software
> +# distributed under the License is distributed on an "AS IS" BASIS,
> +# WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
> +# See the License for the specific language governing permissions and
> +# limitations under the License.
> +
> +import re
> +import subprocess
> +
> +
> +class DpPortMapping:
> + """Class used to retrieve and cache datapath port numbers to port
> names."""
> +
> + def __init__(self):
> + self.cache_map = None
> +
> + def get_map(self):
> + """Get the cache map."""
> + if not self.cache_map:
> + self._get_mapping()
> +
> + return self.cache_map
> +
> + def set_map(self, cache_map):
> + """Override the internal cache map."""
> + self.cache_map = cache_map
> +
> + def get(self, dp, port_no):
> + """Get the portname from a port number."""
> + if self.cache_map is None:
> + self._get_mapping()
> +
> + return self.cache_map.get(dp, {}).get(port_no, None)
> +
> + def _get_mapping(self):
> + """Get the datapath port mapping from the running OVS."""
> + try:
> + output = subprocess.check_output(
> + ["ovs-appctl", "dpctl/show"], encoding="utf8"
> + ).split("\n")
> + except subprocess.CalledProcessError:
> + output = ""
> + return
> +
> + current_dp = None
> + self.cache_map = {}
> +
> + for line in output:
> + match = re.match("^system@(.*):$", line)
> + if match:
> + current_dp = match.group(1)
> +
> + match = re.match("^ port ([0-9]+): ([^ /]*)", line)
> + if match and current_dp:
> + try:
> + self.cache_map[current_dp][match.group(2)] = int(
> + match.group(1)
> + )
> + except KeyError:
> + self.cache_map[current_dp] = {
> + match.group(2): int(match.group(1))
> + }
> --
> 2.48.1
>
> _______________________________________________
> dev mailing list
> [email protected]
> https://mail.openvswitch.org/mailman/listinfo/ovs-dev
_______________________________________________
dev mailing list
[email protected]
https://mail.openvswitch.org/mailman/listinfo/ovs-dev
