[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 <u9012063 at gmail.com> 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=<N>)' 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=<N>) is supported.  Output to IN_PORT,
>       TABLE, NORMAL, FLOOD, ALL, and LOCAL are not supported.
>
> Signed-off-by: William Tu <u9012063 at gmail.com>
> ---
>  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;
> +                       max_len = output->max_len;
>                         break;
> +               }

It is better to add separate action to truncate a packet rather than
extending output action. Separate action would allow greater reuse of
the action. For example we can use truncate action for sampling
truncated packets.