From: Adrien Mazarguil <adrien.mazarguil@6wind.com>
To: Ferruh Yigit <ferruh.yigit@intel.com>
Cc: "Zhao1, Wei" <wei.zhao1@intel.com>, "dev@dpdk.org" <dev@dpdk.org>,
"Lu, Wenzhuo" <wenzhuo.lu@intel.com>
Subject: Re: [dpdk-dev] [PATCH 15/18] net/ixgbe: parse flow director filter
Date: Fri, 23 Dec 2016 09:13:10 +0100 [thread overview]
Message-ID: <20161223081310.GH10340@6wind.com> (raw)
In-Reply-To: <86cc3f7c-4488-9efa-48ce-82eb57ae572c@intel.com>
Hi,
On Thu, Dec 22, 2016 at 10:44:32AM +0000, Ferruh Yigit wrote:
> On 12/22/2016 9:19 AM, Zhao1, Wei wrote:
> > Hi, Yigit
> >
> >> -----Original Message-----
> >> From: Yigit, Ferruh
> >> Sent: Wednesday, December 21, 2016 1:01 AM
> >> To: Zhao1, Wei <wei.zhao1@intel.com>; dev@dpdk.org
> >> Cc: Lu, Wenzhuo <wenzhuo.lu@intel.com>
> >> Subject: Re: [dpdk-dev] [PATCH 15/18] net/ixgbe: parse flow director filter
> >>
> >> On 12/2/2016 10:43 AM, Wei Zhao wrote:
> >>> From: wei zhao1 <wei.zhao1@intel.com>
> >>>
> >>> check if the rule is a flow director rule, and get the flow director info.
> >>>
> >>> Signed-off-by: wei zhao1 <wei.zhao1@intel.com>
> >>> Signed-off-by: Wenzhuo Lu <wenzhuo.lu@intel.com>
> >>> ---
> >>
> >> <...>
> >>
> >>> + PATTERN_SKIP_VOID(rule, struct ixgbe_fdir_rule,
> >>> + RTE_FLOW_ERROR_TYPE_ITEM_NUM);
> >>> + if (item->type != RTE_FLOW_ITEM_TYPE_ETH &&
> >>> + item->type != RTE_FLOW_ITEM_TYPE_IPV4 &&
> >>> + item->type != RTE_FLOW_ITEM_TYPE_IPV6 &&
> >>> + item->type != RTE_FLOW_ITEM_TYPE_UDP &&
> >>> + item->type != RTE_FLOW_ITEM_TYPE_VXLAN &&
> >>> + item->type != RTE_FLOW_ITEM_TYPE_NVGRE) {
> >>
> >> This gives build error [1], there are a few more same usage:
> >>
> >> .../drivers/net/ixgbe/ixgbe_ethdev.c:9238:17: error: comparison of constant
> >> 241 with expression of type 'const enum rte_flow_item_type' is always true
> >> [-Werror,-Wtautological-constant-out-of-range-compare]
> >> item->type != RTE_FLOW_ITEM_TYPE_NVGRE) {
> >>
> >>
> >>
> >
> > Ok, I will add two type definition RTE_FLOW_ITEM_TYPE_NVGRE and RTE_FLOW_ITEM_TYPE_E_TAG into const enum rte_flow_item_type to eliminate this problem.
> > Thank you.
> >
>
> CC: Adrien Mazarguil <adrien.mazarguil@6wind.com>
Thanks, the original message did not catch my attention.
> Yes, that is what right thing to do, since rte_flow patchset not merged
> yet, perhaps Adrien may want to include this as next version of his
> patchset?
>
> What do you think Adrien?
I think by now the rte_flow series is automatically categorized as spam by
half the community already, and that new items such as these can be added
subsequently on their own, ideally before the entire ixgbe/i40e series.
I have a few comments regarding these new items if we want to make them part
of rte_flow.h (see definitions below).
Unfortunately, even though they are super convenient to use and expose in the
testpmd flow command, we need to avoid C-style bit-field definitions such as
these for protocol header matching because they are not endian-safe,
particularly multi-byte fields. Only meta-data that should be interpreted
with host endianness can use them (e.g. struct rte_flow_attr, struct
rte_flow_action_vf, etc):
struct rte_flow_item_nvgre {
uint32_t flags0:1; /**< 0 */
uint32_t rsvd1:1; /**< 1 bit not defined */
uint32_t flags1:2; /**< 2 bits, 1 0 */
uint32_t rsvd0:9; /**< Reserved0 */
uint32_t ver:3; /**< version */
uint32_t protocol:16; /**< protocol type, 0x6558 */
uint32_t tni:24; /**< tenant network ID or virtual subnet ID */
uint32_t flow_id:8; /**< flow ID or Reserved */
};
For an example how to avoid them, see struct ipv6_hdr definition in
rte_ip.h, where field vtc_flow is 32 bit but covers three protocol fields
and is considered big-endian (Nelio's endianness series [1] would be really
handy to eliminate confusion here). Also see struct rte_flow_item_vxlan,
which covers 24-bit fields using uint8_t arrays.
struct rte_flow_item_e_tag {
struct ether_addr dst; /**< Destination MAC. */
struct ether_addr src; /**< Source MAC. */
uint16_t e_tag_ethertype; /**< E-tag EtherType, 0x893F. */
uint16_t e_pcp:3; /**< E-PCP */
uint16_t dei:1; /**< DEI */
uint16_t in_e_cid_base:12; /**< Ingress E-CID base */
uint16_t rsv:2; /**< reserved */
uint16_t grp:2; /**< GRP */
uint16_t e_cid_base:12; /**< E-CID base */
uint16_t in_e_cid_ext:8; /**< Ingress E-CID extend */
uint16_t e_cid_ext:8; /**< E-CID extend */
uint16_t type; /**< MAC type. */
unsigned int tags; /**< Number of 802.1Q/ad tags defined. */
struct {
uint16_t tpid; /**< Tag protocol identifier. */
uint16_t tci; /**< Tag control information. */
} tag[]; /**< 802.1Q/ad tag definitions, outermost first. */
};
Besides the bit-field issue for this one, looks like it should be split, at
least the initial part is already covered by rte_flow_item_eth.
I do not know much about E-Tag (IEEE 802.1BR right?) but it sort of looks
like a 2.5 layer protocol reminiscent of VLAN.
tags and tag[] fields seem based on the VLAN definition of the original
rte_flow RFC that has since been replaced with stacked rte_flow_item_vlan
items, much easier to program for. Perhaps this can be relied on instead of
having e_tag implement its own variant.
As a protocol-matching item and E-Tag TCI being 6 bytes according to IEEE
802.1BR, sizeof(struct rte_flow_item_e_tag) should ideally return 6 as well
and would likely comprise three big-endian uint16_t fields. See how
PCP/DEI/VID VLAN fields are interpreted in testpmd [2].
Again, these concerns only stand if you intend to include these definitions
into rte_flow.h.
[1] http://dpdk.org/ml/archives/dev/2016-November/050060.html
[2] http://dpdk.org/ml/archives/dev/2016-December/052976.html
--
Adrien Mazarguil
6WIND
next prev parent reply other threads:[~2016-12-23 8:13 UTC|newest]
Thread overview: 36+ messages / expand[flat|nested] mbox.gz Atom feed top
2016-12-02 10:42 [dpdk-dev] [PATCH 00/18] net/ixgbe: Consistent filter API Wei Zhao
2016-12-02 10:42 ` [dpdk-dev] [PATCH 01/18] net/ixgbe: store SYN filter Wei Zhao
2016-12-20 16:55 ` Ferruh Yigit
2016-12-22 9:48 ` Zhao1, Wei
2017-01-06 16:26 ` Ferruh Yigit
2017-01-10 6:48 ` Zhao1, Wei
2016-12-26 1:47 ` Zhao1, Wei
2016-12-02 10:42 ` [dpdk-dev] [PATCH 02/18] net/ixgbe: store flow director filter Wei Zhao
2016-12-20 16:58 ` Ferruh Yigit
2016-12-26 2:50 ` Zhao1, Wei
2016-12-02 10:42 ` [dpdk-dev] [PATCH 03/18] net/ixgbe: store L2 tunnel filter Wei Zhao
2016-12-02 10:43 ` [dpdk-dev] [PATCH 04/18] net/ixgbe: restore n-tuple filter Wei Zhao
2016-12-20 16:58 ` Ferruh Yigit
2016-12-26 3:32 ` Zhao1, Wei
2016-12-02 10:43 ` [dpdk-dev] [PATCH 05/18] net/ixgbe: restore ether type filter Wei Zhao
2016-12-02 10:43 ` [dpdk-dev] [PATCH 06/18] net/ixgbe: restore SYN filter Wei Zhao
2016-12-02 10:43 ` [dpdk-dev] [PATCH 07/18] net/ixgbe: restore flow director filter Wei Zhao
2016-12-02 10:43 ` [dpdk-dev] [PATCH 08/18] net/ixgbe: restore L2 tunnel filter Wei Zhao
2016-12-02 10:43 ` [dpdk-dev] [PATCH 09/18] net/ixgbe: store and restore L2 tunnel configuration Wei Zhao
2016-12-02 10:43 ` [dpdk-dev] [PATCH 10/18] net/ixgbe: flush all the filters Wei Zhao
2016-12-02 10:43 ` [dpdk-dev] [PATCH 11/18] net/ixgbe: parse n-tuple filter Wei Zhao
2016-12-20 17:23 ` Ferruh Yigit
2016-12-02 10:43 ` [dpdk-dev] [PATCH 12/18] net/ixgbe: parse ethertype filter Wei Zhao
2016-12-02 10:43 ` [dpdk-dev] [PATCH 13/18] net/ixgbe: parse SYN filter Wei Zhao
2016-12-02 10:43 ` [dpdk-dev] [PATCH 14/18] net/ixgbe: parse L2 tunnel filter Wei Zhao
2016-12-02 10:43 ` [dpdk-dev] [PATCH 15/18] net/ixgbe: parse flow director filter Wei Zhao
2016-12-20 17:00 ` Ferruh Yigit
2016-12-22 9:19 ` Zhao1, Wei
2016-12-22 10:44 ` Ferruh Yigit
2016-12-23 8:13 ` Adrien Mazarguil [this message]
2016-12-27 3:31 ` Zhao1, Wei
2016-12-02 10:43 ` [dpdk-dev] [PATCH 16/18] net/ixgbe: create consistent filter Wei Zhao
2016-12-20 17:25 ` Ferruh Yigit
2016-12-23 6:26 ` Zhao1, Wei
2016-12-02 10:43 ` [dpdk-dev] [PATCH 17/18] net/ixgbe: destroy " Wei Zhao
2016-12-02 10:43 ` [dpdk-dev] [PATCH 18/18] net/ixgbe: flush " Wei Zhao
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=20161223081310.GH10340@6wind.com \
--to=adrien.mazarguil@6wind.com \
--cc=dev@dpdk.org \
--cc=ferruh.yigit@intel.com \
--cc=wei.zhao1@intel.com \
--cc=wenzhuo.lu@intel.com \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).