[dpdk-dev,RFC,v2,1/5] ether: add flow action to redirect packet in a switch domain

Message ID 1513823719-36066-2-git-send-email-qi.z.zhang@intel.com (mailing list archive)
State Superseded, archived
Delegated to: Ferruh Yigit
Headers

Checks

Context Check Description
ci/checkpatch warning coding style issues
ci/Intel-compilation success Compilation OK

Commit Message

Qi Zhang Dec. 21, 2017, 2:35 a.m. UTC
  Add action RTE_FLOW_ACTION_TYPE_SWITCH_PORT, it can be used to redirect
a packet to a network interface that connect to the same switch domain,
rte_ethdev's port_id is used as an identification of the destination.
A typical use case is: with a smart NIC for vSwitch acceleration, flow
is defined to forward packets between the switch port that is managed by
Port Representor.

Signed-off-by: Qi Zhang <qi.z.zhang@intel.com>
---
 doc/guides/prog_guide/rte_flow.rst | 22 ++++++++++++++++++++++
 lib/librte_ether/rte_flow.h        | 19 ++++++++++++++++++-
 2 files changed, 40 insertions(+), 1 deletion(-)
  

Comments

Alex Rosenbaum Dec. 21, 2017, 12:37 p.m. UTC | #1
On Thu, Dec 21, 2017 at 4:35 AM, Qi Zhang <qi.z.zhang@intel.com> wrote:
> Add action RTE_FLOW_ACTION_TYPE_SWITCH_PORT, it can be used to redirect

I guess the word "SWITCH" should be remove from commit message. you
don't use it later in the patch.


>
> +Action: ``PORT``
> +^^^^^^^^^^^^^^^^
> +
> +Redirect packets to an interface that connect to the same switch domain.
> +
> +The desitnation should be managed by a rte_ethdev instance, port_id is
> +the identification of the destination. A typical use case is to define
> +a flow that redirect packet to a interface that mananged by a Port
> +Representor.


A verbs would be better suited for an ACTION_TYPE. while
".._TYPE_PORT" is a nous.
Probably ".._TYPE_REDIRECT" would better fit here.
See man tc-mirred as referance:
http://man7.org/linux/man-pages/man8/tc-mirred.8.html

Do we want to distinguish between different destination type?
The target might be a port (port_id) or potencial other destinations/queue.
So maybe use ".._TYPE_REDIRECT_TO_PORT"?

Anyway, I think you should remove the "same switch domain" from docs
since there is no switch domain yet in DPDK.
Lets let the PMD decided if this sucessed or fails, based on the
target type and other HW limitations. Not just based on switch domain.

PS: I agree switch domain needs to be introduced. I don't think port
representor is the correct direction.

Alex
  
Qi Zhang Dec. 22, 2017, 8:20 a.m. UTC | #2
Hi Alex

> -----Original Message-----

> From: Alex Rosenbaum [mailto:rosenbaumalex@gmail.com]

> Sent: Thursday, December 21, 2017 8:37 PM

> To: Zhang, Qi Z <qi.z.zhang@intel.com>

> Cc: adrien.mazarguil@6wind.com; DPDK <dev@dpdk.org>; Doherty, Declan

> <declan.doherty@intel.com>

> Subject: Re: [dpdk-dev] [RFC v2 1/5] ether: add flow action to redirect packet

> in a switch domain

> 

> On Thu, Dec 21, 2017 at 4:35 AM, Qi Zhang <qi.z.zhang@intel.com> wrote:

> > Add action RTE_FLOW_ACTION_TYPE_SWITCH_PORT, it can be used to

> > redirect

> 

> I guess the word "SWITCH" should be remove from commit message. you

> don't use it later in the patch.


Yes, it should be corrected.
> 

> 

> >

> > +Action: ``PORT``

> > +^^^^^^^^^^^^^^^^

> > +

> > +Redirect packets to an interface that connect to the same switch domain.

> > +

> > +The destination should be managed by a rte_ethdev instance, port_id

> > +is the identification of the destination. A typical use case is to

> > +define a flow that redirect packet to an interface that managed by a

> > +Port Representor.

> 

> 

> A verbs would be better suited for an ACTION_TYPE. while ".._TYPE_PORT" is

> a nous.

> Probably ".._TYPE_REDIRECT" would better fit here.

> See man tc-mirred as referance:

> http://man7.org/linux/man-pages/man8/tc-mirred.8.html


I agree it will be better to use verbs for action, so we can have TYPE_REDIRECT_TO_PORT/VF/PF...,
But since we already have ACTION_TYPE_VF, ACTION_TYPE_PF ...
Maybe it's better just to follow the same pattern?

> 

> Do we want to distinguish between different destination type?

> The target might be a port (port_id) or potencial other destinations/queue.

> So maybe use ".._TYPE_REDIRECT_TO_PORT"?

> 

> Anyway, I think you should remove the "same switch domain" from docs

> since there is no switch domain yet in DPDK.

> Lets let the PMD decided if this sucessed or fails, based on the target type

> and other HW limitations. Not just based on switch domain.


Yes, it's not necessary to be specific here, the new action is just add the semantic
to support packet redirect between port that managed by etherdevs, device driver
can figure out the way or just reject it.
I will capture this in v3.
> 

> PS: I agree switch domain needs to be introduced. I don't think port

> representor is the correct direction.


OK, thanks for your sharing, I think this can be discussed more on Port Representor mail list

> 

> Alex


Thanks
Qi
  
Alex Rosenbaum Dec. 22, 2017, 10:10 p.m. UTC | #3
+Adrien

On Fri, Dec 22, 2017 at 10:20 AM, Zhang, Qi Z <qi.z.zhang@intel.com> wrote:
>> On Thu, Dec 21, 2017 at 4:35 AM, Qi Zhang <qi.z.zhang@intel.com> wrote:
>> > Add action RTE_FLOW_ACTION_TYPE_SWITCH_PORT, it can be used to
>> > redirect

>> A verbs would be better suited for an ACTION_TYPE. while ".._TYPE_PORT" is
>> a nous.
>> Probably ".._TYPE_REDIRECT" would better fit here.
>
> I agree it will be better to use verbs for action, so we can have TYPE_REDIRECT_TO_PORT/VF/PF...,
> But since we already have ACTION_TYPE_VF, ACTION_TYPE_PF ...
> Maybe it's better just to follow the same pattern?

hemmm, missed these ACTION_TYPE_VF/PF...
Adrien, what do you think about these naming conventions?
  

Patch

diff --git a/doc/guides/prog_guide/rte_flow.rst b/doc/guides/prog_guide/rte_flow.rst
index d158be5..dcea2f6 100644
--- a/doc/guides/prog_guide/rte_flow.rst
+++ b/doc/guides/prog_guide/rte_flow.rst
@@ -1474,6 +1474,28 @@  fields in the pattern items.
    | 1     | END      |
    +-------+----------+
 
+Action: ``PORT``
+^^^^^^^^^^^^^^^^
+
+Redirect packets to an interface that connect to the same switch domain.
+
+The desitnation should be managed by a rte_ethdev instance, port_id is
+the identification of the destination. A typical use case is to define
+a flow that redirect packet to a interface that mananged by a Port
+Representor.
+
+- Terminating by default.
+
+.. _table_rte_flow_action_port:
+
+.. table:: PORT
+
+   +--------------+-----------------------------------+
+   | Field        | Value                             |
+   +==============+===================================+
+   | ``port_id``  | identification of the destination |
+   +--------------+-----------------------------------+
+
 Negative types
 ~~~~~~~~~~~~~~
 
diff --git a/lib/librte_ether/rte_flow.h b/lib/librte_ether/rte_flow.h
index 47c88ea..91706e2 100644
--- a/lib/librte_ether/rte_flow.h
+++ b/lib/librte_ether/rte_flow.h
@@ -1008,7 +1008,14 @@  enum rte_flow_action_type {
 	 *
 	 * See struct rte_flow_action_security.
 	 */
-	RTE_FLOW_ACTION_TYPE_SECURITY
+	RTE_FLOW_ACTION_TYPE_SECURITY,
+
+	/**
+	 * Redirect packets to a network interface in the same switch domain.
+	 *
+	 * See struct rte_flow_action_port.
+	 */
+	RTE_FLOW_ACTION_TYPE_PORT,
 };
 
 /**
@@ -1146,6 +1153,16 @@  struct rte_flow_action_security {
 	void *security_session; /**< Pointer to security session structure. */
 };
 
+/** RTE_FLOW_ACTION_TYPE_PORT
+ *
+ * Redirect packets to a network interface in the same switch domain.
+ *
+ * Terminateing by default.
+ */
+struct rte_flow_action_port {
+       uint16_t port_id; /**< identification of the forward destination. */
+};
+
 /**
  * Definition of a single action.
  *