Re: [ovs-dev] [RFC] ofp-actions: Extend the use of max_len in OUTPUT action.

[Ben Pfaff]

On Wed, Mar 30, 2016 at 12:31:27PM -0700, pravin shelar wrote:
> On Wed, Mar 30, 2016 at 10:24 AM, William Tu  wrote:
> > Hi Pravin,
> >
> > Thanks for the feedback.
> > So another option is to add an new truncate action, and modify the size for
> > all the following output packets.  For example, the output:1 remains the
> > original size and the output:2 and output:3 will have size 100.
> >   # ovs-ofctl add-flow br0 'actions=output:1, truncate:100, output:2,
> > output:3'
> >
> > Or we can implement truncate_output action so that the truncate only scope
> > at a specific output. For example:
> >   # ovs-ofctl add-flow br0 'actions=output:1, truncate_output:
> > (2,max_len=100), output:3'
> > So that only output:2 is re-sized to 100.
> >
> > Which one do you think is more useful?
> >
> The comment was about datapath action. I am fine with ofctl interface
> that you have.

The formatting/parsing syntax diverges from that for the other newer
actions we've introduced; it's kind of weird to have parenthesized
arguments following another argument.

Therefore I'd prefer something more like:
output(port=1, max_len=128)

Also, I don't think it's reasonable to reuse OFPAT_OUTPUT for this.  One
simply cannot rely on every existing controller to fill something
sensible in for the max_len field.  I suggest inventing a new OpenFlow
extension action.  (That doesn't mean you have to change the syntax used
in ovs-ofctl; we have plenty of examples where multiple OpenFlow actions
have a common syntax.)
___
dev mailing list
dev@openvswitch.org
http://openvswitch.org/mailman/listinfo/dev

Re: [ovs-dev] [RFC] ofp-actions: Extend the use of max_len in OUTPUT action.

[pravin shelar]

On Wed, Mar 30, 2016 at 10:24 AM, William Tu  wrote:
> Hi Pravin,
>
> Thanks for the feedback.
> So another option is to add an new truncate action, and modify the size for
> all the following output packets.  For example, the output:1 remains the
> original size and the output:2 and output:3 will have size 100.
>   # ovs-ofctl add-flow br0 'actions=output:1, truncate:100, output:2,
> output:3'
>
> Or we can implement truncate_output action so that the truncate only scope
> at a specific output. For example:
>   # ovs-ofctl add-flow br0 'actions=output:1, truncate_output:
> (2,max_len=100), output:3'
> So that only output:2 is re-sized to 100.
>
> Which one do you think is more useful?
>
The comment was about datapath action. I am fine with ofctl interface
that you have.
As far as datapath truncate action is concerned I think we could limit
scope of the truncate action to next output or user-space action.
___
dev mailing list
dev@openvswitch.org
http://openvswitch.org/mailman/listinfo/dev

Re: [ovs-dev] [RFC] ofp-actions: Extend the use of max_len in OUTPUT action.

[William Tu]

Hi Pravin,

Thanks for the feedback.
So another option is to add an new truncate action, and modify the size for
all the following output packets.  For example, the output:1 remains the
original size and the output:2 and output:3 will have size 100.
  # ovs-ofctl add-flow br0 'actions=output:1, truncate:100, output:2,
output:3'

Or we can implement truncate_output action so that the truncate only scope
at a specific output. For example:
  # ovs-ofctl add-flow br0 'actions=output:1, truncate_output:
(2,max_len=100), output:3'
So that only output:2 is re-sized to 100.

Which one do you think is more useful?

Regards,
William

On Tue, Mar 29, 2016 at 4:52 PM, pravin shelar  wrote:

> On Tue, Mar 29, 2016 at 3:16 PM, William Tu  wrote:
> > Before, OpenFlow specification defines 'max_len' in struct
> ofp_action_output
> > as the max number of bytes to send when port is OFPP_CONTROLLER.  A
> max_len
> > of zero means no bytes of the packet should be sent, and max_len of
> > OFPCML_NO_BUFFER means the complete packet is sent to the controller.
> > It is left undefined of max_len, when output port is not OFPP_CONTROLLER.
> > The patch extends the use of max_len when output is non-controller.
> >
> > One use case is to enable port mirroring to send smaller packets to the
> > destination port so that only useful packet information is
> mirrored/copied,
> > saving some performance overhead of copying entire packet payload.
> > The patch proposes adding a '(max_len=)' after the output action.  An
> > example use case is below as well as shown in the tests/:
> >
> > - Output to port 1 with max_len 100 bytes.
> > - The output packet size on port 1 will be MIN(original_packet_size,
> 100).
> > # ovs-ofctl add-flow br0 'actions=output:1(max_len=100)'
> >
> > - The scope of max_len is limited to output action itself.  The
> following
> >   output:1 and output:2 will be the original packet size.
> > # ovs-ofctl add-flow br0
> 'actions=output:1(max_len=100),output:1,output:2'
> >
> > Implementation/Limitaions:
> > - Userspace and kernel datapath is supported, no Windows support.
> > - The minimum value of max_len is 60 byte (minimum Ethernet frame
> size).
> >   This is defined in OVS_ACTION_OUTPUT_MIN.
> > - Since 'max_len' is undefined in OpenFlow spec, the controller might
> >   accidentally place a garbage value in max_len and send to OVS.
> > - For compatibility, if the kernel datapath is not supported, set
> >   max_len to zero.
> > - OUTPUT_REG with max_len is not supported.
> > - actions=1(max_len=100) is not supported, must specify as
> 'output:1'.
> > - Only output:[0-9]*(max_len=) is supported.  Output to IN_PORT,
> >   TABLE, NORMAL, FLOOD, ALL, and LOCAL are not supported.
> >
> > Signed-off-by: William Tu 
> > ---
> >  datapath/actions.c| 19 +--
> >  datapath/datapath.h   |  1 +
> >  datapath/flow_netlink.c   | 10 ++--
> >  datapath/linux/compat/include/linux/openvswitch.h |  7 +++
> >  datapath/vport.c  |  6 +++
> >  lib/dp-packet.c   |  1 +
> >  lib/dp-packet.h   |  1 +
> >  lib/dpctl.c   | 19 ---
> >  lib/dpif-netdev.c | 20 ++-
> >  lib/netdev.c  |  8 +++
> >  lib/netlink.h |  1 +
> >  lib/odp-util.c| 27 --
> >  lib/ofp-actions.c | 41 +++
> >  lib/ofp-actions.h |  4 +-
> >  ofproto/ofproto-dpif-xlate.c  | 33 +++-
> >  ofproto/ofproto-dpif.c| 45 
> >  ofproto/ofproto-dpif.h|  4 ++
> >  tests/ofp-print.at|  6 +--
> >  tests/ofproto-dpif.at | 53
> +++
> >  tests/system-traffic.at   | 63
> +++
> >  20 files changed, 330 insertions(+), 39 deletions(-)
> >
> > diff --git a/datapath/actions.c b/datapath/actions.c
> > index dcf8591..d64dadf 100644
> > --- a/datapath/actions.c
> > +++ b/datapath/actions.c
> > @@ -738,10 +738,15 @@ err:
> >  }
> >
> >  static void do_output(struct datapath *dp, struct sk_buff *skb, int
> out_port,
> > - struct sw_flow_key *key)
> > +uint16_t max_len, struct sw_flow_key *key)
> >  {
> > struct vport *vport = ovs_vport_rcu(dp, out_port);
> >
> > +   /* This is after skb_clone called from do_execute_actions,
> > +  so max_len only applies to the current skb. */
> > +   if (unlikely(max_len != 0))
> > +   OVS_CB(skb)->max_len = 

Re: [ovs-dev] [RFC] ofp-actions: Extend the use of max_len in OUTPUT action.

[pravin shelar]

On Tue, Mar 29, 2016 at 3:16 PM, William Tu  wrote:
> Before, OpenFlow specification defines 'max_len' in struct ofp_action_output
> as the max number of bytes to send when port is OFPP_CONTROLLER.  A max_len
> of zero means no bytes of the packet should be sent, and max_len of
> OFPCML_NO_BUFFER means the complete packet is sent to the controller.
> It is left undefined of max_len, when output port is not OFPP_CONTROLLER.
> The patch extends the use of max_len when output is non-controller.
>
> One use case is to enable port mirroring to send smaller packets to the
> destination port so that only useful packet information is mirrored/copied,
> saving some performance overhead of copying entire packet payload.
> The patch proposes adding a '(max_len=)' after the output action.  An
> example use case is below as well as shown in the tests/:
>
> - Output to port 1 with max_len 100 bytes.
> - The output packet size on port 1 will be MIN(original_packet_size, 100).
> # ovs-ofctl add-flow br0 'actions=output:1(max_len=100)'
>
> - The scope of max_len is limited to output action itself.  The following
>   output:1 and output:2 will be the original packet size.
> # ovs-ofctl add-flow br0 'actions=output:1(max_len=100),output:1,output:2'
>
> Implementation/Limitaions:
> - Userspace and kernel datapath is supported, no Windows support.
> - The minimum value of max_len is 60 byte (minimum Ethernet frame size).
>   This is defined in OVS_ACTION_OUTPUT_MIN.
> - Since 'max_len' is undefined in OpenFlow spec, the controller might
>   accidentally place a garbage value in max_len and send to OVS.
> - For compatibility, if the kernel datapath is not supported, set
>   max_len to zero.
> - OUTPUT_REG with max_len is not supported.
> - actions=1(max_len=100) is not supported, must specify as 'output:1'.
> - Only output:[0-9]*(max_len=) is supported.  Output to IN_PORT,
>   TABLE, NORMAL, FLOOD, ALL, and LOCAL are not supported.
>
> Signed-off-by: William Tu 
> ---
>  datapath/actions.c| 19 +--
>  datapath/datapath.h   |  1 +
>  datapath/flow_netlink.c   | 10 ++--
>  datapath/linux/compat/include/linux/openvswitch.h |  7 +++
>  datapath/vport.c  |  6 +++
>  lib/dp-packet.c   |  1 +
>  lib/dp-packet.h   |  1 +
>  lib/dpctl.c   | 19 ---
>  lib/dpif-netdev.c | 20 ++-
>  lib/netdev.c  |  8 +++
>  lib/netlink.h |  1 +
>  lib/odp-util.c| 27 --
>  lib/ofp-actions.c | 41 +++
>  lib/ofp-actions.h |  4 +-
>  ofproto/ofproto-dpif-xlate.c  | 33 +++-
>  ofproto/ofproto-dpif.c| 45 
>  ofproto/ofproto-dpif.h|  4 ++
>  tests/ofp-print.at|  6 +--
>  tests/ofproto-dpif.at | 53 +++
>  tests/system-traffic.at   | 63 
> +++
>  20 files changed, 330 insertions(+), 39 deletions(-)
>
> diff --git a/datapath/actions.c b/datapath/actions.c
> index dcf8591..d64dadf 100644
> --- a/datapath/actions.c
> +++ b/datapath/actions.c
> @@ -738,10 +738,15 @@ err:
>  }
>
>  static void do_output(struct datapath *dp, struct sk_buff *skb, int out_port,
> - struct sw_flow_key *key)
> +uint16_t max_len, struct sw_flow_key *key)
>  {
> struct vport *vport = ovs_vport_rcu(dp, out_port);
>
> +   /* This is after skb_clone called from do_execute_actions,
> +  so max_len only applies to the current skb. */
> +   if (unlikely(max_len != 0))
> +   OVS_CB(skb)->max_len = max_len;
> +
> if (likely(vport)) {
> u16 mru = OVS_CB(skb)->mru;
>
> @@ -1034,6 +1039,7 @@ static int do_execute_actions(struct datapath *dp, 
> struct sk_buff *skb,
>  * is slightly obscure just to avoid that.
>  */
> int prev_port = -1;
> +   uint16_t max_len = 0;
> const struct nlattr *a;
> int rem;
>
> @@ -1045,15 +1051,18 @@ static int do_execute_actions(struct datapath *dp, 
> struct sk_buff *skb,
> struct sk_buff *out_skb = skb_clone(skb, GFP_ATOMIC);
>
> if (out_skb)
> -   do_output(dp, out_skb, prev_port, key);
> +   do_output(dp, out_skb, prev_port, max_len, 
> key);
>
> prev_port = -1;
> }
>
> switch (nla_type(a)) {
> - 

[ovs-dev] [RFC] ofp-actions: Extend the use of max_len in OUTPUT action.

[William Tu]

Before, OpenFlow specification defines 'max_len' in struct ofp_action_output
as the max number of bytes to send when port is OFPP_CONTROLLER.  A max_len
of zero means no bytes of the packet should be sent, and max_len of
OFPCML_NO_BUFFER means the complete packet is sent to the controller.
It is left undefined of max_len, when output port is not OFPP_CONTROLLER.
The patch extends the use of max_len when output is non-controller.

One use case is to enable port mirroring to send smaller packets to the
destination port so that only useful packet information is mirrored/copied,
saving some performance overhead of copying entire packet payload.
The patch proposes adding a '(max_len=)' after the output action.  An
example use case is below as well as shown in the tests/:

- Output to port 1 with max_len 100 bytes.
- The output packet size on port 1 will be MIN(original_packet_size, 100).
# ovs-ofctl add-flow br0 'actions=output:1(max_len=100)'

- The scope of max_len is limited to output action itself.  The following
  output:1 and output:2 will be the original packet size.
# ovs-ofctl add-flow br0 'actions=output:1(max_len=100),output:1,output:2'

Implementation/Limitaions:
- Userspace and kernel datapath is supported, no Windows support.
- The minimum value of max_len is 60 byte (minimum Ethernet frame size).
  This is defined in OVS_ACTION_OUTPUT_MIN.
- Since 'max_len' is undefined in OpenFlow spec, the controller might
  accidentally place a garbage value in max_len and send to OVS.
- For compatibility, if the kernel datapath is not supported, set
  max_len to zero.
- OUTPUT_REG with max_len is not supported.
- actions=1(max_len=100) is not supported, must specify as 'output:1'.
- Only output:[0-9]*(max_len=) is supported.  Output to IN_PORT,
  TABLE, NORMAL, FLOOD, ALL, and LOCAL are not supported.

Signed-off-by: William Tu 
---
 datapath/actions.c| 19 +--
 datapath/datapath.h   |  1 +
 datapath/flow_netlink.c   | 10 ++--
 datapath/linux/compat/include/linux/openvswitch.h |  7 +++
 datapath/vport.c  |  6 +++
 lib/dp-packet.c   |  1 +
 lib/dp-packet.h   |  1 +
 lib/dpctl.c   | 19 ---
 lib/dpif-netdev.c | 20 ++-
 lib/netdev.c  |  8 +++
 lib/netlink.h |  1 +
 lib/odp-util.c| 27 --
 lib/ofp-actions.c | 41 +++
 lib/ofp-actions.h |  4 +-
 ofproto/ofproto-dpif-xlate.c  | 33 +++-
 ofproto/ofproto-dpif.c| 45 
 ofproto/ofproto-dpif.h|  4 ++
 tests/ofp-print.at|  6 +--
 tests/ofproto-dpif.at | 53 +++
 tests/system-traffic.at   | 63 +++
 20 files changed, 330 insertions(+), 39 deletions(-)

diff --git a/datapath/actions.c b/datapath/actions.c
index dcf8591..d64dadf 100644
--- a/datapath/actions.c
+++ b/datapath/actions.c
@@ -738,10 +738,15 @@ err:
 }
 
 static void do_output(struct datapath *dp, struct sk_buff *skb, int out_port,
- struct sw_flow_key *key)
+uint16_t max_len, struct sw_flow_key *key)
 {
struct vport *vport = ovs_vport_rcu(dp, out_port);
 
+   /* This is after skb_clone called from do_execute_actions,
+  so max_len only applies to the current skb. */
+   if (unlikely(max_len != 0))
+   OVS_CB(skb)->max_len = max_len;
+
if (likely(vport)) {
u16 mru = OVS_CB(skb)->mru;
 
@@ -1034,6 +1039,7 @@ static int do_execute_actions(struct datapath *dp, struct 
sk_buff *skb,
 * is slightly obscure just to avoid that.
 */
int prev_port = -1;
+   uint16_t max_len = 0;
const struct nlattr *a;
int rem;
 
@@ -1045,15 +1051,18 @@ static int do_execute_actions(struct datapath *dp, 
struct sk_buff *skb,
struct sk_buff *out_skb = skb_clone(skb, GFP_ATOMIC);
 
if (out_skb)
-   do_output(dp, out_skb, prev_port, key);
+   do_output(dp, out_skb, prev_port, max_len, key);
 
prev_port = -1;
}
 
switch (nla_type(a)) {
-   case OVS_ACTION_ATTR_OUTPUT:
-   prev_port = nla_get_u32(a);
+   case OVS_ACTION_ATTR_OUTPUT: {
+   struct ovs_action_output *output = nla_data(a);
+   prev_port = output->port;
+