[dpdk-dev] [PATCH 15/18] net/ixgbe: parse flow director filter

Adrien Mazarguil adrien.mazarguil at 6wind.com
Fri Dec 23 09:13:10 CET 2016


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 at intel.com>; dev at dpdk.org
> >> Cc: Lu, Wenzhuo <wenzhuo.lu at 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 at intel.com>
> >>>
> >>> check if the rule is a flow director rule, and get the flow director info.
> >>>
> >>> Signed-off-by: wei zhao1 <wei.zhao1 at intel.com>
> >>> Signed-off-by: Wenzhuo Lu <wenzhuo.lu at 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 at 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


More information about the dev mailing list