From mboxrd@z Thu Jan 1 00:00:00 1970 From: Andrew Rybchenko Subject: Re: [PATCH v3] ethdev: add isolated mode to flow API Date: Wed, 14 Jun 2017 17:04:16 +0300 Message-ID: References: <94c640cfbddb6afbe6f068efccbc40dc3c0f0a6b.1495637730.git.adrien.mazarguil@6wind.com> <6ed2734fe0baa20cf993434d81375deb1f647179.1497444240.git.adrien.mazarguil@6wind.com> <20170614133529.GO1758@6wind.com> Mime-Version: 1.0 Content-Type: text/plain; charset="windows-1252"; format=flowed Content-Transfer-Encoding: 7bit Cc: To: Adrien Mazarguil Return-path: Received: from dispatch1-us1.ppe-hosted.com (dispatch1-us1.ppe-hosted.com [67.231.154.164]) by dpdk.org (Postfix) with ESMTP id E4DBD7D12 for ; Wed, 14 Jun 2017 16:04:36 +0200 (CEST) In-Reply-To: <20170614133529.GO1758@6wind.com> List-Id: DPDK patches and discussions List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Errors-To: dev-bounces@dpdk.org Sender: "dev" On 06/14/2017 04:35 PM, Adrien Mazarguil wrote: > On Wed, Jun 14, 2017 at 04:01:46PM +0300, Andrew Rybchenko wrote: >> On 06/14/2017 03:45 PM, Adrien Mazarguil wrote: >>> Isolated mode can be requested by applications on individual ports to avoid >>> ingress traffic outside of the flow rules they define. >>> >>> Besides making ingress more deterministic, it allows PMDs to safely reuse >>> resources otherwise assigned to handle the remaining traffic, such as >>> global RSS configuration settings, VLAN filters, MAC address entries, >>> legacy filter API rules and so on in order to expand the set of possible >>> flow rule types. >>> >>> To minimize code complexity, PMDs implementing this mode may provide >>> partial (or even no) support for flow rules when not enabled (e.g. no >>> priorities, no RSS action). Applications written to use the flow API are >>> therefore encouraged to enable it. >>> >>> Once effective, leaving isolated mode may not be possible depending on PMD >>> implementation. >>> >>> Signed-off-by: Adrien Mazarguil >>> Acked-by: Nelio Laranjeiro >>> >>> --- >>> >>> v3: >>> - Rebased on next-net/master. Note this patch depends on >>> commit c0688ef1eded ("net/igb: parse flow API n-tuple filter") due to a >>> necessary fix in igb's rte_flow_ops definition to avoid a compilation >>> issue. >>> >>> v2: >>> - Rebased on master. >>> --- >>> app/test-pmd/cmdline.c | 4 ++ >>> app/test-pmd/cmdline_flow.c | 49 ++++++++++++++++++++- >>> app/test-pmd/config.c | 16 +++++++ >>> app/test-pmd/testpmd.h | 1 + >>> doc/guides/prog_guide/rte_flow.rst | 56 ++++++++++++++++++++++++ >>> doc/guides/testpmd_app_ug/testpmd_funcs.rst | 48 +++++++++++++++++++- >>> drivers/net/e1000/igb_flow.c | 9 ++-- >>> drivers/net/ixgbe/ixgbe_flow.c | 9 ++-- >>> lib/librte_ether/rte_ether_version.map | 7 +++ >>> lib/librte_ether/rte_flow.c | 18 ++++++++ >>> lib/librte_ether/rte_flow.h | 33 ++++++++++++++ >>> lib/librte_ether/rte_flow_driver.h | 5 +++ >>> 12 files changed, 242 insertions(+), 13 deletions(-) >> >> >>> diff --git a/doc/guides/prog_guide/rte_flow.rst b/doc/guides/prog_guide/rte_flow.rst >>> index b587ba9..699d2b2 100644 >>> --- a/doc/guides/prog_guide/rte_flow.rst >>> +++ b/doc/guides/prog_guide/rte_flow.rst >>> @@ -1517,6 +1517,62 @@ Return values: >>> - 0 on success, a negative errno value otherwise and ``rte_errno`` is set. >>> +Isolated mode >>> +------------- >>> + >>> +The general expectation for ingress traffic is that flow rules process it >>> +first; the remaining unmatched or pass-through traffic usually ends up in a >>> +queue (with or without RSS, locally or in some sub-device instance) >>> +depending on the global configuration settings of a port. >>> + >>> +While fine from a compatibility standpoint, this approach makes drivers more >>> +complex as they have to check for possible side effects outside of this API >>> +when creating or destroying flow rules. It results in a more limited set of >>> +available rule types due to the way device resources are assigned (e.g. no >>> +support for the RSS action even on capable hardware). >>> + >>> +Given that nonspecific traffic can be handled by flow rules as well, >>> +isolated mode is a means for applications to tell a driver that ingress on >>> +the underlying port must be injected from the defined flow rules only; that >>> +no default traffic is expected outside those rules. >>> + >>> +This has the following benefits: >>> + >>> +- Applications get finer-grained control over the kind of traffic they want >>> + to receive (no traffic by default). >>> + >>> +- More importantly they control at what point nonspecific traffic is handled >>> + relative to other flow rules, by adjusting priority levels. >>> + >>> +- Drivers can assign more hardware resources to flow rules and expand the >>> + set of supported rule types. >>> + >>> +Because toggling isolated mode may cause profound changes to the ingress >>> +processing path of a driver, it may not be possible to leave it once >>> +entered. Likewise, existing flow rules or global configuration settings may >>> +prevent a driver from entering isolated mode. >>> + >>> +Applications relying on this mode are therefore encouraged to toggle it as >>> +soon as possible after device initialization, ideally before the first call >>> +to ``rte_eth_dev_configure()`` to avoid possible failures due to conflicting >>> +settings. >>> + >> I think it would be useful to highlight how isolated mode coexists with >> promiscuous >> and all-multicast. What is the expected behaviour of the functions which >> toggle >> promiscuous and all-multicast mode if isolated mode is enabled? These >> functions >> return void right now, so it is impossible to return error. What should >> rte_eth_promiscuous_get() and rte_eth_allmulticast_get() return? > They can technically return nothing/anything as long as they have no effect > on received traffic, as described. I was just asking to highlight it in the documentation. Yes, idea of the isolated mode is clear and may be it is enough. > Modifying existing wrappers that currently return void instead of an error > is outside the scope of this patch and requires ABI breakage. This can be > done later when the need arises. It is perfectly clear. > For mlx4/mlx5, we plan to expose a different set of rte_eth_dev_ops > depending on whether isolated mode is toggled. When enabled, the > allmulti/promisc/MAC/VLAN/etc callbacks would be NULL for instance, and the > associated ethdev wrappers would automatically return an error where > applicable. Thanks for the idea. We'll consider it as well. Andrew.