From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from mails.dpdk.org (mails.dpdk.org [217.70.189.124]) by inbox.dpdk.org (Postfix) with ESMTP id 73E0943382; Wed, 22 Nov 2023 14:24:14 +0100 (CET) Received: from mails.dpdk.org (localhost [127.0.0.1]) by mails.dpdk.org (Postfix) with ESMTP id 09488402E8; Wed, 22 Nov 2023 14:24:14 +0100 (CET) Received: from foss.arm.com (foss.arm.com [217.140.110.172]) by mails.dpdk.org (Postfix) with ESMTP id 4ED7D4028C for ; Wed, 22 Nov 2023 14:24:12 +0100 (CET) Received: from usa-sjc-imap-foss1.foss.arm.com (unknown [10.121.207.14]) by usa-sjc-mx-foss1.foss.arm.com (Postfix) with ESMTP id 4F5601595; Wed, 22 Nov 2023 05:24:58 -0800 (PST) Received: from [10.57.72.130] (unknown [10.57.72.130]) by usa-sjc-imap-foss1.foss.arm.com (Postfix) with ESMTPSA id BCB603F7A6; Wed, 22 Nov 2023 05:24:09 -0800 (PST) Message-ID: <6b14a66b-9b05-4ada-9a92-c3b6ead5eb8e@foss.arm.com> Date: Wed, 22 Nov 2023 13:24:07 +0000 MIME-Version: 1.0 User-Agent: Mozilla Thunderbird Subject: Re: [PATCH v7 16/21] dts: posix and linux sessions docstring update Content-Language: en-US To: =?UTF-8?Q?Juraj_Linke=C5=A1?= , thomas@monjalon.net, Honnappa.Nagarahalli@arm.com, jspewock@iol.unh.edu, probb@iol.unh.edu, paul.szczepanek@arm.com Cc: dev@dpdk.org References: <20231108125324.191005-23-juraj.linkes@pantheon.tech> <20231115130959.39420-1-juraj.linkes@pantheon.tech> <20231115130959.39420-17-juraj.linkes@pantheon.tech> From: Yoan Picchi In-Reply-To: <20231115130959.39420-17-juraj.linkes@pantheon.tech> Content-Type: text/plain; charset=UTF-8; format=flowed Content-Transfer-Encoding: 8bit X-BeenThere: dev@dpdk.org X-Mailman-Version: 2.1.29 Precedence: list List-Id: DPDK patches and discussions List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Errors-To: dev-bounces@dpdk.org On 11/15/23 13:09, Juraj Linkeš wrote: > Format according to the Google format and PEP257, with slight > deviations. > > Signed-off-by: Juraj Linkeš > --- > dts/framework/testbed_model/linux_session.py | 63 ++++++++++----- > dts/framework/testbed_model/posix_session.py | 81 +++++++++++++++++--- > 2 files changed, 113 insertions(+), 31 deletions(-) > > diff --git a/dts/framework/testbed_model/linux_session.py b/dts/framework/testbed_model/linux_session.py > index f472bb8f0f..279954ff63 100644 > --- a/dts/framework/testbed_model/linux_session.py > +++ b/dts/framework/testbed_model/linux_session.py > @@ -2,6 +2,13 @@ > # Copyright(c) 2023 PANTHEON.tech s.r.o. > # Copyright(c) 2023 University of New Hampshire > > +"""Linux OS translator. > + > +Translate OS-unaware calls into Linux calls/utilities. Most of Linux distributions are mostly > +compliant with POSIX standards, so this module only implements the parts that aren't. > +This intermediate module implements the common parts of mostly POSIX compliant distributions. > +""" > + > import json > from ipaddress import IPv4Interface, IPv6Interface > from typing import TypedDict, Union > @@ -17,43 +24,51 @@ > > > class LshwConfigurationOutput(TypedDict): > + """The relevant parts of ``lshw``'s ``configuration`` section.""" > + > + #: > link: str > > > class LshwOutput(TypedDict): > - """ > - A model of the relevant information from json lshw output, e.g.: > - { > - ... > - "businfo" : "pci@0000:08:00.0", > - "logicalname" : "enp8s0", > - "version" : "00", > - "serial" : "52:54:00:59:e1:ac", > - ... > - "configuration" : { > - ... > - "link" : "yes", > - ... > - }, > - ... > + """A model of the relevant information from ``lshw``'s json output. > + > + e.g.:: > + > + { > + ... > + "businfo" : "pci@0000:08:00.0", > + "logicalname" : "enp8s0", > + "version" : "00", > + "serial" : "52:54:00:59:e1:ac", > + ... > + "configuration" : { > + ... > + "link" : "yes", > + ... > + }, > + ... > """ > > + #: > businfo: str > + #: > logicalname: NotRequired[str] > + #: > serial: NotRequired[str] > + #: > configuration: LshwConfigurationOutput > > > class LinuxSession(PosixSession): > - """ > - The implementation of non-Posix compliant parts of Linux remote sessions. > - """ > + """The implementation of non-Posix compliant parts of Linux.""" > > @staticmethod > def _get_privileged_command(command: str) -> str: > return f"sudo -- sh -c '{command}'" > > def get_remote_cpus(self, use_first_core: bool) -> list[LogicalCore]: > + """Overrides :meth:`~.os_session.OSSession.get_remote_cpus`.""" > cpu_info = self.send_command("lscpu -p=CPU,CORE,SOCKET,NODE|grep -v \\#").stdout > lcores = [] > for cpu_line in cpu_info.splitlines(): > @@ -65,18 +80,20 @@ def get_remote_cpus(self, use_first_core: bool) -> list[LogicalCore]: > return lcores > > def get_dpdk_file_prefix(self, dpdk_prefix: str) -> str: > + """Overrides :meth:`~.os_session.OSSession.get_dpdk_file_prefix`.""" > return dpdk_prefix > > - def setup_hugepages(self, hugepage_amount: int, force_first_numa: bool) -> None: > + def setup_hugepages(self, hugepage_count: int, force_first_numa: bool) -> None: > + """Overrides :meth:`~.os_session.OSSession.setup_hugepages`.""" > self._logger.info("Getting Hugepage information.") > hugepage_size = self._get_hugepage_size() > hugepages_total = self._get_hugepages_total() > self._numa_nodes = self._get_numa_nodes() > > - if force_first_numa or hugepages_total != hugepage_amount: > + if force_first_numa or hugepages_total != hugepage_count: > # when forcing numa, we need to clear existing hugepages regardless > # of size, so they can be moved to the first numa node > - self._configure_huge_pages(hugepage_amount, hugepage_size, force_first_numa) > + self._configure_huge_pages(hugepage_count, hugepage_size, force_first_numa) > else: > self._logger.info("Hugepages already configured.") > self._mount_huge_pages() > @@ -140,6 +157,7 @@ def _configure_huge_pages( > ) > > def update_ports(self, ports: list[Port]) -> None: > + """Overrides :meth:`~.os_session.OSSession.update_ports`.""" > self._logger.debug("Gathering port info.") > for port in ports: > assert ( > @@ -178,6 +196,7 @@ def _update_port_attr( > ) > > def configure_port_state(self, port: Port, enable: bool) -> None: > + """Overrides :meth:`~.os_session.OSSession.configure_port_state`.""" > state = "up" if enable else "down" > self.send_command( > f"ip link set dev {port.logical_name} {state}", privileged=True > @@ -189,6 +208,7 @@ def configure_port_ip_address( > port: Port, > delete: bool, > ) -> None: > + """Overrides :meth:`~.os_session.OSSession.configure_port_ip_address`.""" > command = "del" if delete else "add" > self.send_command( > f"ip address {command} {address} dev {port.logical_name}", > @@ -197,5 +217,6 @@ def configure_port_ip_address( > ) > > def configure_ipv4_forwarding(self, enable: bool) -> None: > + """Overrides :meth:`~.os_session.OSSession.configure_ipv4_forwarding`.""" > state = 1 if enable else 0 > self.send_command(f"sysctl -w net.ipv4.ip_forward={state}", privileged=True) > diff --git a/dts/framework/testbed_model/posix_session.py b/dts/framework/testbed_model/posix_session.py > index 1d1d5b1b26..a4824aa274 100644 > --- a/dts/framework/testbed_model/posix_session.py > +++ b/dts/framework/testbed_model/posix_session.py > @@ -2,6 +2,15 @@ > # Copyright(c) 2023 PANTHEON.tech s.r.o. > # Copyright(c) 2023 University of New Hampshire > > +"""POSIX compliant OS translator. > + > +Translates OS-unaware calls into POSIX compliant calls/utilities. POSIX is a set of standards > +for portability between Unix operating systems which not all Linux distributions > +(or the tools most frequently bundled with said distributions) adhere to. Most of Linux > +distributions are mostly compliant though. > +This intermediate module implements the common parts of mostly POSIX compliant distributions. > +""" > + > import re > from collections.abc import Iterable > from pathlib import PurePath, PurePosixPath > @@ -15,13 +24,21 @@ > > > class PosixSession(OSSession): > - """ > - An intermediary class implementing the Posix compliant parts of > - Linux and other OS remote sessions. > - """ > + """An intermediary class implementing the POSIX standard.""" > > @staticmethod > def combine_short_options(**opts: bool) -> str: > + """Combine shell options into one argument. > + > + These are options such as ``-x``, ``-v``, ``-f`` which are combined into ``-xvf``. > + > + Args: > + opts: The keys are option names (usually one letter) and the bool values indicate > + whether to include the option in the resulting argument. > + > + Returns: > + The options combined into one argument. > + """ > ret_opts = "" > for opt, include in opts.items(): > if include: > @@ -33,17 +50,19 @@ def combine_short_options(**opts: bool) -> str: > return ret_opts > > def guess_dpdk_remote_dir(self, remote_dir: str | PurePath) -> PurePosixPath: > + """Overrides :meth:`~.os_session.OSSession.guess_dpdk_remote_dir`.""" > remote_guess = self.join_remote_path(remote_dir, "dpdk-*") > result = self.send_command(f"ls -d {remote_guess} | tail -1") > return PurePosixPath(result.stdout) > > def get_remote_tmp_dir(self) -> PurePosixPath: > + """Overrides :meth:`~.os_session.OSSession.get_remote_tmp_dir`.""" > return PurePosixPath("/tmp") > > def get_dpdk_build_env_vars(self, arch: Architecture) -> dict: > - """ > - Create extra environment variables needed for i686 arch build. Get information > - from the node if needed. > + """Overrides :meth:`~.os_session.OSSession.get_dpdk_build_env_vars`. > + > + Supported architecture: ``i686``. > """ > env_vars = {} > if arch == Architecture.i686: > @@ -63,6 +82,7 @@ def get_dpdk_build_env_vars(self, arch: Architecture) -> dict: > return env_vars > > def join_remote_path(self, *args: str | PurePath) -> PurePosixPath: > + """Overrides :meth:`~.os_session.OSSession.join_remote_path`.""" > return PurePosixPath(*args) > > def copy_from( > @@ -70,6 +90,7 @@ def copy_from( > source_file: str | PurePath, > destination_file: str | PurePath, > ) -> None: > + """Overrides :meth:`~.os_session.OSSession.copy_from`.""" > self.remote_session.copy_from(source_file, destination_file) > > def copy_to( > @@ -77,6 +98,7 @@ def copy_to( > source_file: str | PurePath, > destination_file: str | PurePath, > ) -> None: > + """Overrides :meth:`~.os_session.OSSession.copy_to`.""" > self.remote_session.copy_to(source_file, destination_file) > > def remove_remote_dir( > @@ -85,6 +107,7 @@ def remove_remote_dir( > recursive: bool = True, > force: bool = True, > ) -> None: > + """Overrides :meth:`~.os_session.OSSession.remove_remote_dir`.""" > opts = PosixSession.combine_short_options(r=recursive, f=force) > self.send_command(f"rm{opts} {remote_dir_path}") > > @@ -93,6 +116,7 @@ def extract_remote_tarball( > remote_tarball_path: str | PurePath, > expected_dir: str | PurePath | None = None, > ) -> None: > + """Overrides :meth:`~.os_session.OSSession.extract_remote_tarball`.""" > self.send_command( > f"tar xfm {remote_tarball_path} " > f"-C {PurePosixPath(remote_tarball_path).parent}", > @@ -110,6 +134,7 @@ def build_dpdk( > rebuild: bool = False, > timeout: float = SETTINGS.compile_timeout, > ) -> None: > + """Overrides :meth:`~.os_session.OSSession.build_dpdk`.""" > try: > if rebuild: > # reconfigure, then build > @@ -140,12 +165,14 @@ def build_dpdk( > raise DPDKBuildError(f"DPDK build failed when doing '{e.command}'.") > > def get_dpdk_version(self, build_dir: str | PurePath) -> str: > + """Overrides :meth:`~.os_session.OSSession.get_dpdk_version`.""" > out = self.send_command( > f"cat {self.join_remote_path(build_dir, 'VERSION')}", verify=True > ) > return out.stdout > > def kill_cleanup_dpdk_apps(self, dpdk_prefix_list: Iterable[str]) -> None: > + """Overrides :meth:`~.os_session.OSSession.kill_cleanup_dpdk_apps`.""" > self._logger.info("Cleaning up DPDK apps.") > dpdk_runtime_dirs = self._get_dpdk_runtime_dirs(dpdk_prefix_list) > if dpdk_runtime_dirs: > @@ -159,6 +186,14 @@ def kill_cleanup_dpdk_apps(self, dpdk_prefix_list: Iterable[str]) -> None: > def _get_dpdk_runtime_dirs( > self, dpdk_prefix_list: Iterable[str] > ) -> list[PurePosixPath]: > + """Find runtime directories DPDK apps are currently using. > + > + Args: > + dpdk_prefix_list: The prefixes DPDK apps were started with. > + > + Returns: > + The paths of DPDK apps' runtime dirs. > + """ > prefix = PurePosixPath("/var", "run", "dpdk") > if not dpdk_prefix_list: > remote_prefixes = self._list_remote_dirs(prefix) > @@ -170,9 +205,13 @@ def _get_dpdk_runtime_dirs( > return [PurePosixPath(prefix, dpdk_prefix) for dpdk_prefix in dpdk_prefix_list] > > def _list_remote_dirs(self, remote_path: str | PurePath) -> list[str] | None: > - """ > - Return a list of directories of the remote_dir. > - If remote_path doesn't exist, return None. > + """Contents of remote_path. > + > + Args: > + remote_path: List the contents of this path. > + > + Returns: > + The contents of remote_path. If remote_path doesn't exist, return None. > """ > out = self.send_command( > f"ls -l {remote_path} | awk '/^d/ {{print $NF}}'" > @@ -183,6 +222,17 @@ def _list_remote_dirs(self, remote_path: str | PurePath) -> list[str] | None: > return out.splitlines() > > def _get_dpdk_pids(self, dpdk_runtime_dirs: Iterable[str | PurePath]) -> list[int]: > + """Find PIDs of running DPDK apps. > + > + Look at each "config" file found in dpdk_runtime_dirs and find the PIDs of processes > + that opened those file. > + > + Args: > + dpdk_runtime_dirs: The paths of DPDK apps' runtime dirs. > + > + Returns: > + The PIDs of running DPDK apps. > + """ > pids = [] > pid_regex = r"p(\d+)" > for dpdk_runtime_dir in dpdk_runtime_dirs: > @@ -203,6 +253,14 @@ def _remote_files_exists(self, remote_path: PurePath) -> bool: > def _check_dpdk_hugepages( > self, dpdk_runtime_dirs: Iterable[str | PurePath] > ) -> None: > + """Check there aren't any leftover hugepages. > + > + If any hugegapes are found, emit a warning. The hugepages are investigated in the hugegapes -> hugepages > + "hugepage_info" file of dpdk_runtime_dirs. > + > + Args: > + dpdk_runtime_dirs: The paths of DPDK apps' runtime dirs. > + """ > for dpdk_runtime_dir in dpdk_runtime_dirs: > hugepage_info = PurePosixPath(dpdk_runtime_dir, "hugepage_info") > if self._remote_files_exists(hugepage_info): > @@ -220,9 +278,11 @@ def _remove_dpdk_runtime_dirs( > self.remove_remote_dir(dpdk_runtime_dir) > > def get_dpdk_file_prefix(self, dpdk_prefix: str) -> str: > + """Overrides :meth:`~.os_session.OSSession.get_dpdk_file_prefix`.""" > return "" > > def get_compiler_version(self, compiler_name: str) -> str: > + """Overrides :meth:`~.os_session.OSSession.get_compiler_version`.""" > match compiler_name: > case "gcc": > return self.send_command( > @@ -240,6 +300,7 @@ def get_compiler_version(self, compiler_name: str) -> str: > raise ValueError(f"Unknown compiler {compiler_name}") > > def get_node_info(self) -> NodeInfo: > + """Overrides :meth:`~.os_session.OSSession.get_node_info`.""" > os_release_info = self.send_command( > "awk -F= '$1 ~ /^NAME$|^VERSION$/ {print $2}' /etc/os-release", > SETTINGS.timeout,