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 42535459CC; Wed, 18 Sep 2024 16:27:33 +0200 (CEST) Received: from mails.dpdk.org (localhost [127.0.0.1]) by mails.dpdk.org (Postfix) with ESMTP id 17AB6427C2; Wed, 18 Sep 2024 16:27:33 +0200 (CEST) Received: from mail-ed1-f46.google.com (mail-ed1-f46.google.com [209.85.208.46]) by mails.dpdk.org (Postfix) with ESMTP id 048584025E for ; Wed, 18 Sep 2024 16:27:32 +0200 (CEST) Received: by mail-ed1-f46.google.com with SMTP id 4fb4d7f45d1cf-5c42e7adbe0so4647744a12.2 for ; Wed, 18 Sep 2024 07:27:32 -0700 (PDT) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=pantheon.tech; s=google; t=1726669651; x=1727274451; 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=eBkpPdk6iehSb7OF+UCd+X+8T8ydgN5FD6Bfcrgb7Gs=; b=nn52is/SKSJvSWprCvzB++vZMy0lpDCgtzImCAXFW46yj8dvHAEqt+uMJYDeOnz+kO hj14rdNaLWrmN41M88OSiOjGC9+UN6VEdr6P9RVcEvAlvdSS9vEx3Y8un5cHSRr1QegO HpTsvywogtFDv+/n3s0gOR8JIwF15qZr4mAdyvs36qkqe0xxay1Pc0iQB3N78umNIqs7 s8C2FrvgMZkCHuh7fgyb1XWJKWwunpIosTJnWBfyBGEDR+Sc0VHIfyzTYWsrTsBY5y9K MftXs25B/GH6jGhVns3nE3ut0UGty5l0Nz0Dmg1v/U+frJpEntvraWjb7T4EmTAz9bYk SNOQ== X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20230601; t=1726669651; x=1727274451; 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=eBkpPdk6iehSb7OF+UCd+X+8T8ydgN5FD6Bfcrgb7Gs=; b=hJwgnNYKdk5sUipYSgaepPUyhmUbFdC3bddmm+ARSbjH/cDpyvGn/tgFDasYHZtka7 qaoBqWtIFLFvkK2EDJFlMEqdNbKex3dvSqDrhuwZ2M7wxnELVRl3vKMPrcHtDpPUEAxp vUHMWCKL9jviEnI2IhoNwmkbjVc4MdoahvhDjJo8+phVRwufQAAr0VddLIZzXlW4zkCy VpdVHy1GtB2sIeszv9+CR6NRGT8yILyFcGnAYUCVhBkfBQtv48ENapxP+jJfx6UzUqaO GFz5RQdM1HTV7yhmGu9LdveLufbEkf1J9yhqxCWtPbsKcupw2O1s74hx3mGR1y0ZrX3q gGrQ== X-Forwarded-Encrypted: i=1; AJvYcCVLDQVOhO0I6WiIjsqvS6wZoQ13wgczV5W4f3qjdfsooIWM7GQBD8P8rgpcADOmNtHlYd4=@dpdk.org X-Gm-Message-State: AOJu0YwLpCDrT2xBGEfKx0dFDu6HdXIweX3jyfASyvifLv9xyxeJnPsV jPxiKpljV49my2WnyGi1DLmgsDBjXKY6VQSiNNKfChj+ImEDlbNFLr0t81EeMg0= X-Google-Smtp-Source: AGHT+IGcqIMMNv1vBGUVWBDUGIk67+3ZmrhU0AMWU2Ay+XT3irvVNnv29EaVmU8HOUD+vpIkUqX4Hw== X-Received: by 2002:a05:6402:524e:b0:5be:d7d8:49ad with SMTP id 4fb4d7f45d1cf-5c413e4bd32mr17667530a12.22.1726669651358; Wed, 18 Sep 2024 07:27:31 -0700 (PDT) Received: from [192.168.200.22] ([84.245.121.62]) by smtp.gmail.com with ESMTPSA id 4fb4d7f45d1cf-5c42bb49770sm5002781a12.12.2024.09.18.07.27.30 (version=TLS1_3 cipher=TLS_AES_128_GCM_SHA256 bits=128/128); Wed, 18 Sep 2024 07:27:31 -0700 (PDT) Message-ID: <48bee8c4-42a4-4f94-bbda-9ac3a76bcaa4@pantheon.tech> Date: Wed, 18 Sep 2024 16:27:29 +0200 MIME-Version: 1.0 User-Agent: Mozilla Thunderbird Subject: Re: [PATCH v3 11/12] dts: add Rx offload capabilities To: Jeremy Spewock 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 References: <20240301155416.96960-1-juraj.linkes@pantheon.tech> <20240821145315.97974-1-juraj.linkes@pantheon.tech> <20240821145315.97974-12-juraj.linkes@pantheon.tech> Content-Language: en-US From: =?UTF-8?Q?Juraj_Linke=C5=A1?= In-Reply-To: Content-Type: text/plain; charset=UTF-8; format=flowed Content-Transfer-Encoding: 8bit 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 29. 8. 2024 17:40, Jeremy Spewock wrote: > On Wed, Aug 28, 2024 at 1:44 PM Jeremy Spewock wrote: >> >> On Wed, Aug 21, 2024 at 10:53 AM Juraj Linkeš >> wrote: >> >>> diff --git a/dts/framework/remote_session/testpmd_shell.py b/dts/framework/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 = field(metadata=TextParser.find_int(r"Tx-bps:\s+(\d+)")) >>> >>> >>> +class RxOffloadCapability(Flag): >>> + """Rx offload capabilities of a device.""" >>> + >>> + #: >>> + RX_OFFLOAD_VLAN_STRIP = 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). > 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? >> >>> + #: Device supports L3 checksum offload. >>> + RX_OFFLOAD_IPV4_CKSUM = auto() >>> + #: Device supports L4 checksum offload. >>> + RX_OFFLOAD_UDP_CKSUM = auto() >>> + #: Device supports L4 checksum offload. >>> + RX_OFFLOAD_TCP_CKSUM = auto() >>> + #: Device supports Large Receive Offload. >>> + RX_OFFLOAD_TCP_LRO = auto() >>> + #: Device supports QinQ (queue in queue) offload. >>> + RX_OFFLOAD_QINQ_STRIP = auto() >>> + #: Device supports inner packet L3 checksum. >>> + RX_OFFLOAD_OUTER_IPV4_CKSUM = auto() >>> + #: Device supports MACsec. >>> + RX_OFFLOAD_MACSEC_STRIP = auto() >>> + #: Device supports filtering of a VLAN Tag identifier. >>> + RX_OFFLOAD_VLAN_FILTER = 1 << 9 >>> + #: Device supports VLAN offload. >>> + RX_OFFLOAD_VLAN_EXTEND = auto() >>> + #: Device supports receiving segmented mbufs. >>> + RX_OFFLOAD_SCATTER = 1 << 13 >>> + #: Device supports Timestamp. >>> + RX_OFFLOAD_TIMESTAMP = auto() >>> + #: Device supports crypto processing while packet is received in NIC. >>> + RX_OFFLOAD_SECURITY = auto() >>> + #: Device supports CRC stripping. >>> + RX_OFFLOAD_KEEP_CRC = auto() >>> + #: Device supports L4 checksum offload. >>> + RX_OFFLOAD_SCTP_CKSUM = auto() >>> + #: Device supports inner packet L4 checksum. >>> + RX_OFFLOAD_OUTER_UDP_CKSUM = auto() >>> + #: Device supports RSS hashing. >>> + RX_OFFLOAD_RSS_HASH = auto() >>> + #: Device supports >>> + RX_OFFLOAD_BUFFER_SPLIT = auto() >>> + #: Device supports all checksum capabilities. >>> + RX_OFFLOAD_CHECKSUM = RX_OFFLOAD_IPV4_CKSUM | RX_OFFLOAD_UDP_CKSUM | RX_OFFLOAD_TCP_CKSUM >>> + #: Device supports all VLAN capabilities. >>> + RX_OFFLOAD_VLAN = ( >>> + RX_OFFLOAD_VLAN_STRIP >>> + | RX_OFFLOAD_VLAN_FILTER >>> + | RX_OFFLOAD_VLAN_EXTEND >>> + | RX_OFFLOAD_QINQ_STRIP >>> + ) >> >>>