On Thu, Jan 30, 2025 at 02:31:52PM +0100, Eelco Chaudron wrote:
>
>
> 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.
>
Ack
> > + 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.
>
Right! Thanks.
> > +#
> > +# 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
