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 0AC524591F; Fri, 6 Sep 2024 16:29:43 +0200 (CEST) Received: from mails.dpdk.org (localhost [127.0.0.1]) by mails.dpdk.org (Postfix) with ESMTP id EC9AC42F71; Fri, 6 Sep 2024 16:29:42 +0200 (CEST) Received: from mail-ed1-f49.google.com (mail-ed1-f49.google.com [209.85.208.49]) by mails.dpdk.org (Postfix) with ESMTP id 87C0C42F69 for ; Fri, 6 Sep 2024 16:29:41 +0200 (CEST) Received: by mail-ed1-f49.google.com with SMTP id 4fb4d7f45d1cf-5c3d209db94so1818678a12.3 for ; Fri, 06 Sep 2024 07:29:41 -0700 (PDT) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=pantheon.tech; s=google; t=1725632981; x=1726237781; darn=dpdk.org; h=content-transfer-encoding:in-reply-to:from:content-language :references:cc:to:subject:user-agent:mime-version:date:message-id :from:to:cc:subject:date:message-id:reply-to; bh=1fS0touiCY40pxiga50Xq0ypiN8tgMtpzIEQ4cW8i0w=; b=r+Cdte6mqNYWuAea/zM/QJKR08JVAlQx6VuDlL0beeYPqaJ3qnm8YmdKiiAdrPA2Pd uEUpeOFLQQgFmw/XpWqUsQL7K77WmhhUTN0yvtA1Knc5b6IvN8Dfb9CRb5me43PNyU/w F9QZfYq6GI51Wyd9JqO4QNutE0ueSyCBV2B6PSdktCjbS3M/TUV0McsvidxKQZXoGzD0 CyvK8rrrf0vzXhAfY+cpF5QKLKdI9/DTNASmZO6dQ+2Na4JCykD2VrLYRFCO5s4ZFkpQ aD7q8hwZWQMudEhasXU3fwSTbsdp1X/oO+iwTHRyVMLzZwSXNM/1PsKR8L/Rc7p5tyhB e8EA== X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20230601; t=1725632981; x=1726237781; h=content-transfer-encoding:in-reply-to:from:content-language :references:cc:to:subject:user-agent:mime-version:date:message-id :x-gm-message-state:from:to:cc:subject:date:message-id:reply-to; bh=1fS0touiCY40pxiga50Xq0ypiN8tgMtpzIEQ4cW8i0w=; b=f6zhDQKsuL4iHIPCX0D5iCGfpAPFeqbXZEgR7cj+Q5nf0eHk0GvvyTMkGdlpmMu60Q 2j3es1UWaVBqfUlzoEKVV3xJDWqumvrF6irqqWhBpxqhyPhWuKpnZUVsHhctJd53ZRo0 ok00wQLW5+8Kmt0V+n61LdS7eYhrhm4RaAeQ0OMaxGAfBedrKRWd5TBY6Tnnfe1LUcNK qVWzMR3wsCGnvJ8R9qEj8+W6/pjd2G/ebLe82imNlhhxeXZ+mQvHVbbt46BEEZD33ec3 MNuFjuLnoddowh0DzwUlBgOslFUzLECuTS66cTY0UJ1yBSxJRG7MzWUULyWWvCXOHO6i Rmbw== X-Forwarded-Encrypted: i=1; AJvYcCUIP8cWbgovGRKNMSYHm9qj8fnL9nMYCuHot4E2iW2zqUDecGQqoRYDXt7DhbC1mH2YfTM=@dpdk.org X-Gm-Message-State: AOJu0Yy9DWzrbJ2S9leWmVnzUBjLBaE8G2ys0XnA9GsRopMJiZ4YuMp+ olJ6Pzvqd3dyIo0pSJHoTSnDxvpBYr1AS0+6jCBHrTK606YKRXWyGrEixqXjreU= X-Google-Smtp-Source: AGHT+IFMhjk85eM8ccQ7ltADQDWQh9gh9v3h8W07UG6qqmQr9mSVsPRyTAvvFtCQuO9Re4qureYZZQ== X-Received: by 2002:a05:6402:2747:b0:5c2:56d2:20b3 with SMTP id 4fb4d7f45d1cf-5c256d22473mr14507285a12.21.1725632980834; Fri, 06 Sep 2024 07:29:40 -0700 (PDT) Received: from [192.168.200.22] ([84.245.121.62]) by smtp.gmail.com with ESMTPSA id 4fb4d7f45d1cf-5c3cc551397sm2520050a12.38.2024.09.06.07.29.40 (version=TLS1_3 cipher=TLS_AES_128_GCM_SHA256 bits=128/128); Fri, 06 Sep 2024 07:29:40 -0700 (PDT) Message-ID: <0af298a4-9852-458c-8e70-f4c282a3ad6c@pantheon.tech> Date: Fri, 6 Sep 2024 16:29:39 +0200 MIME-Version: 1.0 User-Agent: Mozilla Thunderbird Subject: Re: [PATCH] dts: add RSS functions to testpmd To: Alex Chapman , dev@dpdk.org Cc: Honnappa Nagarahalli , Paul Szczepanek , Luca Vizzarro References: <20240829125020.34341-1-alex.chapman@arm.com> Content-Language: en-US From: =?UTF-8?Q?Juraj_Linke=C5=A1?= In-Reply-To: <20240829125020.34341-1-alex.chapman@arm.com> 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 Two notes on sending patches. The patch is missing a version. I believe this is version 2? Second, looks like you didn't send this as a reply to the first version of the patch. The message-id to use can be found in the patchwork patch page. On 29. 8. 2024 14:50, Alex Chapman wrote: > This patch adds the required functionality for the RSS key_update, > RETA, and hash test suites. This includes: > The setting of custom RETA values for routing packets to specific > queues. > The setting of the RSS mode on all ports, to specify how to hash > the packets. > The updating of the RSS hash key used during the hashing process. > > Alongside this, there is the addition of a __str__ method to the > RSSOffloadTypesFlags class, so that when flag names are cast to > a string they will use '-' as separators, instead of '_'. > This allows them to be directly used within testpmd RSS commands > without any further changes. > > Signed-off-by: Alex Chapman > Reviewed-by: Luca Vizzarro > --- > dts/framework/remote_session/testpmd_shell.py | 84 ++++++++++++++++++- > 1 file changed, 83 insertions(+), 1 deletion(-) > > diff --git a/dts/framework/remote_session/testpmd_shell.py b/dts/framework/remote_session/testpmd_shell.py > index 43e9f56517..7b66901f6e 100644 > --- a/dts/framework/remote_session/testpmd_shell.py > +++ b/dts/framework/remote_session/testpmd_shell.py > @@ -23,7 +23,7 @@ > > from typing_extensions import Self, Unpack > > -from framework.exception import InteractiveCommandExecutionError > +from framework.exception import InteractiveCommandExecutionError, InternalError > from framework.params.testpmd import SimpleForwardingModes, TestPmdParams > from framework.params.types import TestPmdParamsDict > from framework.parser import ParserFn, TextParser > @@ -305,6 +305,12 @@ def make_parser(cls) -> ParserFn: > RSSOffloadTypesFlag.from_list_string, > ) > > + def __str__(self): > + """Stringifies the flag name.""" Since the stringifying is basically a given, I'd put something else here, such as "Replace undercores with hyphens to produce valid testpmd value." or something similar. > + if self.name is None: > + return "" > + return self.name.replace("_", "-") > + > > class DeviceCapabilitiesFlag(Flag): > """Flag representing the device capabilities.""" > @@ -723,6 +729,82 @@ def set_forward_mode(self, mode: SimpleForwardingModes, verify: bool = True): > f"Test pmd failed to set fwd mode to {mode.value}" > ) > > + def port_config_rss_reta( > + self, port_id: int, hash_index: int, queue_id: int, verify: bool = True > + ) -> None: > + """Configures a port's RSS redirection table. Just a note here: I like to use imperative (configure), but we don't really enforce this in the framework (we have both imperative and third person). > + > + Args: > + port_id: Port where redirection table will be configured. This is missing definite articles (which are present everywhere else): port where redirection table -> the port where the redirection table > + hash_index: The index into the redirection table associated with the destination queue. > + queue_id: The destination queue of the packet. > + verify: If :data:`True`, it verifies if a port's redirection table the "it" is redundant > + was correctly configured. > + > + Raises: > + InteractiveCommandExecutionError: If `verify` is :data:`True` > + Testpmd failed to config RSS reta. > + """ > + out = self.send_command(f"port config {port_id} rss reta ({hash_index},{queue_id})") > + if verify: > + if f"The reta size of port {port_id} is" not in out: > + self._logger.debug(f"Failed to config RSS reta: \n{out}") > + raise InteractiveCommandExecutionError("Testpmd failed to config RSS reta.") > + > + def port_config_all_rss_offload_type( > + self, flag: RSSOffloadTypesFlag, verify: bool = True > + ) -> None: > + """Set the RSS mode on all ports. > + > + Args: > + rss_offload_type: The RSS iptype all ports will be configured to. This doesn't match the arg name, we should probably rename flag to rss_offload_type. Also, RSSOffloadTypesFlag says: Flag representing the RSS offload flow types supported by the NIC port. I don't know what iptype is, we could just replace iptype with "offload flow type". > + verify: If :data:`True`, it verifies if all ports RSS offload type > + was correctly configured. > + > + Raises: > + InternalError: Offload Flag has contradictory values set. > + InteractiveCommandExecutionError: If `verify` is :data:`True` > + Testpmd failed to config the RSS mode on all ports. > + """ > + if not flag.name: > + raise InternalError("Offload Flag has contradictory values set") Out of curiosity, when can this happen? Also, two more points: 1. Please end sentences with a dot. Check the whole patch. 2. I really don't like messages constructed like this as it doesn't help that much when debugging. Let's add more info, like what the contradiction is. > + > + out = self.send_command(f"port config all rss {flag.name}") > + > + if verify: > + if "error" in out: > + self._logger.debug(f"Failed to config the RSS mode on all ports: \n{out}") > + raise InteractiveCommandExecutionError( > + "Testpmd failed to config the RSS mode on all ports" > + ) Here we could also add what mode we were configuring. > + > + def port_config_rss_hash_key( > + self, port_id: int, offload_type: RSSOffloadTypesFlag, hex_str: str, verify: bool = True > + ) -> str: > + """Set the RSS hash key for the specified port. Now this is in imperative instead of third person. Let's pick one for the entire patch. > + > + Args: > + port_id: Port the hash key will be set. This looks like an unfinished sentence, should it be set for (as in the docsting's first line)? > + offload_type: The offload type the hash key will be applied to. > + hex_str: The new hash key. > + verify: If :data:`True`, RSS hash key has been correctly set. verify that RSS has key > + > + Raises: > + InteractiveCommandExecutionError: If `verify` is :data:`True` > + Testpmd failed to set the RSS hash key. > + """ > + output = self.send_command(f"port config {port_id} rss-hash-key {offload_type} {hex_str}") > + > + if verify: > + if ( > + "invalid - key must be a string of" in output > + or "Bad arguments" in output > + or "Error during getting device" in output > + ): Would it make sense to check for one positive instead of multiple negatives? Or is it not that simple? > + self._logger.debug(f"Failed to set rss hash key: \n{output}") > + raise InteractiveCommandExecutionError("Testpmd failed to set the RSS hash key.") We can also add extra info here: port, offload_type and the hex str. > + return output > + > def show_port_info_all(self) -> list[TestPmdPort]: > """Returns the information of all the ports. >