On 26 May 2021, at 14:34, Eelco Chaudron wrote:
On 18 May 2021, at 20:15, Ilya Maximets wrote:
On 5/18/21 4:44 PM, Eelco Chaudron wrote:
Add support for the dec_ttl action. Instead of programming the
datapath with
a flow that matches the packet TTL and an IP set, use a single
dec_ttl action.
The old behavior is kept if the new action is not supported by the
datapath.
# ovs-ofctl dump-flows br0
cookie=0x0, duration=12.538s, table=0, n_packets=4, n_bytes=392,
ip actions=dec_ttl,NORMAL
cookie=0x0, duration=12.536s, table=0, n_packets=4, n_bytes=168,
actions=NORMAL
# ping -c1 -t 20 192.168.0.2
PING 192.168.0.2 (192.168.0.2) 56(84) bytes of data.
IP (tos 0x0, ttl 19, id 45336, offset 0, flags [DF], proto ICMP
(1), length 84)
192.168.0.1 > 192.168.0.2: ICMP echo request, id 8865, seq 1,
length 64
Linux netlink datapath support depends on upstream Linux commit:
744676e77720 ("openvswitch: add TTL decrement action")
Note that in the Linux kernel tree the OVS_ACTION_ATTR_ADD_MPLS has
been
defined, and to make sure the IDs are in sync, it had to be added to
the
OVS source tree. This required some additional case statements,
which
should be revisited once the OVS implementation is added.
<SNIP>
@@ -5186,6 +5187,40 @@ xlate_delete_field(struct xlate_ctx *ctx,
ds_destroy(&s);
}
+/* New handling for dec_ttl action. */
'New handling' makes sense in a patch, but doesn't while reading the
code.
Yes, I will remove this comment altogether as it makes no sense.
<SNIP>
+/* Tests whether 'dpif' datapath supports decrement of the IP TTL
via
+ * OVS_ACTION_DEC_TTL. */
+static bool
+check_dec_ttl_action(struct dpif *dpif)
+{
+ struct odputil_keybuf keybuf;
+ struct flow flow = { 0 };
It's probbaly better to just memset it as in other similar functions
to avoid compiler's complains.
ACK, will use a memset here.
<SNIP>
/* Tests whether 'backer''s datapath supports the clone action
* OVS_ACTION_ATTR_CLONE. */
static bool
@@ -1590,6 +1629,7 @@ check_support(struct dpif_backer *backer)
dpif_supports_explicit_drop_action(backer->dpif);
backer->rt_support.lb_output_action=
dpif_supports_lb_output_action(backer->dpif);
+ backer->rt_support.dec_ttl_action =
check_dec_ttl_action(backer->dpif);
During discussions about all-zero SNAT feature support I remembered
that
we have a 'capabilities' table that should reflect all the datapath
supported fetures. So, this should be added there as well. And
documented
in vswitchd/vswitch.xml.
ACK, will add.
<SNIP>
/* Stores the various features which the corresponding backer
supports. */
diff --git a/tests/odp.at b/tests/odp.at
index b762ebb2b..24946bec4 100644
--- a/tests/odp.at
+++ b/tests/odp.at
@@ -384,6 +384,7 @@ check_pkt_len(size=200,gt(drop),le(5))
check_pkt_len(size=200,gt(ct(nat)),le(drop))
check_pkt_len(size=200,gt(set(eth(src=00:01:02:03:04:05,dst=10:11:12:13:14:15))),le(set(eth(src=00:01:02:03:04:06,dst=10:11:12:13:14:16))))
lb_output(1)
+dec_ttl(le_1(userspace(pid=3614533484,controller(reason=2,dont_send=0,continuation=0,recirc_id=1,rule_cookie=0,controller_id=0,max_len=65535))))
Maybe it will make sense to also add a very simple variant of this
action,
e.g. dec_ttl(le_1(drop)).
Added, drop and it resulted in an issue (fixed).
Doing some final tests and will sent out a V5 soon!
Hi Ilya/Bindiya,
I was preparing my v5, and I noticed that a bunch of self-tests fail. I
was wondering why I (and Matteo/Bindiya) never noticed. For me, it was
because I was running make check on my dev system, which had an old
kernel :( The datapath tests I was running on my DUT.
I did solve most of the problems, but there are some corner cases that
might be hard (and was wondering if you guys even thought about them):
- As dec_ttl is none reversible there are some cases that it needs to
clone the packet. Take the following example with a patch port (to solve
this I sent another preceding patch, "ofproto-dpif: fix issue with
non-reversible actions on a patch port"):
Rule set:
ovs-ofctl -O OpenFlow13 add-flow br0
in_port=local,ip,actions=2,1])
[br0 port 2 <===> port 1 br1]
ovs-ofctl -O OpenFlow13 add-flow br1
in_port=1,ip,actions=dec_ttl,push_mpls:0x8847,3])
Becomes:
clone(dec_ttl(le_1(userspace(pid=0,controller(reason=2,dont_send=1,continuation=0,recirc_id=1,rule_cookie=0,controller_id=0,max_len=65535)))),push_mpls(label=0,tc=0,ttl=63,bos=1,eth_type=0x8847),3),1
Where it was:
set(ipv4(ttl=63)),push_mpls(label=0,tc=0,ttl=63,bos=1,eth_type=0x8847),3,pop_mpls(eth_type=0x800),set(ipv4(ttl=64)),1'
This clone action is NOT hardware offloadable, so wondering if this
is needed in your use case? This is fixed in my v5.
- tunnel encapsulation copies TTL value from the header however, because
dec_ttl is a dynamic action, the value from the upcall packet is copied
as the mpls_pop is static
Not sure how to solve this as the TTL value is hardcoded by the
datapath rule? Any ideas, other than changing flower to support dynamic
TTL (with all the additional complex logic), or track the number of
dec_ttl actions before encap?
Note that doing encapsulation after dec_ttl might already cause
dec_ttl dp action to lose its benefits as the nw_ttl match is added.
- The documentation for the dec_ttl OpenFlow action states the
following:
"However, if the current set of actions was reached through resubmit,
the remaining actions in outer levels resume processing."
This is also something that is hard, as the dec_ttl dp action will
stop processing after the exception. So take the following example:
bridge("br0")
-------------
0. in_port=1, priority 32768
dec_ttl
output:2
resubmit(1,1)
1. in_port=1, priority 32768
dec_ttl
output:3
output:4
The old actions are resolved as follows:
Megaflow: recirc_id=0,eth,ip,in_port=1,nw_ttl=2,nw_frag=no
Datapath actions:
set(ipv4(ttl=1)),2,userspace(pid=0,controller(reason=2,dont_send=0,continuation=0,recirc_id=1,rule_cookie=0,controller_id=0,max_len=65535)),4
The new action without a proper fix:
Megaflow: recirc_id=0,eth,ip,in_port=1,nw_frag=no
Datapath actions:
dec_ttl(le_1(userspace(pid=0,controller(reason=2,dont_send=0,continuation=0,recirc_id=1,rule_cookie=0,controller_id=0,max_len=65535)))),2,dec_ttl(le_1(userspace(pid=0,controller(reason=2,dont_send=0,continuation=0,recirc_id=2,rule_cookie=0,controller_id=0,max_len=65535)))),3,4"
Without a proper fix, this will not send the packet to port 4. Also
not sure how to solve this, guess we could do another clone here. But I
have no idea if this path can be detected to avoid an unnecessary clone,
need some research.
With all the above, I'm wondering if dec_ttl will solve the actual
problem of HW offloading? Bindiya do you have an example of flows to
show the use case (both OpenFlow and the generated datapath flows)?
Cheers,
Eelco
_______________________________________________
dev mailing list
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev
