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 C0F97A0C4A; Tue, 13 Jul 2021 12:24:20 +0200 (CEST) Received: from [217.70.189.124] (localhost [127.0.0.1]) by mails.dpdk.org (Postfix) with ESMTP id 86D75410E6; Tue, 13 Jul 2021 12:24:20 +0200 (CEST) Received: from shelob.oktetlabs.ru (shelob.oktetlabs.ru [91.220.146.113]) by mails.dpdk.org (Postfix) with ESMTP id 56D3840687 for ; Tue, 13 Jul 2021 12:24:19 +0200 (CEST) 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 (2048 bits) server-digest SHA256) (No client certificate requested) by shelob.oktetlabs.ru (Postfix) with ESMTPSA id D74827F66F; Tue, 13 Jul 2021 13:24:18 +0300 (MSK) DKIM-Filter: OpenDKIM Filter v2.11.0 shelob.oktetlabs.ru D74827F66F DKIM-Signature: v=1; a=rsa-sha256; c=simple/simple; d=oktetlabs.ru; s=default; t=1626171858; bh=cngiN26I8N8gAxGn1MTNsXjwlgmfdQ4ZWAzsbHPOvgs=; h=Subject:To:Cc:References:From:Date:In-Reply-To; b=tP1mtckJ/y+4XNsvq+0bjbR++00j9Zx4zmhYgQbB96JEYpQReN+hvuCn+p2ih2jgf ySdzCoKgXWGJJUlKXwn20GFGgsM2ggWS/xp33o+Lot1ZzQXz7fIsZaCulOdoTT2o4n nXOrZxbtGBllsmb46jqAdG9NcKEnAgNSuG2civuo= To: "Zhang, AlvinX" , "Zhang, Qi Z" , "ajit.khaparde@broadcom.com" Cc: "dev@dpdk.org" , "Singh, Aman Deep" References: <20210615081956.23656-1-alvinx.zhang@intel.com> <20210713011342.19688-1-alvinx.zhang@intel.com> <182898c1-3553-aa97-d2b5-27410a1fd00d@oktetlabs.ru> From: Andrew Rybchenko Organization: OKTET Labs Message-ID: <40d6213e-2d8f-fc95-a3ef-4c5a2a4616e9@oktetlabs.ru> Date: Tue, 13 Jul 2021 13:24:18 +0300 User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:78.0) Gecko/20100101 Thunderbird/78.11.0 MIME-Version: 1.0 In-Reply-To: Content-Type: text/plain; charset=utf-8 Content-Language: en-US Content-Transfer-Encoding: 7bit Subject: Re: [dpdk-dev] [PATCH v4] ethdev: add IPv4 and L4 checksum RSS offload types 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 Sender: "dev" On 7/13/21 12:38 PM, Zhang, AlvinX wrote: >> -----Original Message----- >> From: Andrew Rybchenko >> Sent: Tuesday, July 13, 2021 3:55 PM >> To: Zhang, AlvinX ; Zhang, Qi Z >> ; ajit.khaparde@broadcom.com >> Cc: dev@dpdk.org; Singh, Aman Deep >> Subject: Re: [PATCH v4] ethdev: add IPv4 and L4 checksum RSS offload types >> >> On 7/13/21 4:13 AM, Alvin Zhang wrote: >>> This patch defines new RSS offload types for IPv4 and >>> L4(TCP/UDP/SCTP) checksum, which are required when users want to >>> distribute packets based on the IPv4 or L4 checksum field. >>> >>> For example "flow create 0 ingress pattern eth / ipv4 / end actions >>> rss types ipv4-chksum end queues end / end", this flow causes all >>> matching packets to be distributed to queues on basis of IPv4 >>> checksum. >>> >>> Signed-off-by: Alvin Zhang >>> Reviewed-by: Andrew Rybchenko >> >> I've failed to find mail where I've added the tag. I've replied with "LGTM" for v2 >> which has IPv4 checksum only and many comments on L4 checksum added in v3. >> So, I think it is incorrect to inherit "LGTM" as Reviewed-by. >> > > I'll correct it at next version. > >>> Acked-by: Ajit Khaparde >>> Acked-by: Aman Deep Singh >>> --- >>> >>> v3: Add L4 checksum RSS offload type >>> v4: Add doc and help string, update commit log >>> --- >>> app/test-pmd/cmdline.c | 12 +++++++++--- >>> app/test-pmd/config.c | 2 ++ >>> lib/ethdev/rte_ethdev.h | 2 ++ >>> 3 files changed, 13 insertions(+), 3 deletions(-) >>> >>> diff --git a/app/test-pmd/cmdline.c b/app/test-pmd/cmdline.c index >>> 0268b18..93543d8 100644 >>> --- a/app/test-pmd/cmdline.c >>> +++ b/app/test-pmd/cmdline.c >>> @@ -2254,6 +2254,10 @@ struct cmd_config_rss { >>> rss_conf.rss_hf = ETH_RSS_ECPRI; >>> else if (!strcmp(res->value, "mpls")) >>> rss_conf.rss_hf = ETH_RSS_MPLS; >>> + else if (!strcmp(res->value, "ipv4-chksum")) >>> + rss_conf.rss_hf = ETH_RSS_IPV4_CHKSUM; >>> + else if (!strcmp(res->value, "l4-chksum")) >>> + rss_conf.rss_hf = ETH_RSS_L4_CHKSUM; >>> else if (!strcmp(res->value, "none")) >>> rss_conf.rss_hf = 0; >>> else if (!strcmp(res->value, "level-default")) { @@ -2325,7 +2329,7 >>> @@ struct cmd_config_rss { >>> .help_str = "port config all rss " >>> "all|default|eth|vlan|ip|tcp|udp|sctp|ether|port|vxlan|geneve|" >>> >> "nvgre|vxlan-gpe|l2tpv3|esp|ah|pfcp|ecpri|mpls|none|level-default|" >>> - "level-outer|level-inner|", >>> + "level-outer|level-inner|ipv4-chksum|l4-chksum|", >>> .tokens = { >>> (void *)&cmd_config_rss_port, >>> (void *)&cmd_config_rss_keyword, >>> @@ -2438,7 +2442,8 @@ struct cmd_config_rss_hash_key { >>> "ipv6-tcp-ex#ipv6-udp-ex#" >>> "l3-src-only#l3-dst-only#l4-src-only#l4-dst-only#" >>> "l2-src-only#l2-dst-only#s-vlan#c-vlan#" >>> - "l2tpv3#esp#ah#pfcp#pppoe#gtpu#ecpri#mpls"); >>> + "l2tpv3#esp#ah#pfcp#pppoe#gtpu#ecpri#mpls#" >>> + "ipv4-chksum#l4-chksum"); >>> cmdline_parse_token_string_t cmd_config_rss_hash_key_value = >>> TOKEN_STRING_INITIALIZER(struct cmd_config_rss_hash_key, key, NULL); >>> >>> @@ -2451,7 +2456,8 @@ struct cmd_config_rss_hash_key { >>> "l2-payload|ipv6-ex|ipv6-tcp-ex|ipv6-udp-ex|" >>> "l3-src-only|l3-dst-only|l4-src-only|l4-dst-only|" >>> "l2-src-only|l2-dst-only|s-vlan|c-vlan|" >>> - "l2tpv3|esp|ah|pfcp|pppoe|gtpu|ecpri|mpls " >>> + "l2tpv3|esp|ah|pfcp|pppoe|gtpu|ecpri|mpls|" >>> + "ipv4-chksum|l4-chksum " >>> "", >>> .tokens = { >>> (void *)&cmd_config_rss_hash_key_port, diff --git >>> a/app/test-pmd/config.c b/app/test-pmd/config.c index 43c79b5..14968bf >>> 100644 >>> --- a/app/test-pmd/config.c >>> +++ b/app/test-pmd/config.c >>> @@ -140,6 +140,8 @@ >>> { "gtpu", ETH_RSS_GTPU }, >>> { "ecpri", ETH_RSS_ECPRI }, >>> { "mpls", ETH_RSS_MPLS }, >>> + { "ipv4-chksum", ETH_RSS_IPV4_CHKSUM }, >>> + { "l4-chksum", ETH_RSS_L4_CHKSUM }, >>> { NULL, 0 }, >>> }; >>> >>> diff --git a/lib/ethdev/rte_ethdev.h b/lib/ethdev/rte_ethdev.h index >>> faf3bd9..63b0321 100644 >>> --- a/lib/ethdev/rte_ethdev.h >>> +++ b/lib/ethdev/rte_ethdev.h >>> @@ -537,6 +537,8 @@ struct rte_eth_rss_conf { >>> #define ETH_RSS_PPPOE (1ULL << 31) >>> #define ETH_RSS_ECPRI (1ULL << 32) >>> #define ETH_RSS_MPLS (1ULL << 33) >>> +#define ETH_RSS_IPV4_CHKSUM (1ULL << 34) >>> +#define ETH_RSS_L4_CHKSUM (1ULL << 35) /* TCP/UDP/SCTP */ >> >> It does not reply on my questions at all. Above list of Layer 4 protocols does not say if the flag may be reported when all above protocols are supported or some protocols are supported only. Also I had a question about UDP checksum 0 since it could be treated in a different ways since logically it is no checksum at all. > You have said: >>>> I don't think so. IMHO, it should report all RSS capabilities >>>> regardless generic vs flow API RSS action. >>> >> May be it is OK to report subset in >> dev_info->flow_type_rss_offloads, but I'm very uncomfortable with the >> approach. Superset sounds more logical to me, but has drawbacks as >> well. > > Here I have another question: > There are flow type definition and RSS offload type definition, > #define RTE_ETH_FLOW_RAW 1 > #define RTE_ETH_FLOW_IPV4 2 > #define RTE_ETH_FLOW_FRAG_IPV4 3 > #define RTE_ETH_FLOW_NONFRAG_IPV4_TCP 4 > > #define ETH_RSS_IPV4 (1ULL << 2) > #define ETH_RSS_FRAG_IPV4 (1ULL << 3) > #define ETH_RSS_NONFRAG_IPV4_TCP (1ULL << 4) > > are they the different expressions of the same concept? Sorry, but I don't understand the question. > > If yes, why they have been decoupled by the commit: fce6b66893. Do you mean that motivation in the changeset description is not clear? > Then what the flow type of ETH_RSS_PORT, it's UDP, TCP, or SCTP? I have no answer to the question as well. That's why I'm trying to avoid it in the patch. > > If not, the PMDs can report supported RSS flow type by dev_info->flow_type_rss_offloads, > but have no way to report supported RSS offload types. Sorry, don't understand. >> >> Nack