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 16A0B459CE; Wed, 18 Sep 2024 18:57:33 +0200 (CEST) Received: from mails.dpdk.org (localhost [127.0.0.1]) by mails.dpdk.org (Postfix) with ESMTP id A405D4025E; Wed, 18 Sep 2024 18:57:32 +0200 (CEST) Received: from mail-pj1-f52.google.com (mail-pj1-f52.google.com [209.85.216.52]) by mails.dpdk.org (Postfix) with ESMTP id 9FC424003C for ; Wed, 18 Sep 2024 18:57:31 +0200 (CEST) Received: by mail-pj1-f52.google.com with SMTP id 98e67ed59e1d1-2d87a0bfaa7so4710529a91.2 for ; Wed, 18 Sep 2024 09:57:31 -0700 (PDT) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=iol.unh.edu; s=unh-iol; t=1726678651; x=1727283451; 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=1dohNdla5BW99PzpM/QpmM8yCVkGC3M68kZ35PehbVk=; b=Mt8eySvpajmcBrokWHixmYBGCKnEtebZSTe+Ulmt5adcy2FbU1qZZZiDxjpG5ZZJCQ k06b1pvHMtBKqw8VN75WFn13IFEXeG/VQkDgH60dP7uKCTUV0rJxxrIsw3rkOco88bgQ TLYGshke1sU0PMZZ5a0GQOHvAudk2/0be6m+M= X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20230601; t=1726678651; x=1727283451; 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=1dohNdla5BW99PzpM/QpmM8yCVkGC3M68kZ35PehbVk=; b=KWyH21QuJuV3LNV9zX3ul3jDoxZI5k1AIEofpONQGDe20zZ7tQaAWPC9AbjvCkaRgI UJ0Gr7QWPTggQrXbj50j+Sv748XdwMgmHeufwkb0ZudUcYk/rSwsLh+Kr6auwjpNRSKj +4U6b2Vbm6xFovaRHksapaUznPsIw7sF/mw5KKfc5viJS2AksnuLtUpjzmtfjzm80ZK2 SQSSDNNam/+t4H/BG25R97k6WczOulQHxfQmLY30ahxpUeKvIPQJzR5w/wUZ6h2iOj7i KBGQj+DJmr6rJpZetN728Wa1Sovysf47JXdAZrNmz2dR+J0HB9x699p2tzL6Swlg5fKB 1UHA== X-Forwarded-Encrypted: i=1; AJvYcCWpIEwfYokMpT2FX2fBNkw0Ep4Yp2r2UcXTMpinIYehKY/lJzYm39Twgf0yJKwTPd+DsW0=@dpdk.org X-Gm-Message-State: AOJu0YzA6QST86bp9lBbLTR3Jldf4lEARD/f5V54I3ztzp/Qx9lkcxyw 2TvOayPzcshjgD3H/YVp+Tqudd+YomGKUu4Rf553h2eQBFU3yLIPGr0Rr/pLFh3ilAE6E1Tc24k Ht5W0yX0JabwX6kxHLlkq8TQmTFneMDqMfTQkQA== X-Google-Smtp-Source: AGHT+IH3qTXzde76mMOqhKnACouno9CoLQCTYLe1lugZt/21/V5npauo++1smsSKzRZNU1lHMNql6IL3tRU+QOlj4Bw= X-Received: by 2002:a17:90b:4b03:b0:2c9:7611:e15d with SMTP id 98e67ed59e1d1-2dbb9e25983mr25631564a91.20.1726678650627; Wed, 18 Sep 2024 09:57:30 -0700 (PDT) MIME-Version: 1.0 References: <20240301155416.96960-1-juraj.linkes@pantheon.tech> <20240821145315.97974-1-juraj.linkes@pantheon.tech> <20240821145315.97974-12-juraj.linkes@pantheon.tech> <48bee8c4-42a4-4f94-bbda-9ac3a76bcaa4@pantheon.tech> In-Reply-To: <48bee8c4-42a4-4f94-bbda-9ac3a76bcaa4@pantheon.tech> From: Jeremy Spewock Date: Wed, 18 Sep 2024 12:57:19 -0400 Message-ID: Subject: Re: [PATCH v3 11/12] dts: add Rx offload capabilities To: =?UTF-8?Q?Juraj_Linke=C5=A1?= Cc: thomas@monjalon.net, Honnappa.Nagarahalli@arm.com, probb@iol.unh.edu, paul.szczepanek@arm.com, Luca.Vizzarro@arm.com, npratte@iol.unh.edu, dmarx@iol.unh.edu, alex.chapman@arm.com, 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, Sep 18, 2024 at 10:27=E2=80=AFAM Juraj Linke=C5=A1 wrote: > > > > On 29. 8. 2024 17:40, Jeremy Spewock wrote: > > On Wed, Aug 28, 2024 at 1:44=E2=80=AFPM Jeremy Spewock wrote: > >> > >> On Wed, Aug 21, 2024 at 10:53=E2=80=AFAM Juraj Linke=C5=A1 > >> wrote: > >> > >>> diff --git a/dts/framework/remote_session/testpmd_shell.py b/dts/fram= ework/remote_session/testpmd_shell.py > >>> index 48c31124d1..f83569669e 100644 > >>> --- a/dts/framework/remote_session/testpmd_shell.py > >>> +++ b/dts/framework/remote_session/testpmd_shell.py > >>> @@ -659,6 +659,103 @@ class TestPmdPortStats(TextParser): > >>> tx_bps: int =3D field(metadata=3DTextParser.find_int(r"Tx-bps:\= s+(\d+)")) > >>> > >>> > >>> +class RxOffloadCapability(Flag): > >>> + """Rx offload capabilities of a device.""" > >>> + > >>> + #: > >>> + RX_OFFLOAD_VLAN_STRIP =3D auto() > >> > >> One other thought that I had about this; was there a specific reason > >> that you decided to prefix all of these with `RX_OFFLOAD_`? I am > >> working on a test suite right now that uses both RX and TX offloads > >> and thought that it would be a great use of capabilities, so I am > >> working on adding a TxOffloadCapability flag as well and, since the > >> output is essentially the same, it made a lot of sense to make it a > >> sibling class of this one with similar parsing functionality. In what > >> I was writing, I found it much easier to remove this prefix so that > >> the parsing method can be the same for both RX and TX, and I didn't > >> have to restate some options that are shared between both (like > >> IPv4_CKSUM, UDP_CKSUM, etc.). Is there a reason you can think of why > >> removing this prefix is a bad idea? Hopefully I will have a patch out > >> soon that shows this extension that I've made so that you can see > >> in-code what I was thinking. > > > > I see now that you actually already answered this question, I was just > > looking too much at that piece of code, and clearly not looking > > further down at the helper-method mapping or the commit message that > > you left :). > > > > "The Flag members correspond to NIC > > capability names so a convenience function that looks for the supported > > Flags in a testpmd output is also added." > > > > Having it prefixed with RX_OFFLOAD_ in NicCapability makes a lot of > > sense since it is more explicit. Since there is a good reason to have > > it like this, then the redundancy makes sense I think. There are some > > ways to potentially avoid this like creating a StrFlag class that > > overrides the __str__ method, or something like an additional type > > that would contain a toString method, but it feels very situational > > and specific to this one use-case so it probably isn't going to be > > super valuable. Another thing I could think of to do would be allowing > > the user to pass in a function or something to the helper-method that > > mapped Flag names to their respective NicCapability name, or just > > doing it in the method that gets the offloads instead of using a > > helper at all, but this also just makes it more complicated and maybe > > it isn't worth it. > > > > I also had it without the prefix, but then I also realized it's needed > in NicCapability so this is where I ended. I'm not sure complicating > things to remove the prefix is worth it, especially when these names are > basically only used internally. The prefix could actually confer some > benefit if the name appears in a log somewhere (although overriding > __str__ could be the way; maybe I'll think about that). It could be done with modifying str, but I found that an approach that was easier was just adding an optional prefix to the _update_capabilities_from_flag() method since you will know whether the capability is Rx or Tx at the point of calling this method. I feel like either or could work, I'm not sure exactly which is better. The change that adds the prefix is in the Rx/Tx offload suite in the first commit [1] if you wanted to look at it. This commit and the one after it are isolated to be only changes to the capabilities series. [1] https://patchwork.dpdk.org/project/dpdk/patch/20240903194642.24458-2-js= pewock@iol.unh.edu/ > > > I apologize for asking you about something that you already explained, > > but maybe something we can get out of this is that, since these names > > have to be consistent, it might be worth putting that in the > > doc-strings of the flag for when people try to make further expansions > > or changes in the future. Or it could also be generally clear that > > flags used for capabilities should follow this idea, let me know what > > you think. > > > > Adding things to docstring is usually a good thing. What should I > document? I guess the correspondence between the flag and NicCapability, > anything else? The only thing I was thinking was that the flag values have to match the values in NicCapability. I think explaining it this way is enough just to make it clear that it is done that way for a purpose and cannot be different (unless of course the no-prefix way is favorable). > > >> > >>> + #: Device supports L3 checksum offload. > >>> + RX_OFFLOAD_IPV4_CKSUM =3D auto() > >>> + #: Device supports L4 checksum offload. > >>> + RX_OFFLOAD_UDP_CKSUM =3D auto() > >>> + #: Device supports L4 checksum offload. > >>> + RX_OFFLOAD_TCP_CKSUM =3D auto() > >>> + #: Device supports Large Receive Offload. > >>> + RX_OFFLOAD_TCP_LRO =3D auto() > >>> + #: Device supports QinQ (queue in queue) offload. > >>> + RX_OFFLOAD_QINQ_STRIP =3D auto() > >>> + #: Device supports inner packet L3 checksum. > >>> + RX_OFFLOAD_OUTER_IPV4_CKSUM =3D auto() > >>> + #: Device supports MACsec. > >>> + RX_OFFLOAD_MACSEC_STRIP =3D auto() > >>> + #: Device supports filtering of a VLAN Tag identifier. > >>> + RX_OFFLOAD_VLAN_FILTER =3D 1 << 9 > >>> + #: Device supports VLAN offload. > >>> + RX_OFFLOAD_VLAN_EXTEND =3D auto() > >>> + #: Device supports receiving segmented mbufs. > >>> + RX_OFFLOAD_SCATTER =3D 1 << 13 > >>> + #: Device supports Timestamp. > >>> + RX_OFFLOAD_TIMESTAMP =3D auto() > >>> + #: Device supports crypto processing while packet is received in= NIC. > >>> + RX_OFFLOAD_SECURITY =3D auto() > >>> + #: Device supports CRC stripping. > >>> + RX_OFFLOAD_KEEP_CRC =3D auto() > >>> + #: Device supports L4 checksum offload. > >>> + RX_OFFLOAD_SCTP_CKSUM =3D auto() > >>> + #: Device supports inner packet L4 checksum. > >>> + RX_OFFLOAD_OUTER_UDP_CKSUM =3D auto() > >>> + #: Device supports RSS hashing. > >>> + RX_OFFLOAD_RSS_HASH =3D auto() > >>> + #: Device supports > >>> + RX_OFFLOAD_BUFFER_SPLIT =3D auto() > >>> + #: Device supports all checksum capabilities. > >>> + RX_OFFLOAD_CHECKSUM =3D RX_OFFLOAD_IPV4_CKSUM | RX_OFFLOAD_UDP_C= KSUM | RX_OFFLOAD_TCP_CKSUM > >>> + #: Device supports all VLAN capabilities. > >>> + RX_OFFLOAD_VLAN =3D ( > >>> + RX_OFFLOAD_VLAN_STRIP > >>> + | RX_OFFLOAD_VLAN_FILTER > >>> + | RX_OFFLOAD_VLAN_EXTEND > >>> + | RX_OFFLOAD_QINQ_STRIP > >>> + ) > >> > >>> >