From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from mga03.intel.com (mga03.intel.com [134.134.136.65]) by dpdk.org (Postfix) with ESMTP id 8833E8E62 for ; Tue, 17 Apr 2018 16:40:05 +0200 (CEST) X-Amp-Result: SKIPPED(no attachment in message) X-Amp-File-Uploaded: False Received: from fmsmga007.fm.intel.com ([10.253.24.52]) by orsmga103.jf.intel.com with ESMTP/TLS/DHE-RSA-AES256-GCM-SHA384; 17 Apr 2018 07:40:04 -0700 X-ExtLoop1: 1 X-IronPort-AV: E=Sophos;i="5.48,463,1517904000"; d="scan'208";a="32631789" Received: from dwdohert-mobl.ger.corp.intel.com (HELO [10.252.9.17]) ([10.252.9.17]) by fmsmga007.fm.intel.com with ESMTP; 17 Apr 2018 07:40:02 -0700 To: Adrien Mazarguil Cc: dev@dpdk.org References: <1523017443-12414-1-git-send-email-declan.doherty@intel.com> <1523017443-12414-5-git-send-email-declan.doherty@intel.com> <20180406202701.GU4957@6wind.com> From: "Doherty, Declan" Message-ID: <328cb51d-9bd5-aa61-c498-cbeb0262f4c0@intel.com> Date: Tue, 17 Apr 2018 15:40:02 +0100 User-Agent: Mozilla/5.0 (Windows NT 10.0; WOW64; rv:52.0) Gecko/20100101 Thunderbird/52.7.0 MIME-Version: 1.0 In-Reply-To: <20180406202701.GU4957@6wind.com> Content-Type: text/plain; charset=utf-8; format=flowed Content-Language: en-US Content-Transfer-Encoding: 7bit Subject: Re: [dpdk-dev] [PATCH v3 4/4] ethdev: Add metadata flow and action items support 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: Tue, 17 Apr 2018 14:40:06 -0000 On 06/04/2018 9:27 PM, Adrien Mazarguil wrote: > On Fri, Apr 06, 2018 at 01:24:03PM +0100, Declan Doherty wrote: >> Introduces a new action type RTE_FLOW_ACTION_TYPE_METADATA which enables >> metadata extraction from a packet into a specified metadata container >> for consumption on further pipeline stages or for propagation to the host >> interface. >> >> As complementary function to the new metadata action type this patch also >> introduces a new flow item type which enables flow patterns to specify a >> specific metadata container as a matching criteria for a flow rule. >> >> Signed-off-by: Declan Doherty > > Is there already HW support for extraction and subsequent matching? > > Otherwise it looks overkill. If it's meant to be implemented in software by > PMDs, isn't the existing MARK action with arbitrary value good enough? > All it needs is a corresponding MARK pattern item (which reminds me I forgot > to add it...) > > My suggestion would be to submit this new item/action combo at the same > time as the first actual PMD implementation. In the meantime I think a much > simpler MARK pattern item could satisfy the most pressing needs. > I admit this was a bit future looking :) and probably overkill for what is needed today. I'll add the suggested MARK pattern item which will be enough to handle the OVS-DPDK tunnel encap/decap requirements. >> --- >> doc/guides/prog_guide/rte_flow.rst | 85 ++++++++++++++++++++++++++++++++++++++ >> lib/librte_ether/rte_flow.h | 42 +++++++++++++++++++ >> 2 files changed, 127 insertions(+) >> >> diff --git a/doc/guides/prog_guide/rte_flow.rst b/doc/guides/prog_guide/rte_flow.rst >> index 2f0a47a..9b2b0e3 100644 >> --- a/doc/guides/prog_guide/rte_flow.rst >> +++ b/doc/guides/prog_guide/rte_flow.rst >> @@ -1580,6 +1580,91 @@ group on that device. >> +--------------+---------------------------------+ >> >> >> + > > You keep adding these empty lines :) sorry.... what can I say I love whitespace :D > >> +Action: ``METADATA`` >> +^^^^^^^^^^^^^^^^^^^^ >> + >> +Action extracts data from packet into user specified metadata field in pipeline >> +for use in downstream processing stages or for propagation to host interface. >> + >> +The pattern mask is used to define the data which is to be extracted from the >> +packet. The mask pattern types defined in the action metadata pattern must >> +match the flow pattern definitions up to the last flow item from which data is >> +to be extracted. >> + >> +- Non-terminating by default. > > Usual comment [1] regarding this property. > > [1] "ethdev: alter behavior of flow API actions" > http://dpdk.org/ml/archives/dev/2018-April/095779.html > >> + >> +.. _table_rte_flow_action_metadata: >> + >> +.. table:: METADATA >> + >> + +--------------+---------------------------+ >> + | Field | Value | >> + +==============+===========================+ >> + | ``id`` | Metadata field Identifier | >> + +--------------+---------------------------+ >> + | ``pattern`` | Extraction mask pattern | >> + +--------------+---------------------------+ >> + >> +The example below demonstrates how the extraction mask to extract the source/ >> +destination IPv4 address, the UDP destination port and and the VxLAN VNI can be >> +specified. >> + >> +.. _table_rte_flow_action_metadata_example: >> + >> +.. table:: IPv4 VxLAN metadata extraction >> + >> + +-------+--------------------------+-----------------------------------+ >> + | Index | Flow Item Type | Flow Mask | >> + +=======+==========================+===================================+ >> + | 0 | RTE_FLOW_ITEM_TYPE_ETH | .dst = "\x00\x00\x00\x00\x00\x00" | >> + | | +-----------------------------------+ >> + | | | .src = "\x00\x00\x00\x00\x00\x00" | >> + | | +-----------------------------------+ >> + | | | .type = RTE_BE16(0x0) | >> + +-------+--------------------------+-----------------------------------+ >> + | 1 | RTE_FLOW_ITEM_TYPE_IPV4 | .src_addr = RTE_BE32(0xffffffff) | >> + | | +-----------------------------------+ >> + | | | .dst_addr = RTE_BE32(0xffffffff) | >> + +-------+--------------------------+-----------------------------------+ >> + | 2 | RTE_FLOW_ITEM_TYPE_UDP | .src_port = RTE_BE16(0x0) | >> + | | +-----------------------------------+ >> + | | | .dst_port = RTE_BE16(0xffff) | >> + +-------+--------------------------+-----------------------------------+ >> + | 3 | RTE_FLOW_ITEM_TYPE_VXLAN | .vni = "\xff\xff\xff" | >> + +-------+--------------------------+-----------------------------------+ >> + | 4 | RTE_FLOW_ITEM_TYPE_END | NULL | >> + +-------+--------------------------+-----------------------------------+ >> + >> +If only the VxLAN VNI extraction was required then the extraction mask would be >> +as follows. > > VxLAN => VXLAN > >> + >> +.. _table_rte_flow_action_metadata_example_2: >> + >> +.. table:: VxLAN VNI metadata extraction >> + >> + +-------+--------------------------+-----------------------------------+ >> + | Index | Flow Item Type | Flow Mask | >> + +=======+==========================+===================================+ >> + | 0 | RTE_FLOW_ITEM_TYPE_ETH | .dst = "\x00\x00\x00\x00\x00\x00" | >> + | | +-----------------------------------+ >> + | | | .src = "\x00\x00\x00\x00\x00\x00" | >> + | | +-----------------------------------+ >> + | | | .type = RTE_BE16(0x0) | >> + +-------+--------------------------+-----------------------------------+ >> + | 1 | RTE_FLOW_ITEM_TYPE_IPV4 | .src_addr = RTE_BE32(0x0) | >> + | | +-----------------------------------+ >> + | | | .dst_addr = RTE_BE32(0x0) | >> + +-------+--------------------------+-----------------------------------+ >> + | 2 | RTE_FLOW_ITEM_TYPE_UDP | .src_port = RTE_BE16(0x0) | >> + | | +-----------------------------------+ >> + | | | .dst_port = RTE_BE16(0x0) | >> + +-------+--------------------------+-----------------------------------+ >> + | 3 | RTE_FLOW_ITEM_TYPE_VXLAN | .vni = "\xff\xff\xff" | >> + +-------+--------------------------+-----------------------------------+ >> + | 4 | RTE_FLOW_ITEM_TYPE_END | NULL | >> + +-------+--------------------------+-----------------------------------+ >> + >> Negative types >> ~~~~~~~~~~~~~~ >> >> diff --git a/lib/librte_ether/rte_flow.h b/lib/librte_ether/rte_flow.h >> index 968a23b..f75dee7 100644 >> --- a/lib/librte_ether/rte_flow.h >> +++ b/lib/librte_ether/rte_flow.h >> @@ -323,6 +323,13 @@ enum rte_flow_item_type { >> * See struct rte_flow_item_geneve. >> */ >> RTE_FLOW_ITEM_TYPE_GENEVE, >> + >> + /** >> + * Matches specified pipeline metadata field. >> + * >> + * See struct rte_flow_item_metadata. >> + */ >> + RTE_FLOW_ITEM_TYPE_METADATA > > Please add the trailing comma to ease subsequent contributions. > >> }; >> >> /** >> @@ -815,6 +822,17 @@ static const struct rte_flow_item_geneve rte_flow_item_geneve_mask = { >> #endif >> >> /** >> + * RTE_FLOW_ITEM_TYPE_METADATA >> + * >> + * Allow arbitrary pipeline metadata to be used in specification flow pattern >> + */ >> +struct rte_flow_item_metadata { >> + uint32_t id; /**< field identifier */ >> + uint32_t size; /**< field size */ >> + uint8_t *bytes; /**< field value */ >> +}; > > One last nit regarding this definition (assuming we keep it), please remove > extra spacing before comments (no alignment) and add caps to format it like > the rest of that file. > >> + >> +/** >> * Matching pattern item definition. >> * >> * A pattern is formed by stacking items starting from the lowest protocol >> @@ -1266,6 +1284,30 @@ struct rte_flow_action_group { >> }; >> >> /** >> + * RTE_FLOW_ACTION_TYPE_METADATA > > Definition in enum rte_flow_action_type should be part of this commit. > >> + * >> + * Action extracts data from packet into specified metadata field in pipeline >> + * for use in downstream processing stages or for propagation to host interface. >> + * >> + * Pattern mask is use to define the extraction data for packet. The mask >> + * pattern types defined in the action metadata pattern must match the flow >> + * pattern definitions up to the last item from which data is to be extracted. >> + * >> + * Non-terminating by default. >> + */ >> +struct rte_flow_action_metadata { >> + uint32_t id; /**< field identifier */ >> + >> + struct rte_flow_action_mask_item { >> + enum rte_flow_item_type type; >> + /**< Flow item type. */ >> + const void *mask; >> + /**< Flow item mask. */ >> + } *pattern; >> + /** metadata extraction pattern mask specification */ >> +}; > > Same comment as in prior commit regarding this structure. You should use > struct rte_flow_item directly, except this time a pointer to a pattern > with multiple items seems warranted. > >> + >> +/** >> * Definition of a single action. >> * >> * A list of actions is terminated by a END action. >> -- >> 2.7.4 >> >