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 0A038A0093; Mon, 7 Nov 2022 12:31:31 +0100 (CET) Received: from [217.70.189.124] (localhost [127.0.0.1]) by mails.dpdk.org (Postfix) with ESMTP id AAC8C40156; Mon, 7 Nov 2022 12:31:30 +0100 (CET) Received: from shelob.oktetlabs.ru (shelob.oktetlabs.ru [91.220.146.113]) by mails.dpdk.org (Postfix) with ESMTP id 4A60140151 for ; Mon, 7 Nov 2022 12:31:29 +0100 (CET) Received: from [192.168.38.17] (aros.oktetlabs.ru [192.168.38.17]) (using TLSv1.3 with cipher TLS_AES_128_GCM_SHA256 (128/128 bits) key-exchange X25519 server-signature RSA-PSS (4096 bits) server-digest SHA256) (No client certificate requested) by shelob.oktetlabs.ru (Postfix) with ESMTPSA id C5A355A; Mon, 7 Nov 2022 14:31:27 +0300 (MSK) DKIM-Filter: OpenDKIM Filter v2.11.0 shelob.oktetlabs.ru C5A355A DKIM-Signature: v=1; a=rsa-sha256; c=simple/simple; d=oktetlabs.ru; s=default; t=1667820687; bh=O9xrPi2ACXT6DIIKpAypjp2ixokobkpBfPk7DRblx34=; h=Date:Subject:To:Cc:References:From:In-Reply-To:From; b=VPbJ1ZyJUohvAtrDCUUvlZquBJlhqmzGbcwBtbb5r7dDf2fwkf/9s/hHRAklSqF8y MBdh+48F9nBG4E+qvu9ZgWDkPhXaZveu4MKAmLsdlcEHvIw2RDgpziQsVL4d3EKMKO A/Y36dkan8s7pUWczfSijC4dvkuCyClMPrL6SsvU= Message-ID: <3df1c2bc-2b1d-798e-da1f-5907bdf06746@oktetlabs.ru> Date: Mon, 7 Nov 2022 14:31:27 +0300 MIME-Version: 1.0 User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:102.0) Gecko/20100101 Thunderbird/102.4.0 Subject: Re: [PATCH v3] app/testpmd: fix protocol header display for Rx buffer split Content-Language: en-US To: Yuan Wang , aman.deep.singh@intel.com, yuying.zhang@intel.com Cc: xuan.ding@intel.com, yaqi.tang@intel.com, dev@dpdk.org References: <20221017174054.800259-1-yuanx.wang@intel.com> <20221107084506.1896422-1-yuanx.wang@intel.com> From: Andrew Rybchenko Organization: OKTET Labs In-Reply-To: <20221107084506.1896422-1-yuanx.wang@intel.com> Content-Type: text/plain; charset=UTF-8; format=flowed Content-Transfer-Encoding: 7bit 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 11/7/22 11:45, Yuan Wang wrote: > The "show config rxhdrs" cmd displays the configured protocol headers > that are used for protocol-based buffer split. > However, it shows inner-ipv6 as inner-ipv4. > > This patch fixes that by adjusting the order of condition judgments. > This patch also uses RTE_PTYPE_*_MASK as masks. > > Fixes: 52e2e7edcf48 ("app/testpmd: add protocol-based buffer split") > > Signed-off-by: Yuan Wang > > --- > v3: > - use RTE_PTYPE_*_MASK as masks. > - refactor to use switch statement. > v2: > - add fixline. > > --- > app/test-pmd/config.c | 89 +++++++++++++++++++++---------------------- > 1 file changed, 44 insertions(+), 45 deletions(-) > > diff --git a/app/test-pmd/config.c b/app/test-pmd/config.c > index e8a1b77c2a..8638dfed11 100644 > --- a/app/test-pmd/config.c > +++ b/app/test-pmd/config.c > @@ -5070,73 +5070,72 @@ show_rx_pkt_segments(void) > > static const char *get_ptype_str(uint32_t ptype) > { > - if ((ptype & (RTE_PTYPE_L3_IPV4_EXT_UNKNOWN | RTE_PTYPE_L4_TCP)) == > - (RTE_PTYPE_L3_IPV4_EXT_UNKNOWN | RTE_PTYPE_L4_TCP)) > + switch (ptype & (RTE_PTYPE_L3_MASK | RTE_PTYPE_L4_MASK)) { > + case RTE_PTYPE_L3_IPV4_EXT_UNKNOWN | RTE_PTYPE_L4_TCP: > return "ipv4-tcp"; If I map "ipv4-tcp" to packets types, I get: RTE_PTYPE_L2_ETHER | RTE_PTYPE_L3_IPV4_EXT_UNKNOWN | RTE_PTYPE_L4_TCP but vice versa it is sufficient to have just RTE_PTYPE_L3_IPV4_EXT_UNKNOWN | RTE_PTYPE_L4_TCP I think such asymmetry in mapping is bad. > - else if ((ptype & (RTE_PTYPE_L3_IPV4_EXT_UNKNOWN | RTE_PTYPE_L4_UDP)) == > - (RTE_PTYPE_L3_IPV4_EXT_UNKNOWN | RTE_PTYPE_L4_UDP)) > + case RTE_PTYPE_L3_IPV4_EXT_UNKNOWN | RTE_PTYPE_L4_UDP: > return "ipv4-udp"; > - else if ((ptype & (RTE_PTYPE_L3_IPV4_EXT_UNKNOWN | RTE_PTYPE_L4_SCTP)) == > - (RTE_PTYPE_L3_IPV4_EXT_UNKNOWN | RTE_PTYPE_L4_SCTP)) > + case RTE_PTYPE_L3_IPV4_EXT_UNKNOWN | RTE_PTYPE_L4_SCTP: > return "ipv4-sctp"; > - else if ((ptype & (RTE_PTYPE_L3_IPV6_EXT_UNKNOWN | RTE_PTYPE_L4_TCP)) == > - (RTE_PTYPE_L3_IPV6_EXT_UNKNOWN | RTE_PTYPE_L4_TCP)) > + case RTE_PTYPE_L3_IPV6_EXT_UNKNOWN | RTE_PTYPE_L4_TCP: > return "ipv6-tcp"; > - else if ((ptype & (RTE_PTYPE_L3_IPV6_EXT_UNKNOWN | RTE_PTYPE_L4_UDP)) == > - (RTE_PTYPE_L3_IPV6_EXT_UNKNOWN | RTE_PTYPE_L4_UDP)) > + case RTE_PTYPE_L3_IPV6_EXT_UNKNOWN | RTE_PTYPE_L4_UDP: > return "ipv6-udp"; > - else if ((ptype & (RTE_PTYPE_L3_IPV6_EXT_UNKNOWN | RTE_PTYPE_L4_SCTP)) == > - (RTE_PTYPE_L3_IPV6_EXT_UNKNOWN | RTE_PTYPE_L4_SCTP)) > + case RTE_PTYPE_L3_IPV6_EXT_UNKNOWN | RTE_PTYPE_L4_SCTP: > return "ipv6-sctp"; > - else if ((ptype & RTE_PTYPE_L4_TCP) == RTE_PTYPE_L4_TCP) > + case RTE_PTYPE_L4_TCP: > return "tcp"; > - else if ((ptype & RTE_PTYPE_L4_UDP) == RTE_PTYPE_L4_UDP) > + case RTE_PTYPE_L4_UDP: > return "udp"; > - else if ((ptype & RTE_PTYPE_L4_SCTP) == RTE_PTYPE_L4_SCTP) > + case RTE_PTYPE_L4_SCTP: > return "sctp"; > - else if ((ptype & RTE_PTYPE_L3_IPV4_EXT_UNKNOWN) == RTE_PTYPE_L3_IPV4_EXT_UNKNOWN) > + case RTE_PTYPE_L3_IPV4_EXT_UNKNOWN: > return "ipv4"; > - else if ((ptype & RTE_PTYPE_L3_IPV6_EXT_UNKNOWN) == RTE_PTYPE_L3_IPV6_EXT_UNKNOWN) > + case RTE_PTYPE_L3_IPV6_EXT_UNKNOWN: > return "ipv6"; > - else if ((ptype & RTE_PTYPE_L2_ETHER) == RTE_PTYPE_L2_ETHER) > + } > + > + switch (ptype & RTE_PTYPE_L2_MASK) { Having many switches here looks confusing. Who defines priorities? IMHO it should be single switch here and values should be in exactly the same order as get_ptype(). Ideally both function should be close to each other. > + case RTE_PTYPE_L2_ETHER: > return "eth"; > + } > > - else if ((ptype & (RTE_PTYPE_INNER_L3_IPV4_EXT_UNKNOWN | RTE_PTYPE_INNER_L4_TCP)) == > - (RTE_PTYPE_INNER_L3_IPV4_EXT_UNKNOWN | RTE_PTYPE_INNER_L4_TCP)) > - return "inner-ipv4-tcp"; > - else if ((ptype & (RTE_PTYPE_INNER_L3_IPV4_EXT_UNKNOWN | RTE_PTYPE_INNER_L4_UDP)) == > - (RTE_PTYPE_INNER_L3_IPV4_EXT_UNKNOWN | RTE_PTYPE_INNER_L4_UDP)) > - return "inner-ipv4-udp"; > - else if ((ptype & (RTE_PTYPE_INNER_L3_IPV4_EXT_UNKNOWN | RTE_PTYPE_INNER_L4_SCTP)) == > - (RTE_PTYPE_INNER_L3_IPV4_EXT_UNKNOWN | RTE_PTYPE_INNER_L4_SCTP)) > - return "inner-ipv4-sctp"; > - else if ((ptype & (RTE_PTYPE_INNER_L3_IPV6_EXT_UNKNOWN | RTE_PTYPE_INNER_L4_TCP)) == > - (RTE_PTYPE_INNER_L3_IPV6_EXT_UNKNOWN | RTE_PTYPE_INNER_L4_TCP)) > + switch (ptype & (RTE_PTYPE_INNER_L3_MASK | RTE_PTYPE_INNER_L4_MASK)) { > + case RTE_PTYPE_INNER_L3_IPV6_EXT_UNKNOWN | RTE_PTYPE_INNER_L4_TCP: > return "inner-ipv6-tcp"; get_ptype(): inner-ipv6-tcp -> RTE_PTYPE_TUNNEL_GRENAT | RTE_PTYPE_INNER_L2_ETHER | RTE_PTYPE_INNER_L3_IPV6_EXT_UNKNOWN | RTE_PTYPE_INNER_L4_TCP So, mapping is asymmetric again. Out of topic for the patch: Also I'm wondering why inner-ipv6-tcp is a grenat. Why not VxLAN, not GENEVE? > - else if ((ptype & (RTE_PTYPE_INNER_L3_IPV6_EXT_UNKNOWN | RTE_PTYPE_INNER_L4_UDP)) == > - (RTE_PTYPE_INNER_L3_IPV6_EXT_UNKNOWN | RTE_PTYPE_INNER_L4_UDP)) > + case RTE_PTYPE_INNER_L3_IPV6_EXT_UNKNOWN | RTE_PTYPE_INNER_L4_UDP: > return "inner-ipv6-udp"; > - else if ((ptype & (RTE_PTYPE_INNER_L3_IPV6_EXT_UNKNOWN | RTE_PTYPE_INNER_L4_SCTP)) == > - (RTE_PTYPE_INNER_L3_IPV6_EXT_UNKNOWN | RTE_PTYPE_INNER_L4_SCTP)) > + case RTE_PTYPE_INNER_L3_IPV6_EXT_UNKNOWN | RTE_PTYPE_INNER_L4_SCTP: > return "inner-ipv6-sctp"; > - else if ((ptype & RTE_PTYPE_INNER_L4_TCP) == RTE_PTYPE_INNER_L4_TCP) > + case RTE_PTYPE_INNER_L3_IPV4_EXT_UNKNOWN | RTE_PTYPE_INNER_L4_TCP: > + return "inner-ipv4-tcp"; > + case RTE_PTYPE_INNER_L3_IPV4_EXT_UNKNOWN | RTE_PTYPE_INNER_L4_UDP: > + return "inner-ipv4-udp"; > + case RTE_PTYPE_INNER_L3_IPV4_EXT_UNKNOWN | RTE_PTYPE_INNER_L4_SCTP: > + return "inner-ipv4-sctp"; > + case RTE_PTYPE_INNER_L4_TCP: > return "inner-tcp"; > - else if ((ptype & RTE_PTYPE_INNER_L4_UDP) == RTE_PTYPE_INNER_L4_UDP) > + case RTE_PTYPE_INNER_L4_UDP: > return "inner-udp"; > - else if ((ptype & RTE_PTYPE_INNER_L4_SCTP) == RTE_PTYPE_INNER_L4_SCTP) > + case RTE_PTYPE_INNER_L4_SCTP: > return "inner-sctp"; > - else if ((ptype & RTE_PTYPE_INNER_L3_IPV4_EXT_UNKNOWN) == > - RTE_PTYPE_INNER_L3_IPV4_EXT_UNKNOWN) > - return "inner-ipv4"; > - else if ((ptype & RTE_PTYPE_INNER_L3_IPV6_EXT_UNKNOWN) == > - RTE_PTYPE_INNER_L3_IPV4_EXT_UNKNOWN) > + case RTE_PTYPE_INNER_L3_IPV6_EXT_UNKNOWN: > return "inner-ipv6"; > - else if ((ptype & RTE_PTYPE_INNER_L2_ETHER) == RTE_PTYPE_INNER_L2_ETHER) > + case RTE_PTYPE_INNER_L3_IPV4_EXT_UNKNOWN: > + return "inner-ipv4"; > + } > + > + switch (ptype & RTE_PTYPE_INNER_L2_MASK) { > + case RTE_PTYPE_INNER_L2_ETHER: > return "inner-eth"; > - else if ((ptype & RTE_PTYPE_TUNNEL_GRENAT) == RTE_PTYPE_TUNNEL_GRENAT) > + } > + > + switch (ptype & RTE_PTYPE_TUNNEL_MASK) { > + case RTE_PTYPE_TUNNEL_GRENAT: > return "grenat"; > - else > - return "unsupported"; > + } > + > + return "unsupported"; > } > > void