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 01D8F45899; Thu, 29 Aug 2024 17:40:50 +0200 (CEST) Received: from mails.dpdk.org (localhost [127.0.0.1]) by mails.dpdk.org (Postfix) with ESMTP id 7235740279; Thu, 29 Aug 2024 17:40:50 +0200 (CEST) Received: from mail-pg1-f180.google.com (mail-pg1-f180.google.com [209.85.215.180]) by mails.dpdk.org (Postfix) with ESMTP id CE94A4026A for ; Thu, 29 Aug 2024 17:40:48 +0200 (CEST) Received: by mail-pg1-f180.google.com with SMTP id 41be03b00d2f7-7cd835872ceso536674a12.3 for ; Thu, 29 Aug 2024 08:40:48 -0700 (PDT) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=iol.unh.edu; s=unh-iol; t=1724946048; x=1725550848; 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=jUHnvhiAmqOHPgfJ2m284nNS4oNsDuxklF/KhSYLRII=; b=T6ydARmxfP5KEdUcsBfCTRKDNgAKa4jsMG2TT5DlvWS+TPd/VzicpK01wgmruTFAhM u/EKIXjTzuoppb+BeBVRsd9R4n2H6nEJ5yH4RxhR7cKzwUHOvaj6Z6XrmrYNNk5ESSE9 8c76qS9E4Xq8rNkxdyzhIRLiLM9iGA55M8zd0= X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20230601; t=1724946048; x=1725550848; 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=jUHnvhiAmqOHPgfJ2m284nNS4oNsDuxklF/KhSYLRII=; b=cwbJu5TIZEUh36IgXou9rMqEc4yFCx1fPp4H/suyKPxhzsiiQXopxILYT56sAsQzly X1qVN8hRACl89gDiNqaqxqgVj1S5NoNRVEZf5w8Iyi6RAPb6y8+QJ4WyTxpwLWnQSkUV 1ynVoQMZU7+bxGqloMWG3nbGPlMiqW1TYVYr/1it1gJfGeh47aCnbNI8tUsJ05VDUw4f TfZU3dgvVSFdKEJse04f2iZKnThNI9ll9UqLFiJ2wzj9p1+oOrEaPWLi+0FJpUhrVmuc eTNlIcfj9IZySI+BUfMe07PsERKiqTzRjhP5aeWTf9mxlbQieAHcHeZNw3OtpO+G3luK c7wQ== X-Forwarded-Encrypted: i=1; AJvYcCUe/GkwAeADOemVpOFYI9NjoNVn+OssJLQRRk34x5xJTm5KoWcjq4XWHgCc9xE6ni3AyFs=@dpdk.org X-Gm-Message-State: AOJu0YwJiO2h/QShYxbzAgBnob8IBCZxl+WeJJqJEMqrGT2+rxIRGQ1t QRekk8R7s86cWfW1VSZFtAcnFtuE6jIjw205OC8b/1EkMUG7LPLCFFr4oi2DmItJVl7QuhDx0rc 7+tAJSJny6Oo8GB06/FiI2olWSiaD0vB9ODcKDw== X-Google-Smtp-Source: AGHT+IEqHlkSHizXFbICKgz8FMOqPZd3Mdaql8Mc6VrMyzbhkrgM+Afj89zFUC85uEhJlHySKBt8iejfc6QMMHG/ALY= X-Received: by 2002:a17:90b:4acb:b0:2cb:4e14:fd5d with SMTP id 98e67ed59e1d1-2d8561a3cb0mr3069012a91.17.1724946047584; Thu, 29 Aug 2024 08:40:47 -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> In-Reply-To: From: Jeremy Spewock Date: Thu, 29 Aug 2024 11:40:36 -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, 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/framew= ork/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 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. > > > + #: 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 N= IC. > > + 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_CKS= UM | 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 > > + ) > > >