From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from mga09.intel.com (mga09.intel.com [134.134.136.24]) by dpdk.org (Postfix) with ESMTP id 0E07EAAD5 for ; Tue, 17 Apr 2018 16:40:19 +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 orsmga102.jf.intel.com with ESMTP/TLS/DHE-RSA-AES256-GCM-SHA384; 17 Apr 2018 07:40:18 -0700 X-ExtLoop1: 1 X-IronPort-AV: E=Sophos;i="5.48,463,1517904000"; d="scan'208";a="32631839" 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:17 -0700 To: Adrien Mazarguil Cc: dev@dpdk.org References: <1523017443-12414-1-git-send-email-declan.doherty@intel.com> <1523017443-12414-4-git-send-email-declan.doherty@intel.com> <20180406202651.GT4957@6wind.com> From: "Doherty, Declan" Message-ID: Date: Tue, 17 Apr 2018 15:40:17 +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: <20180406202651.GT4957@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 3/4] ethdev: Add group action type to rte_flow 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:20 -0000 On 06/04/2018 9:26 PM, Adrien Mazarguil wrote: > On Fri, Apr 06, 2018 at 01:24:02PM +0100, Declan Doherty wrote: >> Add group action type which defines a terminating action which >> allows a matched flow to be redirect to a group. This allows logical >> flow table hierarchies to be managed through rte_flow. >> >> Signed-off-by: Declan Doherty > > OK, I'm wondering if perhaps with the addition of this action, we should > redefine groups as unlinked by default? Sure that makes sense to me, as in a device with multiple groups I would image that you are more likely to have a tree hierarchy of groups, based on something like packet types, rather than a straight priority ordering. > > Currently traffic enters through the flow rule with the lowest priority of > the group with the lowest ID and iterates through subsequent flow rules and > groups until matched by a flow rule without PASSTHRU (according to latest > definition [1]). > > This would make jumps between groups always explicit, not necessarily a bad > idea given no PMD implements groups as of yet. Thoughts? I think this is a good idea, as in the case with multiple groups which support the same packet time, there may be some flows which could potentially hit on many groups, but should be directed to a specific group. If group matching was always resolved in a strict priority it would result on a hit on an incorrect group. I think making it explicit from the start makes sense. One issue is that groups will need to be populated with a specific rule to allow flow to fall through to the next priority group if no match is found on the default group. This just requires changes to the API comments, correct? Do you want to capture those in [1] or would you like me to make them with this patch? > > Also as a rather fundamental API addition, I suggest to add it after > RTE_FLOW_ACTION_TYPE_PASSTHRU. It's OK because ABI is already broken. You > just need to mention it in the commit log [1]. no problem, will do. > > Another suggestion would be to rename it "JUMP" (reasons below). I have no issue with changing the action name to JUMP > > [1] "ethdev: alter behavior of flow API actions" > http://dpdk.org/ml/archives/dev/2018-April/095779.html > >> --- >> doc/guides/prog_guide/rte_flow.rst | 23 +++++++++++++++++++++++ >> lib/librte_ether/rte_flow.h | 15 +++++++++++++++ >> 2 files changed, 38 insertions(+) >> >> diff --git a/doc/guides/prog_guide/rte_flow.rst b/doc/guides/prog_guide/rte_flow.rst >> index 106fb93..2f0a47a 100644 >> --- a/doc/guides/prog_guide/rte_flow.rst >> +++ b/doc/guides/prog_guide/rte_flow.rst >> @@ -1557,6 +1557,29 @@ set of overlay header type. >> | ``item type`` | Item type of tunnel end-point to decapsulate | >> +---------------+----------------------------------------------+ >> >> + > > Unnecessary empty line. > >> +Action: ``GROUP`` >> +^^^^^^^^^^^^^^^^^ >> + >> +Redirects packets to a group on the current device. >> + >> +In a hierarchy of groups, which can be used to represent physical or logical >> +flow tables on the device, this action allows the terminating action to be a >> +group on that device. >> + >> +- Terminating by default. > > Keep in mind there's no such thing as a terminating action anymore [1]. > >> + >> +.. _table_rte_flow_action_group: >> + >> +.. table:: GROUP >> + >> + +--------------+---------------------------------+ >> + | Field | Value | >> + +==============+=================================+ >> + | ``id`` | Group ID to redirect packets to | >> + +--------------+---------------------------------+ > > "Field" column can be shrunk somewhat. > >> + >> + >> Negative types >> ~~~~~~~~~~~~~~ >> >> diff --git a/lib/librte_ether/rte_flow.h b/lib/librte_ether/rte_flow.h >> index 6d94423..968a23b 100644 >> --- a/lib/librte_ether/rte_flow.h >> +++ b/lib/librte_ether/rte_flow.h >> @@ -1251,6 +1251,21 @@ struct rte_flow_action_tunnel_decap { >> }; >> >> /** >> + * RTE_FLOW_ACTION_TYPE_GROUP > > Its addition to enum rte_flow_action_type should be part of this commit. > that was a rebasing issue, I'll fix in next revision >> + * >> + * Redirects packets to a group on the current device. >> + * >> + * In a hierarchy of groups, which can be used to represent physical or logical >> + * flow tables on the device, this action allows the terminating action to be a >> + * group on that device. >> + * >> + * Terminating by default. > > See [1]. > >> + */ >> +struct rte_flow_action_group { >> + uint32_t id; > > Assuming this structure is named rte_flow_action_jump, naming this field > "group" would match the attribute of the same name. > >> +}; >> + >> +/** >> * Definition of a single action. >> * >> * A list of actions is terminated by a END action. >> -- >> 2.7.4 >> > > Don't forget testpmd code and documentation update. >