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 0553D4578F; Fri, 23 Aug 2024 16:54:19 +0200 (CEST) Received: from mails.dpdk.org (localhost [127.0.0.1]) by mails.dpdk.org (Postfix) with ESMTP id 7613E43367; Fri, 23 Aug 2024 16:54:13 +0200 (CEST) Received: from mail-pj1-f45.google.com (mail-pj1-f45.google.com [209.85.216.45]) by mails.dpdk.org (Postfix) with ESMTP id 8F9D243370 for ; Fri, 23 Aug 2024 16:54:11 +0200 (CEST) Received: by mail-pj1-f45.google.com with SMTP id 98e67ed59e1d1-2d3bd8784d3so1617854a91.3 for ; Fri, 23 Aug 2024 07:54:11 -0700 (PDT) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=iol.unh.edu; s=unh-iol; t=1724424851; x=1725029651; darn=dpdk.org; h=content-transfer-encoding:cc:to:subject:message-id:date:from :in-reply-to:references:mime-version:from:to:cc:subject:date :message-id:reply-to; bh=SgekdD69BEADT0BgObQFUYE5BuE3ob+NOQsRxAYQPVw=; b=SKnJDJa9IRFDHkyUdPvIn2vBDL71e4sdBCV1P9ZVnjUWT15Iz+aGV89HAC+m8nFRir m5EsmjhlhAd3SzxQubCVHPip6Gomkwk4iZmfjIbrSxPFqd9nFkbCNLZnqye2fD+rTFGm QA+kjtt0957qMG5HUJsSam3LxckDVYNHZ+/VI= X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20230601; t=1724424851; x=1725029651; h=content-transfer-encoding:cc:to:subject:message-id:date:from :in-reply-to:references:mime-version:x-gm-message-state:from:to:cc :subject:date:message-id:reply-to; bh=SgekdD69BEADT0BgObQFUYE5BuE3ob+NOQsRxAYQPVw=; b=DPKS7NzYtKXW1KHX6gMyVAKsybjfV4ak+VP/4g7T8pk244SFxcK+VwkgxZvDqPTl84 peurDsiAUK3JeWunttRfUol62eoF8igDlFBpZyd9HEd4FL5+qjyK3VlarZjy9XVbF3p5 HWIr0f4lmuq4cX4QAjnwv9HwDLm01NeWFFQyD9BUjH09GXmwCZkEBcV57HGs/PNWWGXf 4IIj86Tf+OLLe5vIR05FNeF+masTXVSoJvhgbpeRx0iimMH8dhOWqeb199IEXrFM8RJg QTIfpNZH4zGb9ta9j7hDKPgUPTDyMQjJKHPjH4Vqyk5to4DjRw/L4tTR0mBWul+sWpNA 5P/w== X-Forwarded-Encrypted: i=1; AJvYcCUXRtespfFgSqIKeyp019XS1eJSNG33VK8+/vPRTBee9dhrzvSvIyur3nERkwAtvWFh5io=@dpdk.org X-Gm-Message-State: AOJu0YzPO6QUHUo3EJsOjEB9tGPldGt3ZuO/FDuG0eXzKQhHVrXX2tEH ZQ6fhJt5Ziyok4veWovuTB6VekHbwy42MmqDPlFoP52Vs71EmqvrrCUtM6AzsOJPtEkQ5CbE979 RinWRU9qFh93p17+4R2PAZdjNn8yXekIy9yk1Fg== X-Google-Smtp-Source: AGHT+IHdV67b0oV+3UbHRB3Z8nW2O3hh+PZAIYANQlF9NaWtDQcpkjyS5t849h1Th3jRscWozYKb/Y5iIO/wln3VxD0= X-Received: by 2002:a17:90b:3001:b0:2cf:c2da:771d with SMTP id 98e67ed59e1d1-2d646c452ebmr2626555a91.25.1724424850479; Fri, 23 Aug 2024 07:54:10 -0700 (PDT) MIME-Version: 1.0 References: <20240816142031.15150-1-dmarx@iol.unh.edu> <20240821162550.1163-1-dmarx@iol.unh.edu> <20240821162550.1163-2-dmarx@iol.unh.edu> In-Reply-To: <20240821162550.1163-2-dmarx@iol.unh.edu> From: Jeremy Spewock Date: Fri, 23 Aug 2024 10:53:59 -0400 Message-ID: Subject: Re: [PATCH v2 1/2] dts: add csum HW offload to testpmd shell To: Dean Marx Cc: probb@iol.unh.edu, npratte@iol.unh.edu, luca.vizzarro@arm.com, yoan.picchi@foss.arm.com, Honnappa.Nagarahalli@arm.com, paul.szczepanek@arm.com, juraj.linkes@pantheon.tech, dev@dpdk.org Content-Type: text/plain; charset="UTF-8" Content-Transfer-Encoding: quoted-printable 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 Wed, Aug 21, 2024 at 12:25=E2=80=AFPM Dean Marx wrot= e: > > add csum_set_hw method to testpmd shell class. Port over > set_verbose and port start/stop from queue start/stop suite. Since we had that discussion in a DTS meeting about there not really being a rule against multiple dependencies or anything like that, I think it might be better if we start moving to just always depending on other patches rather than duplicating code in between multiple series'. Not a call out to you at all because I think I have multiple patches open right now where I also borrow from other suites because I didn't want long dependency lists, but I think the lists of dependencies might end up being easier to track than where the code is from. It also makes for more targeted commit messages. Let me know what you think though. This might be something worth talking about with the larger development group as well to get more opinions on it. > > Signed-off-by: Dean Marx > --- > dts/framework/remote_session/testpmd_shell.py | 124 ++++++++++++++++++ > 1 file changed, 124 insertions(+) > > diff --git a/dts/framework/remote_session/testpmd_shell.py b/dts/framewor= k/remote_session/testpmd_shell.py > index 43e9f56517..be7cd16b96 100644 > --- a/dts/framework/remote_session/testpmd_shell.py > +++ b/dts/framework/remote_session/testpmd_shell.py > @@ -334,6 +334,32 @@ def make_parser(cls) -> ParserFn: > ) > > > +class ChecksumOffloadOptions(Flag): > + """Flag representing checksum hardware offload layer options.""" > + > + #: > + ip =3D auto() > + #: > + udp =3D auto() > + #: > + tcp =3D auto() > + #: > + sctp =3D auto() > + #: > + outerip =3D auto() > + #: > + outerudp =3D auto() > + > + def __str__(self): > + """String method for use in csum_set_hw.""" > + if self =3D=3D ChecksumOffloadOptions.outerip: > + return "outer-ip" > + elif self =3D=3D ChecksumOffloadOptions.outerudp: > + return "outer-udp" It might be easier to name these values outer_ip and outer_udp and then just do a str.replace("_", "-") on them to get the same result. > + else: > + return f"{self.name}" How does this behave if you have multiple flags combined? Like, for example, if I had `ChecksumOffloadOptions.ip | ChecksumOffloadOptions.tcp | ChecksumOffloadOptions.outerip`? I think, since it is not exactly equal to ChecksumOffloadOptions.outerip, it wouldn't hit those cases, and I'm not sure exactly what the name attribute becomes with multiple combined (I know the default string method does something like "ChecksumOffloadOptions.ip|tcp|outerip"). If you want it to only be one value instead of combinations of values it is probably better to make this an Enum, but I actually like it being a Flag for a reason I'll expand on more in the method where this is used. > + > + > class DeviceErrorHandlingMode(StrEnum): > """Enum representing the device error handling mode.""" > > @@ -806,6 +832,104 @@ def show_port_stats(self, port_id: int) -> TestPmdP= ortStats: > > return TestPmdPortStats.parse(output) > > + def port_stop(self, port: int, verify: bool =3D True): This method is missing the return-type. > + """Stop specified port. > + > + Args: > + port: Specifies the port number to use, must be between 0-32= . > + verify: If :data:`True`, the output of the command is scanne= d > + to ensure specified port is stopped. If not, it is consi= dered > + an error. > + > + Raises: > + InteractiveCommandExecutionError: If `verify` is :data:`True= ` and the port > + is not stopped. > + """ > + port_output =3D self.send_command(f"port stop {port}") > + if verify: > + if "Done" not in port_output: > + self._logger.debug(f"Failed to stop port {port}: \n{port= _output}") > + raise InteractiveCommandExecutionError(f"Testpmd failed = to stop port {port}.") > + > + def port_start(self, port: int, verify: bool =3D True): This method is also missing the return type annotation. > + """Start specified port. > + > + Args: > + port: Specifies the port number to use, must be between 0-32= . > + verify: If :data:`True`, the output of the command is scanne= d > + to ensure specified port is started. If not, it is consi= dered > + an error. > + > + Raises: > + InteractiveCommandExecutionError: If `verify` is :data:`True= ` and the port > + is not started. > + """ > + port_output =3D self.send_command(f"port start {port}") > + if verify: > + if "Done" not in port_output: > + self._logger.debug(f"Failed to start port {port}: \n{por= t_output}") > + raise InteractiveCommandExecutionError(f"Testpmd failed = to start port {port}.") > + > + def csum_set_hw(self, layer: ChecksumOffloadOptions, port_id: int, v= erify: bool =3D True) -> None: > + """Enables hardware checksum offloading on the specified layer. > + > + Args: > + layer: The layer that checksum offloading should be enabled = on. > + options: tcp, ip, udp, sctp, outer-ip, outer-udp. Now that the options are contained in a Flag, it might be better to not specify the options here and just let the Flag class be the one that documents it. That way this doc-string doesn't have to be modified if the class is. > + port_id: The port number to enable checksum offloading on, s= hould be within 0-32. > + verify: If :data:`True` the output of the command will be sc= anned in an attempt to > + verify that checksum offloading was enabled on the port. > + > + Raises: > + InteractiveCommandExecutionError: If checksum offload is not= enabled successfully. I think you may have forgotten to raise the exception in the `if verify` statement , it looks like it only prints a debug message right now. > + """ > + csum_output =3D self.send_command(f"csum set {str(layer)} hw {po= rt_id}") I think this might also run into some issues if you had a combination like the example I mentioned above (`ChecksumOffloadOptions.ip | ChecksumOffloadOptions.tcp | ChecksumOffloadOptions.outerip`). The reason I think it is a good idea to leave it as a Flag instead of an Enum however is because it would make it so that if a user wanted to enable multiple offloads at once they would only have to call this method one time rather than once for each offload they want to enable. You wouldn't be able to do that directly using the string version of the flag like this however, I think the easiest way to do it would be a for loop like this: for offload in ChecksumOffloadOptions.__members__: if offload in layer: csum_output =3D self.send_command(f"csum set {offload.name.replace("_", "-")} hw {port_id}") if verify: ... ... ... So basically the same as what you have already, but instead you call the command for each layer that is specified in the options instead of trying to convert the Flag to a string. Then someone can call this method with the example combination I gave above and enable all 3 offloads in one call. I'm not sure if the flags have a way to extract all of the values that are within an instance of the flag, but that would also make it easier since you wouldn't have to loop through all of the ChecksumOffloadOptions.__members__. > + if verify: > + if "Bad arguments" in csum_output or f"Please stop port {por= t_id} first" in csum_output: > + self._logger.debug(f"Failed to set csum hw mode on port = {port_id}:\n{csum_output}") > + raise InteractiveCommandExecutionError( > + f"Failed to set csum hw mode on port {port_id}" > + ) > + if verify and f"checksum offload is not supported by port {port_= id}" in csum_output: > + self._logger.debug(f"Checksum {layer} offload is not support= ed:\n{csum_output}") > + > + success =3D False > + if layer =3D=3D ChecksumOffloadOptions.outerip: > + if "Outer-Ip checksum offload is hw" in csum_output: > + success =3D True > + elif layer =3D=3D ChecksumOffloadOptions.outerudp: > + if "Outer-Udp checksum offload is hw" in csum_output: > + success =3D True > + else: > + if f"{str(layer).upper} checksum offload is hw" in csum_outp= ut: The call to `upper` here is missing the parenthesis so this might not be producing the right string. > + success =3D True > + if not success and verify: > + self._logger.debug(f"Failed to set csum hw mode on port {por= t_id}:\n{csum_output}") I think these if-statements would also be simplified if you did it in a for-loop since all you would have to do is: if "-" in offload.name and f"{offload.name.title()} offload is hw" not in csum_output: ... elif f"{offload.name.upper()} offload is hw" not in csum_output: ... I honestly didn't know the `title()` method of a string existed in python until I just did a little searching and it seems strange to me, but it would be helpful for this use case. It also is weird to me that they would have everything other than outer-ip and outer-udp be all upper case. > + > + def set_verbose(self, level: int, verify: bool =3D True) -> None: > -- > 2.44.0 >