| Message ID | 1459557431-3365-1-git-send-email-u9012063@gmail.com |
|---|---|
| State | Changes Requested |
| Headers | show |
On 1 April 2016 at 17:37, William Tu <u9012063@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@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.
On Fri, Apr 1, 2016 at 5:37 PM, William Tu <u9012063@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@gmail.com> > --- > v1->v2 > - Create new OpenFlow action, do not reuse OFPACT_OUTPUT > - Create new datapath action > > --- > datapath/actions.c | 18 +++- > datapath/datapath.h | 1 + > datapath/flow_netlink.c | 9 ++ > datapath/linux/compat/include/linux/openvswitch.h | 9 ++ > datapath/vport.c | 6 ++ > lib/dp-packet.c | 1 + > lib/dp-packet.h | 1 + > lib/dpif-netdev.c | 28 ++++++ > lib/dpif.c | 1 + > lib/netdev.c | 8 ++ > lib/odp-execute.c | 2 + > lib/odp-util.c | 11 ++- > lib/ofp-actions.c | 102 ++++++++++++++++++++++ > lib/ofp-actions.h | 10 +++ > ofproto/ofproto-dpif-sflow.c | 6 ++ > ofproto/ofproto-dpif-xlate.c | 39 +++++++++ > tests/ofproto-dpif.at | 53 +++++++++++ > tests/system-traffic.at | 66 ++++++++++++++ > 18 files changed, 366 insertions(+), 5 deletions(-) > > diff --git a/datapath/actions.c b/datapath/actions.c > index dcf8591..a1a1241 100644 > --- a/datapath/actions.c > +++ b/datapath/actions.c > @@ -738,10 +738,13 @@ 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); > > + if (unlikely(max_len != 0)) > + OVS_CB(skb)->max_len = max_len; > + > if (likely(vport)) { > u16 mru = OVS_CB(skb)->mru; > > @@ -1034,6 +1037,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,9 +1049,10 @@ 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; > + max_len = 0; > } > > switch (nla_type(a)) { > @@ -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; > + } > + The datapath truncate action should not be associated with output action. It should be general truncate action with truncate length parameter. This way we can apply it to actions other than output.
On Mon, Apr 11, 2016 at 01:14:26PM -0700, pravin shelar wrote: > The datapath truncate action should not be associated with output > action. It should be general truncate action with truncate length > parameter. This way we can apply it to actions other than output. This may require more care than perhaps you expect. For example, if the truncation removes a header, or part of a header, then the extracted flow may need to be updated. It might also mean that datapath actions to modify fields become invalid.
On Mon, Apr 11, 2016 at 5:58 PM, Ben Pfaff <blp@ovn.org> wrote: > On Mon, Apr 11, 2016 at 01:14:26PM -0700, pravin shelar wrote: >> The datapath truncate action should not be associated with output >> action. It should be general truncate action with truncate length >> parameter. This way we can apply it to actions other than output. > > This may require more care than perhaps you expect. For example, if the > truncation removes a header, or part of a header, then the extracted > flow may need to be updated. It might also mean that datapath actions > to modify fields become invalid. There is no need to truncate packet immediately. We can keep the truncate len in skb_ovs_cb and use it to truncate packet right before seeing it over port or to user-space up-call.
On Mon, Apr 11, 2016 at 06:46:27PM -0700, pravin shelar wrote: > On Mon, Apr 11, 2016 at 5:58 PM, Ben Pfaff <blp@ovn.org> wrote: > > On Mon, Apr 11, 2016 at 01:14:26PM -0700, pravin shelar wrote: > >> The datapath truncate action should not be associated with output > >> action. It should be general truncate action with truncate length > >> parameter. This way we can apply it to actions other than output. > > > > This may require more care than perhaps you expect. For example, if the > > truncation removes a header, or part of a header, then the extracted > > flow may need to be updated. It might also mean that datapath actions > > to modify fields become invalid. > > There is no need to truncate packet immediately. We can keep the > truncate len in skb_ovs_cb and use it to truncate packet right before > seeing it over port or to user-space up-call. OK, that's a different situation, I agree.
Hi Pravin and Ben, Thanks for the feedback and yes, it's better to make truncate without associating to the output action. I will make the modification and resubmit v3 patch. Regards, William On Mon, Apr 11, 2016 at 6:49 PM, Ben Pfaff <blp@ovn.org> wrote: > On Mon, Apr 11, 2016 at 06:46:27PM -0700, pravin shelar wrote: > > On Mon, Apr 11, 2016 at 5:58 PM, Ben Pfaff <blp@ovn.org> wrote: > > > On Mon, Apr 11, 2016 at 01:14:26PM -0700, pravin shelar wrote: > > >> The datapath truncate action should not be associated with output > > >> action. It should be general truncate action with truncate length > > >> parameter. This way we can apply it to actions other than output. > > > > > > This may require more care than perhaps you expect. For example, if > the > > > truncation removes a header, or part of a header, then the extracted > > > flow may need to be updated. It might also mean that datapath actions > > > to modify fields become invalid. > > > > There is no need to truncate packet immediately. We can keep the > > truncate len in skb_ovs_cb and use it to truncate packet right before > > seeing it over port or to user-space up-call. > > OK, that's a different situation, I agree. >
On 11 April 2016 at 18:46, pravin shelar <pshelar@ovn.org> wrote: > On Mon, Apr 11, 2016 at 5:58 PM, Ben Pfaff <blp@ovn.org> wrote: >> On Mon, Apr 11, 2016 at 01:14:26PM -0700, pravin shelar wrote: >>> The datapath truncate action should not be associated with output >>> action. It should be general truncate action with truncate length >>> parameter. This way we can apply it to actions other than output. >> >> This may require more care than perhaps you expect. For example, if the >> truncation removes a header, or part of a header, then the extracted >> flow may need to be updated. It might also mean that datapath actions >> to modify fields become invalid. > > There is no need to truncate packet immediately. We can keep the > truncate len in skb_ovs_cb and use it to truncate packet right before > seeing it over port or to user-space up-call. So, if you have a packet with length 100, then do the following actions: truncate(64),output(1),truncate(100),output(2), then port 1 will see the truncated packet but port 2 will see the full (100 bytes of) packet?
Hi Joe, > There is no need to truncate packet immediately. We can keep the > > truncate len in skb_ovs_cb and use it to truncate packet right before > > seeing it over port or to user-space up-call. > > So, if you have a packet with length 100, then do the following actions: > > truncate(64),output(1),truncate(100),output(2), then port 1 will see > the truncated packet but port 2 will see the full (100 bytes of) > packet? > Yes, when seeing a truncate(64) action, we set len=64 in skb_ovs_cb, and it is until output(1) then we do the real truncate, clear the len. So the next truncate(100) will start over with a new packet and output(2) should see the full packet. Regards, William
On 12 April 2016 at 11:10, William Tu <u9012063@gmail.com> wrote: > Hi Joe, > >> There is no need to truncate packet immediately. We can keep the >> >> > truncate len in skb_ovs_cb and use it to truncate packet right before >> > seeing it over port or to user-space up-call. >> >> So, if you have a packet with length 100, then do the following actions: >> >> truncate(64),output(1),truncate(100),output(2), then port 1 will see >> the truncated packet but port 2 will see the full (100 bytes of) >> packet? > > > Yes, when seeing a truncate(64) action, we set len=64 in skb_ovs_cb, and it > is until output(1) then we do the real truncate, clear the len. > So the next truncate(100) will start over with a new packet and output(2) > should see the full packet. Ah okay, so the following would also forward a 64B packet to port 1 and 100B packet to port 2 in this situation: truncate(64),output(1),output(2)
Should we expose "truncate" to the ovs-ofctl action list?
I was thinking about this ovs-ofctl syntax:
actions='output(max_len=64, port=1), output:2'
then at datapath it translates to actions
truncate(64), output(1), output(2)
So 64B to port1, and 100B to port2.
Regards,
William
On Tue, Apr 12, 2016 at 11:20 AM, Joe Stringer <joe@ovn.org> wrote:
> On 12 April 2016 at 11:10, William Tu <u9012063@gmail.com> wrote:
> > Hi Joe,
> >
> >> There is no need to truncate packet immediately. We can keep the
> >>
> >> > truncate len in skb_ovs_cb and use it to truncate packet right before
> >> > seeing it over port or to user-space up-call.
> >>
> >> So, if you have a packet with length 100, then do the following actions:
> >>
> >> truncate(64),output(1),truncate(100),output(2), then port 1 will see
> >> the truncated packet but port 2 will see the full (100 bytes of)
> >> packet?
> >
> >
> > Yes, when seeing a truncate(64) action, we set len=64 in skb_ovs_cb, and
> it
> > is until output(1) then we do the real truncate, clear the len.
> > So the next truncate(100) will start over with a new packet and output(2)
> > should see the full packet.
>
> Ah okay, so the following would also forward a 64B packet to port 1
> and 100B packet to port 2 in this situation:
>
> truncate(64),output(1),output(2)
>
On Tue, Apr 12, 2016 at 11:38:38AM -0700, William Tu wrote: > Should we expose "truncate" to the ovs-ofctl action list? > > I was thinking about this ovs-ofctl syntax: > actions='output(max_len=64, port=1), output:2' > > then at datapath it translates to actions > truncate(64), output(1), output(2) > > So 64B to port1, and 100B to port2. I think that's OK. Pravin or Joe, do you have an opinion?
On 12 April 2016 at 21:13, Ben Pfaff <blp@ovn.org> wrote: > On Tue, Apr 12, 2016 at 11:38:38AM -0700, William Tu wrote: >> Should we expose "truncate" to the ovs-ofctl action list? >> >> I was thinking about this ovs-ofctl syntax: >> actions='output(max_len=64, port=1), output:2' >> >> then at datapath it translates to actions >> truncate(64), output(1), output(2) >> >> So 64B to port1, and 100B to port2. > > I think that's OK. > > Pravin or Joe, do you have an opinion? Seems fine. As an aside, it might be worth creating some tests that output to a bond port to ensure that case works, in addition to the existing cases.
On Tue, Apr 12, 2016 at 9:13 PM, Ben Pfaff <blp@ovn.org> wrote: > On Tue, Apr 12, 2016 at 11:38:38AM -0700, William Tu wrote: >> Should we expose "truncate" to the ovs-ofctl action list? >> >> I was thinking about this ovs-ofctl syntax: >> actions='output(max_len=64, port=1), output:2' >> >> then at datapath it translates to actions >> truncate(64), output(1), output(2) >> >> So 64B to port1, and 100B to port2. > > I think that's OK. > It looks good to me too.
On Tue, Apr 12, 2016 at 10:56:50PM -0700, Joe Stringer wrote: > On 12 April 2016 at 21:13, Ben Pfaff <blp@ovn.org> wrote: > > On Tue, Apr 12, 2016 at 11:38:38AM -0700, William Tu wrote: > >> Should we expose "truncate" to the ovs-ofctl action list? > >> > >> I was thinking about this ovs-ofctl syntax: > >> actions='output(max_len=64, port=1), output:2' > >> > >> then at datapath it translates to actions > >> truncate(64), output(1), output(2) > >> > >> So 64B to port1, and 100B to port2. > > > > I think that's OK. > > > > Pravin or Joe, do you have an opinion? > > Seems fine. > > As an aside, it might be worth creating some tests that output to a > bond port to ensure that case works, in addition to the existing > cases. I don't know what that means; there are no "bond ports" at the OpenFlow level.
On 13 April 2016 at 16:12, Ben Pfaff <blp@ovn.org> wrote: > On Tue, Apr 12, 2016 at 10:56:50PM -0700, Joe Stringer wrote: >> On 12 April 2016 at 21:13, Ben Pfaff <blp@ovn.org> wrote: >> > On Tue, Apr 12, 2016 at 11:38:38AM -0700, William Tu wrote: >> >> Should we expose "truncate" to the ovs-ofctl action list? >> >> >> >> I was thinking about this ovs-ofctl syntax: >> >> actions='output(max_len=64, port=1), output:2' >> >> >> >> then at datapath it translates to actions >> >> truncate(64), output(1), output(2) >> >> >> >> So 64B to port1, and 100B to port2. >> > >> > I think that's OK. >> > >> > Pravin or Joe, do you have an opinion? >> >> Seems fine. >> >> As an aside, it might be worth creating some tests that output to a >> bond port to ensure that case works, in addition to the existing >> cases. > > I don't know what that means; there are no "bond ports" at the OpenFlow > level. I meant, to configure a bond and use that port as the output, to check for corner cases where the datapath flows break up the output across two flows (recirc + actual output)
On Wed, Apr 13, 2016 at 05:06:16PM -0700, Joe Stringer wrote: > On 13 April 2016 at 16:12, Ben Pfaff <blp@ovn.org> wrote: > > On Tue, Apr 12, 2016 at 10:56:50PM -0700, Joe Stringer wrote: > >> On 12 April 2016 at 21:13, Ben Pfaff <blp@ovn.org> wrote: > >> > On Tue, Apr 12, 2016 at 11:38:38AM -0700, William Tu wrote: > >> >> Should we expose "truncate" to the ovs-ofctl action list? > >> >> > >> >> I was thinking about this ovs-ofctl syntax: > >> >> actions='output(max_len=64, port=1), output:2' > >> >> > >> >> then at datapath it translates to actions > >> >> truncate(64), output(1), output(2) > >> >> > >> >> So 64B to port1, and 100B to port2. > >> > > >> > I think that's OK. > >> > > >> > Pravin or Joe, do you have an opinion? > >> > >> Seems fine. > >> > >> As an aside, it might be worth creating some tests that output to a > >> bond port to ensure that case works, in addition to the existing > >> cases. > > > > I don't know what that means; there are no "bond ports" at the OpenFlow > > level. > > I meant, to configure a bond and use that port as the output, to check > for corner cases where the datapath flows break up the output across > two flows (recirc + actual output) Oh, I guess that could be possible if one used "output(max_len=64, port=NORMAL)". It's quite a corner case!
On Wed, Apr 13, 2016 at 05:17:13PM -0700, Ben Pfaff wrote: > On Wed, Apr 13, 2016 at 05:06:16PM -0700, Joe Stringer wrote: > > On 13 April 2016 at 16:12, Ben Pfaff <blp@ovn.org> wrote: > > > On Tue, Apr 12, 2016 at 10:56:50PM -0700, Joe Stringer wrote: > > >> On 12 April 2016 at 21:13, Ben Pfaff <blp@ovn.org> wrote: > > >> > On Tue, Apr 12, 2016 at 11:38:38AM -0700, William Tu wrote: > > >> >> Should we expose "truncate" to the ovs-ofctl action list? > > >> >> > > >> >> I was thinking about this ovs-ofctl syntax: > > >> >> actions='output(max_len=64, port=1), output:2' > > >> >> > > >> >> then at datapath it translates to actions > > >> >> truncate(64), output(1), output(2) > > >> >> > > >> >> So 64B to port1, and 100B to port2. > > >> > > > >> > I think that's OK. > > >> > > > >> > Pravin or Joe, do you have an opinion? > > >> > > >> Seems fine. > > >> > > >> As an aside, it might be worth creating some tests that output to a > > >> bond port to ensure that case works, in addition to the existing > > >> cases. > > > > > > I don't know what that means; there are no "bond ports" at the OpenFlow > > > level. > > > > I meant, to configure a bond and use that port as the output, to check > > for corner cases where the datapath flows break up the output across > > two flows (recirc + actual output) > > Oh, I guess that could be possible if one used "output(max_len=64, > port=NORMAL)". It's quite a corner case! Also I wonder whether truncated output to FLOOD or ALL should be well-defined. We could just forbid them I suppose.
> On Apr 13, 2016, at 5:17 PM, Ben Pfaff <blp@ovn.org> wrote: > > On Wed, Apr 13, 2016 at 05:17:13PM -0700, Ben Pfaff wrote: >> On Wed, Apr 13, 2016 at 05:06:16PM -0700, Joe Stringer wrote: >>> On 13 April 2016 at 16:12, Ben Pfaff <blp@ovn.org> wrote: >>>> On Tue, Apr 12, 2016 at 10:56:50PM -0700, Joe Stringer wrote: >>>>> On 12 April 2016 at 21:13, Ben Pfaff <blp@ovn.org> wrote: >>>>>> On Tue, Apr 12, 2016 at 11:38:38AM -0700, William Tu wrote: >>>>>>> Should we expose "truncate" to the ovs-ofctl action list? >>>>>>> >>>>>>> I was thinking about this ovs-ofctl syntax: >>>>>>> actions='output(max_len=64, port=1), output:2' >>>>>>> >>>>>>> then at datapath it translates to actions >>>>>>> truncate(64), output(1), output(2) >>>>>>> >>>>>>> So 64B to port1, and 100B to port2. >>>>>> >>>>>> I think that's OK. >>>>>> >>>>>> Pravin or Joe, do you have an opinion? >>>>> >>>>> Seems fine. >>>>> >>>>> As an aside, it might be worth creating some tests that output to a >>>>> bond port to ensure that case works, in addition to the existing >>>>> cases. >>>> >>>> I don't know what that means; there are no "bond ports" at the OpenFlow >>>> level. >>> >>> I meant, to configure a bond and use that port as the output, to check >>> for corner cases where the datapath flows break up the output across >>> two flows (recirc + actual output) >> >> Oh, I guess that could be possible if one used "output(max_len=64, >> port=NORMAL)". It's quite a corner case! > > Also I wonder whether truncated output to FLOOD or ALL should be > well-defined. We could just forbid them I suppose. How about mirrored output when the original output is truncated? I guess the mirrors should also see truncated output. Jarno > _______________________________________________ > dev mailing list > dev@openvswitch.org <mailto:dev@openvswitch.org> > http://openvswitch.org/mailman/listinfo/dev <http://openvswitch.org/mailman/listinfo/dev>
diff --git a/datapath/actions.c b/datapath/actions.c index dcf8591..a1a1241 100644 --- a/datapath/actions.c +++ b/datapath/actions.c @@ -738,10 +738,13 @@ 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); + if (unlikely(max_len != 0)) + OVS_CB(skb)->max_len = max_len; + if (likely(vport)) { u16 mru = OVS_CB(skb)->mru; @@ -1034,6 +1037,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,9 +1049,10 @@ 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; + max_len = 0; } switch (nla_type(a)) { @@ -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; + } + case OVS_ACTION_ATTR_USERSPACE: output_userspace(dp, skb, key, a, attr, len); break; @@ -1126,7 +1138,7 @@ static int do_execute_actions(struct datapath *dp, struct sk_buff *skb, } if (prev_port != -1) - do_output(dp, skb, prev_port, key); + do_output(dp, skb, prev_port, max_len, key); else consume_skb(skb); diff --git a/datapath/datapath.h b/datapath/datapath.h index ceb3372..178d81d 100644 --- a/datapath/datapath.h +++ b/datapath/datapath.h @@ -102,6 +102,7 @@ struct datapath { struct ovs_skb_cb { struct vport *input_vport; u16 mru; + u16 max_len; /* 0 means original packet size. */ }; #define OVS_CB(skb) ((struct ovs_skb_cb *)(skb)->cb) diff --git a/datapath/flow_netlink.c b/datapath/flow_netlink.c index 6ffcc53..4fc4f7b 100644 --- a/datapath/flow_netlink.c +++ b/datapath/flow_netlink.c @@ -2170,6 +2170,7 @@ static int __ovs_nla_copy_actions(struct net *net, const struct nlattr *attr, /* Expected argument lengths, (u32)-1 for variable length. */ static const u32 action_lens[OVS_ACTION_ATTR_MAX + 1] = { [OVS_ACTION_ATTR_OUTPUT] = sizeof(u32), + [OVS_ACTION_ATTR_OUTPUT_TRUNC] = sizeof(struct ovs_action_output_trunc), [OVS_ACTION_ATTR_RECIRC] = sizeof(u32), [OVS_ACTION_ATTR_USERSPACE] = (u32)-1, [OVS_ACTION_ATTR_PUSH_MPLS] = sizeof(struct ovs_action_push_mpls), @@ -2207,6 +2208,14 @@ static int __ovs_nla_copy_actions(struct net *net, const struct nlattr *attr, return -EINVAL; break; + case OVS_ACTION_ATTR_OUTPUT_TRUNC: { + const struct ovs_action_output_trunc *output_trunc = nla_data(a); + if (output_trunc->port >= DP_MAX_PORTS || + output_trunc->max_len < OVS_ACTION_OUTPUT_MIN) + return -EINVAL; + break; + } + case OVS_ACTION_ATTR_HASH: { const struct ovs_action_hash *act_hash = nla_data(a); diff --git a/datapath/linux/compat/include/linux/openvswitch.h b/datapath/linux/compat/include/linux/openvswitch.h index 3b39ebb..4574eb1 100644 --- a/datapath/linux/compat/include/linux/openvswitch.h +++ b/datapath/linux/compat/include/linux/openvswitch.h @@ -600,6 +600,13 @@ enum ovs_userspace_attr { #define OVS_USERSPACE_ATTR_MAX (__OVS_USERSPACE_ATTR_MAX - 1) +struct ovs_action_output_trunc { + uint32_t port; + uint16_t max_len; /* 0 means original packet size. */ +}; +/* Minimum packet size max_len can have, 60 = ETH_MIN_FRAME_LEN. */ +#define OVS_ACTION_OUTPUT_MIN 60 + /** * struct ovs_action_push_mpls - %OVS_ACTION_ATTR_PUSH_MPLS action argument. * @mpls_lse: MPLS label stack entry to push. @@ -742,6 +749,7 @@ enum ovs_nat_attr { * enum ovs_action_attr - Action types. * * @OVS_ACTION_ATTR_OUTPUT: Output packet to port. + * @OVS_ACTION_ATTR_TRUNC: Output packet to port with truncated packet size. * @OVS_ACTION_ATTR_USERSPACE: Send packet to userspace according to nested * %OVS_USERSPACE_ATTR_* attributes. * @OVS_ACTION_ATTR_PUSH_VLAN: Push a new outermost 802.1Q header onto the @@ -788,6 +796,7 @@ enum ovs_nat_attr { enum ovs_action_attr { OVS_ACTION_ATTR_UNSPEC, OVS_ACTION_ATTR_OUTPUT, /* u32 port number. */ + OVS_ACTION_ATTR_OUTPUT_TRUNC, /* u32 port number + u16 max_len */ OVS_ACTION_ATTR_USERSPACE, /* Nested OVS_USERSPACE_ATTR_*. */ OVS_ACTION_ATTR_SET, /* One nested OVS_KEY_ATTR_*. */ OVS_ACTION_ATTR_PUSH_VLAN, /* struct ovs_action_push_vlan. */ diff --git a/datapath/vport.c b/datapath/vport.c index 44b9dfb..31a6128 100644 --- a/datapath/vport.c +++ b/datapath/vport.c @@ -487,6 +487,8 @@ int ovs_vport_receive(struct vport *vport, struct sk_buff *skb, OVS_CB(skb)->input_vport = vport; OVS_CB(skb)->mru = 0; + OVS_CB(skb)->max_len = 0; + if (unlikely(dev_net(skb->dev) != ovs_dp_get_net(vport->dp))) { u32 mark; @@ -615,6 +617,7 @@ static unsigned int packet_length(const struct sk_buff *skb) void ovs_vport_send(struct vport *vport, struct sk_buff *skb) { int mtu = vport->dev->mtu; + u16 max_len = OVS_CB(skb)->max_len; if (unlikely(packet_length(skb) > mtu && !skb_is_gso(skb))) { net_warn_ratelimited("%s: dropped over-mtu packet: %d > %d\n", @@ -624,6 +627,9 @@ void ovs_vport_send(struct vport *vport, struct sk_buff *skb) goto drop; } + if (unlikely(max_len >= OVS_ACTION_OUTPUT_MIN)) + skb_trim(skb, max_len); + skb->dev = vport->dev; vport->ops->send(skb); return; diff --git a/lib/dp-packet.c b/lib/dp-packet.c index aec7fe7..d32fa85 100644 --- a/lib/dp-packet.c +++ b/lib/dp-packet.c @@ -29,6 +29,7 @@ dp_packet_init__(struct dp_packet *b, size_t allocated, enum dp_packet_source so b->source = source; dp_packet_reset_offsets(b); pkt_metadata_init(&b->md, 0); + b->max_len = 0; } static void diff --git a/lib/dp-packet.h b/lib/dp-packet.h index 000b09d..9f10fa8 100644 --- a/lib/dp-packet.h +++ b/lib/dp-packet.h @@ -58,6 +58,7 @@ struct dp_packet { * or UINT16_MAX. */ uint16_t l4_ofs; /* Transport-level header offset, or UINT16_MAX. */ + uint16_t max_len; /* Max packet sent size. 0 means original size. */ struct pkt_metadata md; }; diff --git a/lib/dpif-netdev.c b/lib/dpif-netdev.c index 119dc2d..4b9d559 100644 --- a/lib/dpif-netdev.c +++ b/lib/dpif-netdev.c @@ -3744,6 +3744,34 @@ dp_execute_cb(void *aux_, struct dp_packet **packets, int cnt, int i; switch ((enum ovs_action_attr)type) { + case OVS_ACTION_ATTR_OUTPUT_TRUNC: { + struct dp_packet *trunc_pkts[cnt]; + const struct ovs_action_output_trunc *output_trunc = + nl_attr_get_unspec(a, sizeof *output_trunc); + + p = dp_netdev_lookup_port(dp, output_trunc->port); + + if (output_trunc->max_len >= OVS_ACTION_OUTPUT_MIN) { + if (!may_steal) { + dp_netdev_clone_pkt_batch(trunc_pkts, packets, cnt); + packets = trunc_pkts; + } + for (i = 0; i < cnt; i++) { + packets[i]->max_len = output_trunc->max_len; + } + } + + if (OVS_LIKELY(p)) { + int tx_qid; + + atomic_read_relaxed(&pmd->tx_qid, &tx_qid); + + netdev_send(p->netdev, tx_qid, packets, cnt, may_steal); + return; + } + break; + } + case OVS_ACTION_ATTR_OUTPUT: p = dp_netdev_lookup_port(dp, u32_to_odp(nl_attr_get_u32(a))); if (OVS_LIKELY(p)) { diff --git a/lib/dpif.c b/lib/dpif.c index a784de7..272bd24 100644 --- a/lib/dpif.c +++ b/lib/dpif.c @@ -1099,6 +1099,7 @@ dpif_execute_helper_cb(void *aux_, struct dp_packet **packets, int cnt, switch ((enum ovs_action_attr)type) { case OVS_ACTION_ATTR_CT: case OVS_ACTION_ATTR_OUTPUT: + case OVS_ACTION_ATTR_OUTPUT_TRUNC: case OVS_ACTION_ATTR_TUNNEL_PUSH: case OVS_ACTION_ATTR_TUNNEL_POP: case OVS_ACTION_ATTR_USERSPACE: diff --git a/lib/netdev.c b/lib/netdev.c index 3e50694..92201c6 100644 --- a/lib/netdev.c +++ b/lib/netdev.c @@ -758,6 +758,14 @@ netdev_send(struct netdev *netdev, int qid, struct dp_packet **buffers, return EOPNOTSUPP; } + for (int i = 0; i < cnt; i++) { + struct dp_packet *packet = buffers[i]; + if (packet->max_len >= OVS_ACTION_OUTPUT_MIN && + packet->max_len < dp_packet_size(packet)) { + dp_packet_set_size(packet, packet->max_len); + } + } + int error = netdev->netdev_class->send(netdev, qid, buffers, cnt, may_steal); if (!error) { diff --git a/lib/odp-execute.c b/lib/odp-execute.c index b5204b2..4281334 100644 --- a/lib/odp-execute.c +++ b/lib/odp-execute.c @@ -488,6 +488,7 @@ requires_datapath_assistance(const struct nlattr *a) switch (type) { /* These only make sense in the context of a datapath. */ case OVS_ACTION_ATTR_OUTPUT: + case OVS_ACTION_ATTR_OUTPUT_TRUNC: case OVS_ACTION_ATTR_TUNNEL_PUSH: case OVS_ACTION_ATTR_TUNNEL_POP: case OVS_ACTION_ATTR_USERSPACE: @@ -624,6 +625,7 @@ odp_execute_actions(void *dp, struct dp_packet **packets, int cnt, bool steal, break; case OVS_ACTION_ATTR_OUTPUT: + case OVS_ACTION_ATTR_OUTPUT_TRUNC: case OVS_ACTION_ATTR_TUNNEL_PUSH: case OVS_ACTION_ATTR_TUNNEL_POP: case OVS_ACTION_ATTR_USERSPACE: diff --git a/lib/odp-util.c b/lib/odp-util.c index b4689cc..a4b4802 100644 --- a/lib/odp-util.c +++ b/lib/odp-util.c @@ -107,6 +107,7 @@ odp_action_len(uint16_t type) switch ((enum ovs_action_attr) type) { case OVS_ACTION_ATTR_OUTPUT: return sizeof(uint32_t); + case OVS_ACTION_ATTR_OUTPUT_TRUNC: return sizeof(struct ovs_action_output_trunc); case OVS_ACTION_ATTR_TUNNEL_PUSH: return ATTR_LEN_VARIABLE; case OVS_ACTION_ATTR_TUNNEL_POP: return sizeof(uint32_t); case OVS_ACTION_ATTR_USERSPACE: return ATTR_LEN_VARIABLE; @@ -775,6 +776,14 @@ format_odp_action(struct ds *ds, const struct nlattr *a) case OVS_ACTION_ATTR_OUTPUT: ds_put_format(ds, "%"PRIu32, nl_attr_get_u32(a)); break; + case OVS_ACTION_ATTR_OUTPUT_TRUNC: { + const struct ovs_action_output_trunc *output_trunc = + nl_attr_get_unspec(a, sizeof *output_trunc); + ds_put_format(ds, "(port=%"PRIu32",max_len=%"PRIu16")", + output_trunc->port, output_trunc->max_len); + break; + } + break; case OVS_ACTION_ATTR_TUNNEL_POP: ds_put_format(ds, "tnl_pop(%"PRIu32")", nl_attr_get_u32(a)); break; @@ -1516,13 +1525,11 @@ parse_odp_action(const char *s, const struct simap *port_names, { uint32_t port; int n; - if (ovs_scan(s, "%"SCNi32"%n", &port, &n)) { nl_msg_put_u32(actions, OVS_ACTION_ATTR_OUTPUT, port); return n; } } - if (port_names) { int len = strcspn(s, delimiters); struct simap_node *node; diff --git a/lib/ofp-actions.c b/lib/ofp-actions.c index aac4ff0..9374380 100644 --- a/lib/ofp-actions.c +++ b/lib/ofp-actions.c @@ -299,6 +299,9 @@ enum ofp_raw_action_type { /* NX1.0+(36): struct nx_action_nat, ... */ NXAST_RAW_NAT, + /* NX1.0+(38): struct nx_action_output_trunc. */ + NXAST_RAW_OUTPUT_TRUNC, + /* ## ------------------ ## */ /* ## Debugging actions. ## */ /* ## ------------------ ## */ @@ -379,6 +382,7 @@ ofpact_next_flattened(const struct ofpact *ofpact) case OFPACT_CONTROLLER: case OFPACT_ENQUEUE: case OFPACT_OUTPUT_REG: + case OFPACT_OUTPUT_TRUNC: case OFPACT_BUNDLE: case OFPACT_SET_FIELD: case OFPACT_SET_VLAN_VID: @@ -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) { @@ -545,6 +579,10 @@ parse_OUTPUT(const char *arg, struct ofpbuf *ofpacts, output_reg = ofpact_put_OUTPUT_REG(ofpacts); output_reg->max_len = UINT16_MAX; return mf_parse_subfield(&output_reg->src, arg); + } else if (strstr(arg, "port=") && strstr(arg, "max_len=")) { + struct ofpact_output_trunc *output_trunc; + output_trunc = ofpact_put_OUTPUT_TRUNC(ofpacts); + return parse_truncate_subfield(output_trunc, arg); } else { struct ofpact_output *output; @@ -5512,6 +5550,62 @@ parse_NAT(char *arg, struct ofpbuf *ofpacts, return NULL; } +/* Truncate output action. */ +struct nx_action_output_trunc { + ovs_be16 type; /* OFPAT_VENDOR. */ + ovs_be16 len; /* At least 16. */ + ovs_be32 vendor; /* NX_VENDOR_ID. */ + ovs_be16 subtype; /* NXAST_OUTPUT_TRUNC. */ + ovs_be16 max_len; /* Truncate packet to size bytes */ + ovs_be32 port; /* Output port */ +}; +OFP_ASSERT(sizeof(struct nx_action_output_trunc) == 16); + +static enum ofperr +decode_NXAST_RAW_OUTPUT_TRUNC(const struct nx_action_output_trunc *natrc, + enum ofp_version ofp_version OVS_UNUSED, + struct ofpbuf *out) +{ + struct ofpact_output_trunc *output_trunc; + + output_trunc = ofpact_put_OUTPUT_TRUNC(out); + output_trunc->max_len = ntohs(natrc->max_len); + output_trunc->port = ntohl(natrc->port); + + if (output_trunc->max_len < OVS_ACTION_OUTPUT_MIN) { + return OFPERR_OFPBAC_BAD_ARGUMENT; + } + return 0; +} + +static void +encode_OUTPUT_TRUNC(const struct ofpact_output_trunc *output_trunc, + enum ofp_version ofp_version OVS_UNUSED, + struct ofpbuf *out) +{ + struct nx_action_output_trunc *natrc = put_NXAST_OUTPUT_TRUNC(out); + + natrc->max_len = htons(output_trunc->max_len); + natrc->port = htonl(output_trunc->port); +} + +static char * OVS_WARN_UNUSED_RESULT +parse_OUTPUT_TRUNC(const char *arg OVS_UNUSED, struct ofpbuf *ofpacts OVS_UNUSED, + enum ofputil_protocol *usable_protocols OVS_UNUSED) +{ + OVS_NOT_REACHED(); /* output_trunc action uses parse_OUTPUT. */ + return NULL; +} + +static void +format_OUTPUT_TRUNC(const struct ofpact_output_trunc *a, struct ds *s) +{ + if (ofp_to_u16(a->port) < ofp_to_u16(OFPP_MAX)) { + ds_put_format(s, "%soutput%s(port=%"PRIu16",max_len=%"PRIu16")", + colors.special, colors.end, a->port, a->max_len); + } +} + /* Meter instruction. */ @@ -5906,6 +6000,7 @@ ofpact_is_set_or_move_action(const struct ofpact *a) case OFPACT_NOTE: case OFPACT_OUTPUT: case OFPACT_OUTPUT_REG: + case OFPACT_OUTPUT_TRUNC: case OFPACT_POP_MPLS: case OFPACT_POP_QUEUE: case OFPACT_PUSH_MPLS: @@ -5934,6 +6029,7 @@ ofpact_is_allowed_in_actions_set(const struct ofpact *a) case OFPACT_DEC_TTL: case OFPACT_GROUP: case OFPACT_OUTPUT: + case OFPACT_OUTPUT_TRUNC: case OFPACT_POP_MPLS: case OFPACT_PUSH_MPLS: case OFPACT_PUSH_VLAN: @@ -6157,6 +6253,7 @@ ovs_instruction_type_from_ofpact_type(enum ofpact_type type) case OFPACT_CONTROLLER: case OFPACT_ENQUEUE: case OFPACT_OUTPUT_REG: + case OFPACT_OUTPUT_TRUNC: case OFPACT_BUNDLE: case OFPACT_SET_VLAN_VID: case OFPACT_SET_VLAN_PCP: @@ -6585,6 +6682,10 @@ ofpact_check__(enum ofputil_protocol *usable_protocols, struct ofpact *a, case OFPACT_OUTPUT_REG: return mf_check_src(&ofpact_get_OUTPUT_REG(a)->src, flow); + case OFPACT_OUTPUT_TRUNC: + return ofpact_check_output_port(ofpact_get_OUTPUT_TRUNC(a)->port, + max_ports); + case OFPACT_BUNDLE: return bundle_check(ofpact_get_BUNDLE(a), max_ports, flow); @@ -7261,6 +7362,7 @@ ofpact_outputs_to_port(const struct ofpact *ofpact, ofp_port_t port) return port == OFPP_CONTROLLER; case OFPACT_OUTPUT_REG: + case OFPACT_OUTPUT_TRUNC: case OFPACT_BUNDLE: case OFPACT_SET_VLAN_VID: case OFPACT_SET_VLAN_PCP: diff --git a/lib/ofp-actions.h b/lib/ofp-actions.h index 4bd8854..2df7841 100644 --- a/lib/ofp-actions.h +++ b/lib/ofp-actions.h @@ -108,6 +108,7 @@ OFPACT(UNROLL_XLATE, ofpact_unroll_xlate, ofpact, "unroll_xlate") \ OFPACT(CT, ofpact_conntrack, ofpact, "ct") \ OFPACT(NAT, ofpact_nat, ofpact, "nat") \ + OFPACT(OUTPUT_TRUNC, ofpact_output_trunc,ofpact, "output_trunc") \ \ /* Debugging actions. \ * \ @@ -290,6 +291,15 @@ struct ofpact_output_reg { struct mf_subfield src; }; +/* OFPACT_OUTPUT_TRUNC. + * + * Used for NXAST_OUTPUT_TRUNC. */ +struct ofpact_output_trunc { + struct ofpact ofpact; + ofp_port_t port; /* Output port. */ + uint16_t max_len; /* Max send len. */ +}; + /* Bundle slave choice algorithm to apply. * * In the descriptions below, 'slaves' is the list of possible slaves in the diff --git a/ofproto/ofproto-dpif-sflow.c b/ofproto/ofproto-dpif-sflow.c index fbc82b7..dde4cc6 100644 --- a/ofproto/ofproto-dpif-sflow.c +++ b/ofproto/ofproto-dpif-sflow.c @@ -1105,6 +1105,12 @@ dpif_sflow_read_actions(const struct flow *flow, sflow_actions->out_port = u32_to_odp(nl_attr_get_u32(a)); break; + case OVS_ACTION_ATTR_OUTPUT_TRUNC: { + const struct ovs_action_output_trunc *output = + nl_attr_get_unspec(a, sizeof *output); + sflow_actions->out_port = u32_to_odp(output->port); + break; + } case OVS_ACTION_ATTR_TUNNEL_POP: /* XXX: Do not handle this for now. It's not clear * if we should start with encap_depth == 1 when we diff --git a/ofproto/ofproto-dpif-xlate.c b/ofproto/ofproto-dpif-xlate.c index a02dc24..bf3d47a 100644 --- a/ofproto/ofproto-dpif-xlate.c +++ b/ofproto/ofproto-dpif-xlate.c @@ -3925,6 +3925,39 @@ xlate_output_reg_action(struct xlate_ctx *ctx, } static void +xlate_output_trunc_action(struct xlate_ctx *ctx, + ofp_port_t port, uint16_t max_len) +{ + odp_port_t odp_port; + struct ovs_action_output_trunc *output_trunc; + + switch (port) { + case OFPP_IN_PORT: + case OFPP_TABLE: + case OFPP_NORMAL: + case OFPP_FLOOD: + case OFPP_ALL: + case OFPP_CONTROLLER: + case OFPP_NONE: + case OFPP_LOCAL: + xlate_report(ctx, "output_trunc does not support this port"); + break; + default: + if (port != ctx->xin->flow.in_port.ofp_port) { + output_trunc = nl_msg_put_unspec_uninit(ctx->odp_actions, + OVS_ACTION_ATTR_OUTPUT_TRUNC, + sizeof *output_trunc); + odp_port = ofp_port_to_odp_port(ctx->xbridge, port); + output_trunc->port = odp_port; + output_trunc->max_len = max_len; + } else { + xlate_report(ctx, "skipping output to input port"); + } + break; + } +} + +static void xlate_enqueue_action(struct xlate_ctx *ctx, const struct ofpact_enqueue *enqueue) { @@ -4209,6 +4242,7 @@ freeze_unroll_actions(const struct ofpact *a, const struct ofpact *end, for (; a < end; a = ofpact_next(a)) { switch (a->type) { case OFPACT_OUTPUT_REG: + case OFPACT_OUTPUT_TRUNC: case OFPACT_GROUP: case OFPACT_OUTPUT: case OFPACT_CONTROLLER: @@ -4714,6 +4748,11 @@ do_xlate_actions(const struct ofpact *ofpacts, size_t ofpacts_len, xlate_output_reg_action(ctx, ofpact_get_OUTPUT_REG(a)); break; + case OFPACT_OUTPUT_TRUNC: + xlate_output_trunc_action(ctx, ofpact_get_OUTPUT_TRUNC(a)->port, + ofpact_get_OUTPUT_TRUNC(a)->max_len); + break; + case OFPACT_LEARN: xlate_learn_action(ctx, ofpact_get_LEARN(a)); break; diff --git a/tests/ofproto-dpif.at b/tests/ofproto-dpif.at index da29ac2..6d9197b 100644 --- a/tests/ofproto-dpif.at +++ b/tests/ofproto-dpif.at @@ -5309,6 +5309,59 @@ PORTNAME portName=p2 ])]) +AT_SETUP([ofproto-dpif - output_trunc action]) +OVS_VSWITCHD_START +add_of_ports br0 1 2 3 4 5 + +AT_CHECK([ovs-vsctl -- set Interface p1 type=dummy options:pcap=p1.pcap]) +AT_CHECK([ovs-vsctl -- set Interface p2 type=dummy options:pstream=punix:p2.sock]) +AT_CHECK([ovs-vsctl -- set Interface p3 type=dummy options:stream=unix:p2.sock]) +AT_CHECK([ovs-vsctl -- set Interface p4 type=dummy options:pstream=punix:p4.sock]) +AT_CHECK([ovs-vsctl -- set Interface p5 type=dummy options:stream=unix:p4.sock]) + +dnl output:2(max_len=32) shows here as truncated size +AT_CHECK([ovs-ofctl add-flow br0 'in_port=3,actions=drop']) +AT_CHECK([ovs-ofctl add-flow br0 'in_port=5,actions=drop']) +AT_CHECK([ovs-ofctl add-flow br0 'in_port=1,actions=output(port=2,max_len=64),output:4']) + +dnl An 170 byte packet +AT_CHECK([ovs-appctl netdev-dummy/receive p1 '000c29c8a0a4005056c0000808004500009cb4a6000040019003c0a8da01c0a8da640800cb5fa762000556f431ad0009388e08090a0b0c0d0e0f101112131415161718191a1b1c1d1e1f202122232425262728292a2b2c2d2e2f303132333435363738393a3b3c3d3e3f404142434445464748494a4b4c4d4e4f505152535455565758595a5b5c5d5e5f606162636465666768696a6b6c6d6e6f707172737475767778797a7b7c7d7e7f']) + +AT_CHECK([ovs-ofctl parse-pcap p1.pcap], [0], [dnl +icmp,in_port=ANY,vlan_tci=0x0000,dl_src=00:50:56:c0:00:08,dl_dst=00:0c:29:c8:a0:a4,nw_src=192.168.218.1,nw_dst=192.168.218.100,nw_tos=0,nw_ecn=0,nw_ttl=64,icmp_type=8,icmp_code=0 +]) +dnl packet with truncated size +AT_CHECK([ovs-ofctl dump-flows br0 | grep "in_port=3" | sed -n 's/.*\(n\_bytes=64\).*/\1/p'], [0], [dnl +n_bytes=64 +]) +dnl dnl packet with original size +AT_CHECK([ovs-ofctl dump-flows br0 | grep "in_port=5" | sed -n 's/.*\(n\_bytes=170\).*/\1/p'], [0], [dnl +n_bytes=170 +]) + +dnl More complicated case +AT_CHECK([ovs-ofctl del-flows br0]) +AT_CHECK([ovs-ofctl add-flow br0 'in_port=3,actions=drop']) +AT_CHECK([ovs-ofctl add-flow br0 'in_port=5,actions=drop']) +AT_CHECK([ovs-ofctl add-flow br0 'in_port=1,actions=output(port=2,max_len=64),output(port=2, max_len=128),output( port=4,max_len=60 ),output:2,output:4']) + +dnl An 170 byte packet +AT_CHECK([ovs-appctl netdev-dummy/receive p1 '000c29c8a0a4005056c0000808004500009cb4a6000040019003c0a8da01c0a8da640800cb5fa762000556f431ad0009388e08090a0b0c0d0e0f101112131415161718191a1b1c1d1e1f202122232425262728292a2b2c2d2e2f303132333435363738393a3b3c3d3e3f404142434445464748494a4b4c4d4e4f505152535455565758595a5b5c5d5e5f606162636465666768696a6b6c6d6e6f707172737475767778797a7b7c7d7e7f']) + +sleep 1 +dnl packet size: 64 + 128 + 170 = 362 +AT_CHECK([ovs-ofctl dump-flows br0 | grep "in_port=3" | sed -n 's/.*\(n\_bytes=362\).*/\1/p'], [0], [dnl +n_bytes=362 +]) +dnl packet size: 60 + 170 = 230 +AT_CHECK([ovs-ofctl dump-flows br0 | grep "in_port=5" | sed -n 's/.*\(n\_bytes=230\).*/\1/p'], [0], [dnl +n_bytes=230 +]) + +OVS_VSWITCHD_STOP +AT_CLEANUP + + AT_SETUP([ofproto-dpif - sFlow packet sampling - IPv4 collector]) CHECK_SFLOW_SAMPLING_PACKET([127.0.0.1]) AT_CLEANUP 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]) + +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]) +AT_CLEANUP + AT_SETUP([datapath - ping6 between two ports]) OVS_TRAFFIC_VSWITCHD_START()
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@gmail.com> --- v1->v2 - Create new OpenFlow action, do not reuse OFPACT_OUTPUT - Create new datapath action --- datapath/actions.c | 18 +++- datapath/datapath.h | 1 + datapath/flow_netlink.c | 9 ++ datapath/linux/compat/include/linux/openvswitch.h | 9 ++ datapath/vport.c | 6 ++ lib/dp-packet.c | 1 + lib/dp-packet.h | 1 + lib/dpif-netdev.c | 28 ++++++ lib/dpif.c | 1 + lib/netdev.c | 8 ++ lib/odp-execute.c | 2 + lib/odp-util.c | 11 ++- lib/ofp-actions.c | 102 ++++++++++++++++++++++ lib/ofp-actions.h | 10 +++ ofproto/ofproto-dpif-sflow.c | 6 ++ ofproto/ofproto-dpif-xlate.c | 39 +++++++++ tests/ofproto-dpif.at | 53 +++++++++++ tests/system-traffic.at | 66 ++++++++++++++ 18 files changed, 366 insertions(+), 5 deletions(-)