[ovs-dev] [PATCH v2] ofp-actions: Add a new action to truncate a packet.

Joe Stringer joe at ovn.org
Wed Apr 6 17:53:11 UTC 2016


On 1 April 2016 at 17:37, William Tu <u9012063 at gmail.com> wrote:
> The patch proposes adding a new action to support packet truncation.  The new
> action is formatted as 'output(port=n,max_len=m)', as output to port n, with
> packet size being MIN(original_size, m).
>
> 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.  Example
> use case is below as well as shown in the testcases:
>
>     - 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(port=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(port=1,max_len=100),output:1,output:2'
>
> Implementation/Limitaions:
>
>     - The patch adds a new OpenFlow extension action OFPACT_OUTPUT_TRUNC, and
>       a new datapath action, OVS_ACTION_ATTR_OUTPUT_TRUNC.
>     - The minimum value of max_len is 60 byte (minimum Ethernet frame size).
>       This is defined in OVS_ACTION_OUTPUT_MIN.
>     - Only "output(port=[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>

Great to see another user of the kernel testsuite :-)

One broad piece of feedback, what happens if you run an old
openvswitch kernel module with this new userspace that supports the
action? Eg if you took your stock openvswitch module that comes with
your distribution kernel and run this new userspace against it. Can
you still execute this action?

<snip>

> diff --git a/datapath/actions.c b/datapath/actions.c
> index dcf8591..a1a1241 100644
> --- a/datapath/actions.c
> +++ b/datapath/actions.c

...

> @@ -1055,6 +1060,13 @@ static int do_execute_actions(struct datapath *dp, struct sk_buff *skb,
>                         prev_port = nla_get_u32(a);
>                         break;
>
> +        case OVS_ACTION_ATTR_OUTPUT_TRUNC: {
> +                       struct ovs_action_output_trunc *output_trunc = nla_data(a);
> +                       prev_port = output_trunc->port;
> +                       max_len = output_trunc->max_len;
> +                       break;
> +        }
> +

Whitespace.

>                 case OVS_ACTION_ATTR_USERSPACE:
>                         output_userspace(dp, skb, key, a, attr, len);
>                         break;

<snip>

> @@ -536,6 +540,36 @@ encode_OUTPUT(const struct ofpact_output *output,
>  }
>
>  static char * OVS_WARN_UNUSED_RESULT
> +parse_truncate_subfield(struct ofpact_output_trunc *output_trunc,
> +                        const char *arg_)
> +{
> +    char *key, *value;
> +    char *error = NULL;
> +    char *arg = CONST_CAST(char *, arg_);
> +
> +    while (ofputil_parse_key_value(&arg, &key, &value)) {
> +        char *error;
> +        if (!strcmp(key, "port")) {
> +            if (!ofputil_port_from_string(value, &output_trunc->port)) {
> +                return xasprintf("%s: output to unknown port", value);
> +            }
> +        } else if (!strcmp(key, "max_len")) {
> +            error = str_to_u16(value, key, &output_trunc->max_len);
> +        } else {
> +            error = xasprintf("invalid key '%s' in output_trunc argument",
> +                          key);
> +        }
> +        if (error) {
> +            return error;
> +        }
> +    }
> +    if (!error && arg[0]) {
> +        error = xstrdup("unexpected input following field syntax");
> +    }
> +    return error;
> +}
> +
> +static char * OVS_WARN_UNUSED_RESULT
>  parse_OUTPUT(const char *arg, struct ofpbuf *ofpacts,
>               enum ofputil_protocol *usable_protocols OVS_UNUSED)
>  {

Clang-3.7 complains:

../lib/ofp-actions.c:553:17: error: variable 'error' is used
uninitialized whenever 'if' condition is false
[-Werror,-Wsometimes-uninitialized]
            if (!ofputil_port_from_string(value, &output_trunc->port)) {
                ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
../lib/ofp-actions.c:562:13: note: uninitialized use occurs here
        if (error) {
            ^~~~~
../lib/ofp-actions.c:553:13: note: remove the 'if' if its condition is
always true
            if (!ofputil_port_from_string(value, &output_trunc->port)) {
            ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
../lib/ofp-actions.c:551:20: note: initialize the variable 'error' to
silence this warning
        char *error;
                   ^
                    = NULL
1 error generated.


<snip>

> diff --git a/tests/system-traffic.at b/tests/system-traffic.at
> index 28adbdc..cf8d3f3 100644
> --- a/tests/system-traffic.at
> +++ b/tests/system-traffic.at
> @@ -49,6 +49,72 @@ NS_CHECK_EXEC([at_ns0], [ping -s 3200 -q -c 3 -i 0.3 -w 2 10.2.2.2 | FORMAT_PING
>  OVS_TRAFFIC_VSWITCHD_STOP
>  AT_CLEANUP
>
> +AT_SETUP([datapath - output_trunc action])
> +OVS_TRAFFIC_VSWITCHD_START()
> +
> +AT_CHECK([ovs-ofctl del-flows br0])
> +
> +dnl Create p0 and ovs-p0(1)
> +ADD_NAMESPACES(at_ns0)
> +ADD_VETH(p0, at_ns0, br0, "10.1.1.1/24")
> +NS_CHECK_EXEC([at_ns0], [ip link set dev p0 address e6:66:c1:11:11:11])
> +NS_CHECK_EXEC([at_ns0], [arp -s 10.1.1.2 e6:66:c1:22:22:22])
> +
> +dnl Create p1(3) and ovs-p1(2), packets received from ovs-p1 will appear in p1
> +AT_CHECK([ip link add p1 type veth peer name ovs-p1])
> +AT_CHECK([ip link set dev ovs-p1 up])
> +AT_CHECK([ip link set dev p1 up])
> +AT_CHECK([ovs-vsctl add-port br0 ovs-p1 -- set interface ovs-p1 ofport_request=2])
> +dnl Use p1 to check the truncated packet
> +AT_CHECK([ovs-vsctl add-port br0 p1 -- set interface p1 ofport_request=3])

This set of commands is effectively duplicated below. Maybe we could
add a new macro or shell function to tests/system-common-macros.at to
do something like this - then in future, other tests can take
advantage of the same code.

> +dnl Create p2(5) and ovs-p2(4)
> +AT_CHECK([ip link add p2 type veth peer name ovs-p2])
> +AT_CHECK([ip link set dev ovs-p2 up])
> +AT_CHECK([ip link set dev p2 up])
> +AT_CHECK([ovs-vsctl add-port br0 ovs-p2 -- set interface ovs-p2 ofport_request=4])
> +dnl Use p1 to check the truncated packet
> +AT_CHECK([ovs-vsctl add-port br0 p2 -- set interface p2 ofport_request=5])
> +
> +dnl test1: basic
> +AT_CHECK([ovs-ofctl add-flow br0 'in_port=3 dl_dst=e6:66:c1:22:22:22 actions=drop'])
> +AT_CHECK([ovs-ofctl add-flow br0 'in_port=5 dl_dst=e6:66:c1:22:22:22 actions=drop'])
> +AT_CHECK([ovs-ofctl add-flow br0 'in_port=1 dl_dst=e6:66:c1:22:22:22 actions=output(port=2,max_len=100),output:4'])
> +
> +NS_CHECK_EXEC([at_ns0], [ping -q -c 1 -s 1024 -w 0 10.1.1.2 | FORMAT_PING], [0], [dnl
> +1 packets transmitted, 0 received, 100% packet loss, time 0ms
> +])
> +AT_CHECK([ovs-ofctl dump-flows br0 table=0 | grep "in_port=3" |  sed -n 's/.*\(n\_bytes=100\).*/\1/p'], [0], [dnl
> +n_bytes=100
> +])
> +dnl packet with original size
> +AT_CHECK([ovs-ofctl dump-flows br0 table=0 | grep "in_port=5" | sed -n 's/.*\(n\_bytes=1066\).*/\1/p'], [0], [dnl
> +n_bytes=1066
> +])
> +
> +dnl test2: more complicated output actions
> +AT_CHECK([ovs-ofctl del-flows br0])
> +AT_CHECK([ovs-ofctl add-flow br0 'in_port=3 dl_dst=e6:66:c1:22:22:22 actions=drop'])
> +AT_CHECK([ovs-ofctl add-flow br0 'in_port=5 dl_dst=e6:66:c1:22:22:22 actions=drop'])
> +AT_CHECK([ovs-ofctl add-flow br0 'in_port=1 dl_dst=e6:66:c1:22:22:22 actions=output(port=2,max_len=100),output:4,output(port=2,max_len=100),output(port=4,max_len=100),output:2'])
> +
> +NS_CHECK_EXEC([at_ns0], [ping -q -c 1 -s 1024 -w 0 10.1.1.2 | FORMAT_PING], [0], [dnl
> +1 packets transmitted, 0 received, 100% packet loss, time 0ms
> +])
> +dnl 100 + 100 + 1066 = 1266
> +AT_CHECK([ovs-ofctl dump-flows br0 table=0 | grep "in_port=3" |  sed -n 's/.*\(n\_bytes=1266\).*/\1/p'], [0], [dnl
> +n_bytes=1266
> +])
> +dnl 1066 + 100 = 1166
> +AT_CHECK([ovs-ofctl dump-flows br0 table=0 | grep "in_port=5" | sed -n 's/.*\(n\_bytes=1166\).*/\1/p'], [0], [dnl
> +n_bytes=1166
> +])
> +
> +OVS_TRAFFIC_VSWITCHD_STOP
> +AT_CHECK([ip link del ovs-p1])
> +AT_CHECK([ip link del ovs-p2])

Rather than performing cleanup inline here, I think it's better to
queue up the cleanup immediately after adding the link, eg:

AT_CHECK([ip link add p1 type veth peer name ovs-p1])
on_exit "ip link del ovs-p1"

This means that whether the test passes or fails, the link will be cleaned up.

Did you also attempt to run the test using "make
check-system-userspace"? I had a little trouble with it, but I haven't
investigated what is causing the issue.



More information about the dev mailing list