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 6A3754586E; Mon, 26 Aug 2024 23:04:35 +0200 (CEST) Received: from mails.dpdk.org (localhost [127.0.0.1]) by mails.dpdk.org (Postfix) with ESMTP id 4589A40A7A; Mon, 26 Aug 2024 23:04:35 +0200 (CEST) Received: from mail-pf1-f180.google.com (mail-pf1-f180.google.com [209.85.210.180]) by mails.dpdk.org (Postfix) with ESMTP id 103C4400EF for ; Mon, 26 Aug 2024 23:04:34 +0200 (CEST) Received: by mail-pf1-f180.google.com with SMTP id d2e1a72fcca58-713edc53429so3528222b3a.2 for ; Mon, 26 Aug 2024 14:04:33 -0700 (PDT) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=iol.unh.edu; s=unh-iol; t=1724706273; x=1725311073; darn=dpdk.org; h=cc:to:subject:message-id:date:from:in-reply-to:references :mime-version:from:to:cc:subject:date:message-id:reply-to; bh=DAAZTEzf80Jcw10pHN1huPJQmslXWse7wlbVCg1MZ5Q=; b=ZrskkajUgBOtsMUxgw+lenTzutmi7si2hWmSSu/RRuLBYap+y9vne5DU4UnpBJVWPD TNu98Vf2g3s1v+WXM8cALHU+wbqOwdtUPlWubJA8wmTh2FOrufBf466BPFp0s/1+0M/G 3aTEUeAZhWXmHB0cI/YE/brSpEyNTlIByM2kA= X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20230601; t=1724706273; x=1725311073; h=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=DAAZTEzf80Jcw10pHN1huPJQmslXWse7wlbVCg1MZ5Q=; b=cb8mN3zUeb+gZX9wL1P0DlfZ7f43Filk++Vyq3k45dSXwI22K/Jb00QX+Mm1a/xR5S jvZUg8X/ga+6ByIObLqg3UH0Xr4ghQYIjgdwTP2D4AfVsimxLHjq+ffAnz9udXxT3EyR lOxBCrlMwoQqPz2GWXOL+5N+akJzQsDjF3Cu4vkpz0LlFMlQwiciaGFHN3fdxIV9u98q RMOGNCOuI+fpzmc3S1f2IqQKoX1TcrSCIb/hzjQI+FC3hdrPD8bR5QIftfjDMxT2yPRr hf8Re36hiUi8oykfs3I9E+PEklrsAO0F2DoqwPeJZN/cI/KBiqZpNnEEYm1k5YW/KGUI NTow== X-Forwarded-Encrypted: i=1; AJvYcCVL/NKo8eB5e/nLlmmkyUaptI8vYFKzPan6Uum44G3ofB39xM3VGqwF/sPfalePjwMXPOI=@dpdk.org X-Gm-Message-State: AOJu0YwX8ZxJpD96zvjzp3+ow+qbU/Upd/4fB7Tk18212E0x7u8tuyhK sfIW5beRL1uom9UQ4O7dzZauzTQhAv8P+qrIKd8raPv9bsBHRKciNad6iC4cpka2GUkUTpo3uA1 PG+XQsbJffWoKv6rVAITuFq0p9dncHbvbZUuf1A== X-Google-Smtp-Source: AGHT+IGUELmEbATp7mGqdoLr7e1/JjE6sAHx4QtgfwgBbrKTfcyQqMDOmoZbMMDLcJ489KESDUqyvMTPk9Af1hKJb04= X-Received: by 2002:a05:6a20:6f8b:b0:1c6:91e1:f0fc with SMTP id adf61e73a8af0-1ccc09ac26dmr966205637.48.1724706272996; Mon, 26 Aug 2024 14:04:32 -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: From: Dean Marx Date: Mon, 26 Aug 2024 17:04:44 -0400 Message-ID: Subject: Re: [PATCH v2 1/2] dts: add csum HW offload to testpmd shell To: Jeremy Spewock 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: multipart/alternative; boundary="000000000000be8d6806209c7871" 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 --000000000000be8d6806209c7871 Content-Type: text/plain; charset="UTF-8" Content-Transfer-Encoding: quoted-printable On Fri, Aug 23, 2024 at 10:54=E2=80=AFAM Jeremy Spewock wrote: > On Wed, Aug 21, 2024 at 12:25=E2=80=AFPM Dean Marx wr= ote: > > > > 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. > I actually like that idea a lot, I'm going to add the dependency and remove the corresponding methods, especially since it probably makes the maintainer's jobs easier when there's less code duplication. I can also send a message in the slack chat about this to see what other people think. > > +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. > Makes sense, I ended up just getting rid of the __str__ method entirely and iterating through the options within the csum set hw method with the __members__ method you mentioned later in this review. I was able to create a for loop that looks like this: for name, offload in ChecksumOffloadOptions.__members__.items(): if offload in layer: (action) where .items() returns all the flags in a dictionary, where the key is a string of the flag name and the offload value is the actual flag instance from the class. This way I could just call name =3D name.replace("_", "-") within the loop and use name for send_command and offload for comparing flags. > 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. Yeah it is really odd, I'm not sure if they had consistency in mind while developing this part of testpmd. The title command is a great idea though, I added that to the second part of the csum method and it really simplified everything. --000000000000be8d6806209c7871 Content-Type: text/html; charset="UTF-8" Content-Transfer-Encoding: quoted-printable
On Fri, Aug 23, 2024 at 10:54=E2=80=AFAM = Jeremy Spewock <jspewock@iol.unh= .edu> wrote:
On Wed, Aug 21, 2024 at 12:25=E2=80=AFPM D= ean Marx <dmarx@i= ol.unh.edu> wrote:
>
> 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.
<snip>

= I actually like that idea a lot, I'm going to add the dependency and re= move the corresponding methods, especially since it probably makes the main= tainer's jobs easier when there's less code duplication. I can also= send a message in the slack chat about this to see what other people think= .
=C2=A0
> +class ChecksumOffloadOptions(Flag):
> +=C2=A0 =C2=A0 """Flag representing checksum hardware o= ffload layer options."""
> +
> +=C2=A0 =C2=A0 #:
> +=C2=A0 =C2=A0 ip =3D auto()
> +=C2=A0 =C2=A0 #:
> +=C2=A0 =C2=A0 udp =3D auto()
> +=C2=A0 =C2=A0 #:
> +=C2=A0 =C2=A0 tcp =3D auto()
> +=C2=A0 =C2=A0 #:
> +=C2=A0 =C2=A0 sctp =3D auto()
> +=C2=A0 =C2=A0 #:
> +=C2=A0 =C2=A0 outerip =3D auto()
> +=C2=A0 =C2=A0 #:
> +=C2=A0 =C2=A0 outerudp =3D auto()
> +
> +=C2=A0 =C2=A0 def __str__(self):
> +=C2=A0 =C2=A0 =C2=A0 =C2=A0 """String method for use i= n csum_set_hw."""
> +=C2=A0 =C2=A0 =C2=A0 =C2=A0 if self =3D=3D ChecksumOffloadOptions.out= erip:
> +=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 return "outer-ip"=
> +=C2=A0 =C2=A0 =C2=A0 =C2=A0 elif self =3D=3D ChecksumOffloadOptions.o= uterudp:
> +=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 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.

Makes sense, I ended up j= ust getting rid of the __str__ method entirely and iterating through the op= tions within the csum set hw method with the __members__ method you mention= ed later in this review. I was able to create a for loop that looks like th= is:

for name, offload in ChecksumOffloadOptions.__= members__.items():
=C2=A0 =C2=A0 =C2=A0 =C2=A0 if offload in laye= r:
=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0(action= )

where .items() returns all the flags in a dictio= nary, where the key is a string of the flag name and the offload value is t= he actual flag instance from the class. This way I could just call name =3D= name.replace("_", "-") within the loop and use name fo= r send_command and offload for comparing flags.
=C2=A0
= <snip>
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.

=C2=A0Yeah it is really odd, I&= #39;m not sure if they had consistency in mind while developing this part o= f testpmd. The title command is a great idea though, I added that to the se= cond part of the csum method and it really simplified everything.=C2=A0
--000000000000be8d6806209c7871--