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 EEFBB45D04; Thu, 14 Nov 2024 13:19:18 +0100 (CET) Received: from mails.dpdk.org (localhost [127.0.0.1]) by mails.dpdk.org (Postfix) with ESMTP id C428F4028A; Thu, 14 Nov 2024 13:19:18 +0100 (CET) Received: from foss.arm.com (foss.arm.com [217.140.110.172]) by mails.dpdk.org (Postfix) with ESMTP id 3CD3F4025D for ; Thu, 14 Nov 2024 13:19:17 +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 873461480; Thu, 14 Nov 2024 04:19:46 -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 6716F3F6A8; Thu, 14 Nov 2024 04:19:15 -0800 (PST) Message-ID: <73a0df5b-feca-4fe8-95b1-d616bb2e8770@arm.com> Date: Thu, 14 Nov 2024 12:19:13 +0000 MIME-Version: 1.0 User-Agent: Mozilla Thunderbird Subject: Re: [PATCH v4] dts: add flow rule dataclass to testpmd shell To: Dean Marx , probb@iol.unh.edu, npratte@iol.unh.edu, yoan.picchi@foss.arm.com, Honnappa.Nagarahalli@arm.com, paul.szczepanek@arm.com Cc: dev@dpdk.org References: <20240813144107.6195-1-dmarx@iol.unh.edu> <20241010210631.705-1-dmarx@iol.unh.edu> Content-Language: en-GB From: Luca Vizzarro In-Reply-To: <20241010210631.705-1-dmarx@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 Hi Dean, This looks fine for most of it, but some of Juraj's prior comments still apply. See comments in line. On 10/10/2024 22:06, Dean Marx wrote: > +@dataclass I'd make this kw_only=True: @dataclass(kw_only=True) this enforces the construction to include field names for the sake of readability. Having: FlowRule(0, True, "ip", "rss") is not really as self-descriptive as: FlowRule(port_id=0, ingress=True, pattern="ip", actions="rss") > +class FlowRule: > + """Dataclass for setting flow rule parameters.""" We should add the pattern that this is meant to represent in the class docstring here above, as suggested by Juraj. Also I'd change the current docstring to what the class is actually doing, e.g.: Class representation of flow rule parameters. This class represents the parameters of any flow rule as per the following pattern: [group {group_id}] [priority {level}] [ingress] [egress] [user_id {user_id}] pattern {item} [/ {item} [...]] / end actions {action} [/ {action} [...]] / end Mind that the pattern above is taking from the testpmd guide[1], I excluded the bits that you haven't implemented. > + > + #: > + port_id: int I would leave out the `port_id` here, if we wanted to apply the same flow rule to multiple ports, this would complicate things. This should belong under the `flow_create` function. > + #: > + ingress: bool > + #: > + pattern: str The rule pattern above also suggests that this... > + #: > + actions: str ...and this can be `list[str]`, and then joined in __str__: "/ ".join(self.actions) > + > + #: > + group_id: int | None = None > + #: > + priority_level: int | None = None > + #: > + user_id: int | None = None Perhaps, order the fields in the way they are represented in the flow rule. So this should go before actions and patterns. > + > + def __str__(self) -> str: > + """Returns the string representation of a flow_func instance. s/flow_func/FlowRule, or just don't mention it at all as it's implicit: Returns the string representation of this instance. > + > + In this case, a properly formatted flow create command that can be sent to testpmd. I reckon that `flow create` should be created by the flow create command function. Let's keep this to just produce the parameters settings... > + """ > + ret = f"flow create {self.port_id} " ...and make this an empty string: ret = "" > + if self.group_id is not None: > + ret += f"group {self.group_id} " > + if self.priority_level is not None: > + ret += f"priority {self.priority_level} " > + ret += "ingress " if self.ingress else "egress " The pattern above suggests we can have flow rules for both ingress and egress. Therefore I'd have a field for egress as well, or we want to get fancy, create a FlowRuleDirection enum.Flag, so that we can do: FlowRule(direction=FlowRuleDirection.INGRESS|EGRESS, ...) Or to not overcomplicate we can just rely on mypy's checking: class FlowRule: ... direction: Literal["ingress", "egress", "both"] FlowRule(direction="ingress") > + if self.user_id is not None: > + ret += f"user_id {self.user_id} " > + ret += f"pattern {self.pattern} / end " > + ret += f"actions {self.actions} / end" > + return ret > + > + > class PacketOffloadFlag(Flag): > """Flag representing the Packet Offload Features Flags in DPDK. > > @@ -1717,6 +1755,25 @@ def show_port_stats(self, port_id: int) -> TestPmdPortStats: > > return TestPmdPortStats.parse(output) > > + def flow_create(self, flow_rule: FlowRule, verify: bool = True) -> None: As mentioned, let's put the port_id here. > + """Creates a flow rule in the testpmd session. > + > + Args: > + flow_rule: FlowRule object used for creating testpmd flow rule. Reference to the class missing: :class:`FlowRule` object > + verify: If :data:`True`, the output of the command is scanned > + to ensure the flow rule was created successfully. > + > + Raises: > + InteractiveCommandExecutionError: If flow rule is invalid. > + """ > + flow_output = self.send_command(str(flow_rule)) And this can be come: flow_output = self.send_command(f"flow create {port_id} {flow_rule}") > + if verify: > + if "created" not in flow_output: > + self._logger.debug(f"Failed to create flow rule:\n{flow_output}") > + raise InteractiveCommandExecutionError( > + f"Failed to create flow rule:\n{flow_output}" > + ) > + > @requires_stopped_ports > def set_port_mtu(self, port_id: int, mtu: int, verify: bool = True) -> None: > """Change the MTU of a port using testpmd. Finally, I also agree with Juraj's comment to make a structure for actions and patterns. But that's overkill for now. We can just deal with it if we have to deal with their complexity in the future. But like also Juraj's said, I'd introduce a flow_delete function with flow_create. You should make it so flow_create returns the flow rule ID, so that we can manipulate it, and use it with flow_delete. Last thing, I'd split the FlowRule class and the flow_(create|delete) commands in two commits. I hope my comments are helpful and make sense. Also mind that I don't have a thorough knowledge of flows, I've just had a crash course for the purpose of this review. So I may not understand everything and could be wrong, in which case please correct me. Best, Luca [1] https://doc.dpdk.org/guides/testpmd_app_ug/testpmd_funcs.html#flow-syntax