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 03B7445D07; Thu, 14 Nov 2024 18:29:21 +0100 (CET) Received: from mails.dpdk.org (localhost [127.0.0.1]) by mails.dpdk.org (Postfix) with ESMTP id C6E704025D; Thu, 14 Nov 2024 18:29:20 +0100 (CET) Received: from foss.arm.com (foss.arm.com [217.140.110.172]) by mails.dpdk.org (Postfix) with ESMTP id 2D91B4021F for ; Thu, 14 Nov 2024 18:29:18 +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 96C47169E; Thu, 14 Nov 2024 09:29:47 -0800 (PST) Received: from [10.1.39.61] (JR4XG4HTQC.cambridge.arm.com [10.1.39.61]) by usa-sjc-imap-foss1.foss.arm.com (Postfix) with ESMTPSA id 035E63F6A8; Thu, 14 Nov 2024 09:29:13 -0800 (PST) Message-ID: <54f1e692-e788-40ce-85e4-f4b9fa92cd72@arm.com> Date: Thu, 14 Nov 2024 17:29:12 +0000 MIME-Version: 1.0 User-Agent: Mozilla Thunderbird Subject: Re: [PATCH v4 4/5] dts: add OS abstractions for creating virtual functions Content-Language: en-GB To: jspewock@iol.unh.edu, npratte@iol.unh.edu, juraj.linkes@pantheon.tech, yoan.picchi@foss.arm.com, thomas@monjalon.net, Honnappa.Nagarahalli@arm.com, probb@iol.unh.edu, wathsala.vithanage@arm.com, paul.szczepanek@arm.com, alex.chapman@arm.com Cc: dev@dpdk.org References: <20240821191557.18744-1-jspewock@iol.unh.edu> <20240923184235.22582-1-jspewock@iol.unh.edu> <20240923184235.22582-5-jspewock@iol.unh.edu> From: Luca Vizzarro In-Reply-To: <20240923184235.22582-5-jspewock@iol.unh.edu> Content-Type: text/plain; charset=UTF-8; format=flowed Content-Transfer-Encoding: 7bit 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 Usual problem with the commit subject. On 23/09/2024 19:42, jspewock@iol.unh.edu wrote: > + def set_num_virtual_functions(self, num: int, pf_port: Port) -> bool: I'd call this set_num_of_virtual_functions, set_virtual_functions_num, or set_number_of_vfs. I think I prefer the last one. > + """Overrides :meth:`~.os_session.OSSession.set_num_virtual_functions`.""" > + sys_bus_path = f"/sys/bus/pci/devices/{pf_port.pci}/sriov_numvfs".replace(":", "\\:") > + curr_num_vfs = int(self.send_command(f"cat {sys_bus_path}").stdout) > + if num > 0 and curr_num_vfs >= num: > + self._logger.info( > + f"{curr_num_vfs} VFs already configured on port {pf_port.identifier.pci} on node " > + f"{pf_port.identifier.node}." > + ) > + return False > + elif num > 0 and curr_num_vfs > 0: > + self._logger.error( > + f"Not enough VFs configured on port {pf_port.identifier.pci} on node " > + f"{pf_port.identifier.node}. Need {num} but only {curr_num_vfs} are configured. " > + "DTS is unable to modify number of preexisting VFs." > + ) > + raise InternalError("Failed to create VFs on port.") This is not an InternalError. That exception represents a bug internal to DTS, it's not related to the node not satisfying requirements. I am guessing this requires a new kind of error. > + self.send_command(f"echo {num} > {sys_bus_path}", privileged=True, verify=True) > + return True > + > + def get_pci_addr_of_vfs(self, pf_port: Port) -> list[str]: > + """Overrides :meth:`~.os_session.OSSession.get_pci_addr_of_vfs`.""" > + sys_bus_path = f"/sys/bus/pci/devices/{pf_port.pci}".replace(":", "\\:") > + curr_num_vfs = int(self.send_command(f"cat {sys_bus_path}/sriov_numvfs").stdout) > + if curr_num_vfs > 0: > + pci_addrs = self.send_command( > + 'awk -F "PCI_SLOT_NAME=" "/PCI_SLOT_NAME=/ {print \\$2}" ' > + + f"{sys_bus_path}/virtfn*/uevent", I like the use of awk. Using a TextWrapper may be overkill when we have easier options like this one. > + def set_num_virtual_functions(self, num: int, pf_port: Port) -> bool: > + """Update the number of virtual functions (VFs) on a port. Like Juraj mentioned, this docstring should say that it can be used for creating and removing VFs.