From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from mail-wm0-f67.google.com (mail-wm0-f67.google.com [74.125.82.67]) by dpdk.org (Postfix) with ESMTP id B75EF1BB93 for ; Wed, 11 Apr 2018 18:32:27 +0200 (CEST) Received: by mail-wm0-f67.google.com with SMTP id r82so4944413wme.0 for ; Wed, 11 Apr 2018 09:32:27 -0700 (PDT) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=6wind-com.20150623.gappssmtp.com; s=20150623; h=date:from:to:cc:subject:message-id:references:mime-version :content-disposition:in-reply-to; bh=sGuWumeubo12LOW0nSwhQ7HfLpNQt9Fxnx7bBqYh2Cc=; b=mU3wa4BcDFSs7jG6rvL9sQgOSSH6XT/qNVWp8lOrTSULz6wlJ+ik9/U8XJIUAZWBlb oVVhCcvK5YXmO3Yi+2MqD7QZoFPvyP8c4hKU/xY6NryjnXTxlBoUqKBpLSV/2hu1UuVQ WT4CyTQOWJCOgibo94kIZKucqXGk5ouYsItVTdZh986KOOGM9Zw7J0ogKiAqeX5JiX5y ORBDVCdF0bEmJ7Sajp0uKqpWn2Z9zWFb4QDxW+rminWNTGhzybzDx4xa1A2Ig1HCsqzW dsw+o+4MncgVFHKDrh18TsU01Fo9y5VI3UqxZfcBNwS0iRWaTCpCs3Lh9Y4aNpabNPMb v1iw== X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20161025; h=x-gm-message-state:date:from:to:cc:subject:message-id:references :mime-version:content-disposition:in-reply-to; bh=sGuWumeubo12LOW0nSwhQ7HfLpNQt9Fxnx7bBqYh2Cc=; b=J7IsSygnCPTvl3rR/dlPuiHgGJ7HS550NHtrsA8zgqm48IRdJiZYLPg/sXo6xvmDq/ FZpazsjtW5Z40I88UR6++biip7WqVhHsgUKndARrVPvQ3CBvf3pmZb97VfTLJhmgum45 C2BfMIPZviJwoVMn6SAP9VTSuGZyiHBBJpwu0+1HluknEogfl1NBgaw83oqPUAmwVZtE /y1oE2Dq45qUJpCw++WaJ3XeYU+yqQstH68pl4y3DmKTkDdwAJIwwSUTtKTSCgmVAZKa 0C/qYtbiidZWv1R6EER3jEhUgKoHqq467QjQoO2M7qD2k9Tf8T8sBNQCXk+hEam0hSH/ Ei1w== X-Gm-Message-State: ALQs6tBuHcj471+94ezTsWaQBgRUr1/2v5VVLS0cxAaGA7izfgY5CqQU YdtUU+bfvN+nvyo8OEdTDaqxGA== X-Google-Smtp-Source: AIpwx4/Hwed2+8Sg9I05se8KDoh0AbtYRcLLjBkuph/Lu6OQPTMs35umAZ8CcFHxOUxNuSyEDrMoHQ== X-Received: by 10.28.103.10 with SMTP id b10mr3066985wmc.132.1523464347345; Wed, 11 Apr 2018 09:32:27 -0700 (PDT) Received: from 6wind.com (host.78.145.23.62.rev.coltfrance.com. [62.23.145.78]) by smtp.gmail.com with ESMTPSA id 58sm3213960wrv.41.2018.04.11.09.32.26 (version=TLS1_2 cipher=ECDHE-RSA-AES128-GCM-SHA256 bits=128/128); Wed, 11 Apr 2018 09:32:26 -0700 (PDT) Date: Wed, 11 Apr 2018 18:32:13 +0200 From: Adrien Mazarguil To: Qi Zhang Cc: dev@dpdk.org, declan.doherty@intel.com, sugesh.chandran@intel.com, michael.j.glynn@intel.com, yu.y.liu@intel.com, konstantin.ananyev@intel.com, bruce.richardson@intel.com Message-ID: <20180411163213.GN4957@6wind.com> References: <1522279780-34842-1-git-send-email-qi.z.zhang@intel.com> <1522617562-25940-1-git-send-email-qi.z.zhang@intel.com> <1522617562-25940-4-git-send-email-qi.z.zhang@intel.com> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: <1522617562-25940-4-git-send-email-qi.z.zhang@intel.com> Subject: Re: [dpdk-dev] [PATCH v2 3/4] ether: add more protocol support in flow API X-BeenThere: dev@dpdk.org X-Mailman-Version: 2.1.15 Precedence: list List-Id: DPDK patches and discussions List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Wed, 11 Apr 2018 16:32:27 -0000 On Sun, Apr 01, 2018 at 05:19:21PM -0400, Qi Zhang wrote: > Add new protocol header match support as below > > RTE_FLOW_ITEM_TYPE_ARP > - match IPv4 ARP header > RTE_FLOW_ITEM_TYPE_EXT_HDR_ANY > - match any IPv6 extension header While properly defined in the patch, "IPV6" is missing here. > RTE_FLOW_ITEM_TYPE_ICMPV6 > - match IPv6 ICMP header > RTE_FLOW_ITEM_TYPE_ICMPV6_TGT_ADDR > - match IPv6 ICMP Target address > RTE_FLOW_ITEM_TYPE_ICMPV6_SSL > - match IPv6 ICMP Source Link-layer address > RTE_FLOW_ITEM_TYPE_ICMPV6_TTL > - match IPv6 ICMP Target Link-layer address > > Signed-off-by: Qi Zhang First, since they are added at the end of enum rte_flow_item_type, no ABI breakage notice is necessary. However testpmd implementation [1][2] and documentation update [3][4] are mandatory for all new pattern items and actions. More comments below regarding these definitions. [1] flow_item[] in app/test-pmd/config.c [2] using ITEM_ICMP as an example in app/test-pmd/cmdline_flow.c [3] "Pattern items" section in doc/guides/testpmd_app_ug/testpmd_funcs.rst [4] using "Item: ``ICMP``" section as an example in doc/guides/prog_guide/rte_flow.rst > --- > lib/librte_ether/rte_flow.h | 160 ++++++++++++++++++++++++++++++++++++++++++++ > 1 file changed, 160 insertions(+) > > diff --git a/lib/librte_ether/rte_flow.h b/lib/librte_ether/rte_flow.h > index 8f75db0..a8ec780 100644 > --- a/lib/librte_ether/rte_flow.h > +++ b/lib/librte_ether/rte_flow.h > @@ -323,6 +323,49 @@ enum rte_flow_item_type { > * See struct rte_flow_item_geneve. > */ > RTE_FLOW_ITEM_TYPE_GENEVE, > + > + /** > + * Matches ARP IPv4 header. => Matches an IPv4 ARP header. > + * > + * See struct rte_flow_item_arp. > + */ > + RTE_FLOW_ITEM_TYPE_ARP, While you're right to make "IPv4" clear since ARP is also used for other protocols DPDK doesn't support (and likely never will), the ARP header has both a fixed and a variably-sized part. Ideally an ARP pattern item should match the fixed part only and a separate ARP_IPV4 match its payload, somewhat like you did for ICMPv6/NDP below. Problem is that in DPDK, struct arp_hdr includes struct arp_ipv4, so one suggestion would be to rename this pattern item ARP_IPV4 directly: => RTE_FLOW_ITEM_TYPE_ARP_IPV4 > + > + /** > + * Matches any IPv6 Extension header. => Matches an IPv6 extension header. > + * > + * See struct rte_flow_item_ipv6_ext_any. > + */ > + RTE_FLOW_ITEM_TYPE_IPV6_EXT_HDR_ANY, I'm not sure this definition is necessary, more below about that. Also I don't see a benefit in having "ANY" part of the name, if you want to keep it, I suggest the simpler: => RTE_FLOW_ITEM_TYPE_IPV6_EXT > + > + /** > + * Matches ICMPv6 header. => Matches an ICMPv6 header. > + * > + * See struct rte_flow_item_icmpv6 Missing "." > + */ > + RTE_FLOW_ITEM_TYPE_ICMPV6, > + Before entering NDP territory below, I understand those should be stacked on top of RTE_FLOW_ITEM_TYPE_ICMPV6. It's fine but for clarity they should be named after the NDP types they represent, not inner data fields. Also I think we should consider NDP as a protocol sitting on top of ICMPv6. We could therefore drop "ICMP" from these definitions. Since "ND" is a common shorthand for this protocol and "6" another when doing something related to IPv6, I suggest to use "ND6" to name he related pattern items. These are the reasons behind my next suggestions: > + /** > + * Match ICMPv6 target address. > + * > + * See struct rte_flow_item_icmpv6_tgt_addr. > + */ > + RTE_FLOW_ITEM_TYPE_ICMPV6_TGT_ADDR, => Matches an IPv6 network discovery router solicitation. => See struct rte_flow_item_nd6_rs. => RTE_FLOW_ITEM_TYPE_ND6_RS, You should add another item for neighbor advertisement messages using the same template: => Match an IPv6 network discovery neighbor advertisement. => See struct rte_flow_item_nd6_na. => RTE_FLOW_ITEM_TYPE_ND6_NA, The following are possible options for these headers, if specified they must be found afterward. Also since IPv6 may run on top of protocols other than Ethernet, you need to clarify these link-layer addresses use the Ethernet format: > + > + /** > + * Match ICMPv6 Source Link-Layer Address. > + * > + * See struct rte_flow_item_icmpv6_sll. > + */ > + RTE_FLOW_ITEM_TYPE_ICMPV6_SLL, => Matches an IPv6 network discovery source Ethernet link-layer address option. => See struct rte_flow_item_nd6_opt_sla_eth. => RTE_FLOW_ITEM_TYPE_ND6_OPT_SLA_ETH, > + > + /** > + * Match ICMPv6 Target Link-Layer Address. > + * > + * See struct rte_flow_item_icmpv6_tll. > + */ > + RTE_FLOW_ITEM_TYPE_ICMPV6_TLL, => Matches an IPv6 network discovery target Ethernet link-layer address option. => See struct rte_flow_item_nd6_opt_tla_eth. => RTE_FLOW_ITEM_TYPE_ND6_OPT_TLA_ETH, > + Unnecessary empty line. > }; > > /** > @@ -815,6 +858,123 @@ static const struct rte_flow_item_geneve rte_flow_item_geneve_mask = { > #endif > > /** > + * RTE_FLOW_ITEM_TYPE_ARP > + * > + * Matches IPv4 ARP packet header As above: => Matches an IPv4 ARP header. => RTE_FLOW_ITEM_TYPE_ARP_IPV4 > + */ > +struct rte_flow_item_arp { > + struct arp_hdr hdr; > +}; Needs #include and a Doxygen comment next to hdr for consistency, see ICMP and other definitions. > + > +/** Default mask for RTE_FLOW_ITEM_TYPE_ARP. */ > +#ifndef __cplusplus > +static const struct rte_flow_item_arp rte_flow_item_arp_mask = { > + .hdr = { > + .arp_data = { > + .arp_sha = { > + .addr_bytes = "\xff\xff\xff\xff\xff\xff", > + }, > + .arp_sip = RTE_BE32(0xffffffff), > + .arp_tha = { > + .addr_bytes = "\xff\xff\xff\xff\xff\xff", > + }, > + .arp_tip = RTE_BE32(0xffffffff), > + }, > + }, > +}; > +#endif > + > +/** > + * RTE_FLOW_ITEM_TYPE_IPV6_EXT_HDR_ANY > + * > + * Matches any IPv6 extension header. > + */ > +struct rte_flow_item_ipv6_ext_hdr_any { > + uint8_t next_hdr; > +}; So what's the point? next_hdr is already part of either struct ipv6_hdr ("proto") and individual extension headers. Moreover it's implicit if an extension header is provided in a pattern. How about removing it? > + > +/** Default mask for RTE_FLOW_ITEM_TYPE_IPV6_EXT_HDR_ANY. */ > +#ifndef __cplusplus > +static const > +struct rte_flow_item_ipv6_ext_hdr_any rte_flow_item_ipv6_ext_any_mask = { > + .next_hdr = 0xff, > +}; > +#endif Ditto. > + > +/** > + * RTE_FLOW_ITEM_TYPE_ICMPV6 > + * > + * Matches ICMPv6 header. => Matches an ICMPv6 header. > + */ > +struct rte_flow_item_icmpv6 { > + uint8_t type; > + uint8_t code; > + uint16_t checksum; The last 32-bit "reserved" data field is missing. > +}; Too bad there is no struct icmp6_hdr definition in rte_icmp.h. You could add it. In any case Doxygen comments are missing, please add them (see other structure definitions for examples). > + > +/** Default mask for RTE_FLOW_ITEM_TYPE_ICMPV6 */ Missing "." > +#ifndef __cplusplus > +static const struct rte_flow_item_icmpv6 rte_flow_item_icmpv6_mask = { > + .type = 0xff, > + .code = 0xff, > + .checksum = RTE_BE16(0xffff), > +}; > +#endif You must remove checksum matching from the default mask. That's the last thing an application might want to match exactly :) > + > +/** > + * RTE_FLOW_ITEM_TYPE_ICMPV6_TGT_ADDR > + * > + * Matches ICMPv6's Target Address. > + */ > +struct rte_flow_item_icmpv6_tgt_addr { > + uint8_t addr[16]; > +}; You need to expand this as two items, see prior comments regarding RTE_FLOW_ITEM_TYPE_ND6_RS, RTE_FLOW_ITEM_TYPE_ND6_NA and their respective structs rte_flow_item_nd6_rs and rte_flow_item_nd6_na. Also Doxygen documentation is missing for the addr field and you need to describe that these are only valid when used after RTE_FLOW_ITEM_TYPE_ICMPV6. > + > +/** Default mask for RTE_FLOW_ITEM_TYPE_ICMPV6_TGT_ADDR */ Missing "." > +#ifndef __cplusplus > +static const > +struct rte_flow_item_icmpv6_tgt_addr rte_flow_item_icmpv6_tgt_addr_mask = { > + .addr = > + "\xff\xff\xff\xff\xff\xff\xff\xff" > + "\xff\xff\xff\xff\xff\xff\xff\xff", > +}; > +#endif > + > +/** > + * RTE_FLOW_ITEM_TYPE_ICPMV6_SLL. > + * > + * Matches ICMPv6 Source Link-Layer address. > + */ > +struct rte_flow_item_icmpv6_sll { > + struct ether_addr addr; > +}; See prior comments regarding RTE_FLOW_ITEM_TYPE_ND6_OPT_SLA_ETH and struct rte_flow_item_type_nd6_opt_sla_eth. Also Doxygen documentation is missing for the addr field and you need to describe that it is only valid when found after either RTE_FLOW_ITEM_TYPE_ND6_RS or RTE_FLOW_ITEM_TYPE_ND6_NA. Also missing empty line here. > +/** Default mask for RTE_FLOW_ITEM_TYPE_ICMPV6_SLL */ Missing "." > +#ifndef __cplusplus > +static const struct rte_flow_item_icmpv6_sll rte_flow_item_icmpv6_sll_mask = { > + .addr = { > + .addr_bytes = "\xff\xff\xff\xff\xff\xff", > + } > +}; > +#endif > + > +/** > + * RTE_FLOW_ITEM_TYPE_ICMPV6_TLL. > + * > + * Matches ICMPv6 Target Link-Layer address. > + */ > +struct rte_flow_item_icmpv6_tll { > + struct ether_addr addr; > +}; See prior comments regarding RTE_FLOW_ITEM_TYPE_ND6_OPT_TLA_ETH and struct rte_flow_item_type_nd6_opt_tla_eth. Also Doxygen documentation is missing for the addr field and you need to describe that it is only valid when found after either RTE_FLOW_ITEM_TYPE_ND6_RS or RTE_FLOW_ITEM_TYPE_ND6_NA. Also missing empty line here. > +/** Default mask for RTE_FLOW_ITEM_TYPE_ICMPV6_TLL */ Missing "." > +#ifndef __cplusplus > +static const struct rte_flow_item_icmpv6_tll rte_flow_item_icmpv6_tll_mask = { > + .addr = { > + .addr_bytes = "\xff\xff\xff\xff\xff\xff", > + } > +}; > +#endif > + > +/** > * Matching pattern item definition. > * > * A pattern is formed by stacking items starting from the lowest protocol > -- > 2.7.4 > -- Adrien Mazarguil 6WIND