[ovs-dev] [PATCH v4] datapath: Add a new action dec_ttl
Eelco Chaudron
echaudro at redhat.com
Mon Jul 26 10:14:16 UTC 2021
On 20 Jul 2021, at 9:55, bindiya Kurle wrote:
> Comments inline BK >>
>
> On Wed, Jun 23, 2021 at 3:50 PM Eelco Chaudron <echaudro at redhat.com> wrote:
>
>> 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.
>>
> BK >> We do not have use-case of offload flows with clone action to hardware
Ack so this is less of a problem for you.
>>
>> -
>> -
>>
>> 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.
>>
>> BK >> Didn't get this question clearly can you please
> elaborate more on this " because dec_gtl is a dynamic action, the value
> from the upcall packet is copied as the mpls_pop is static"
When you encapsulate an IP packet in a tunnel the TTL value for the tunnel is copied from the original IP packet (the matching part). In the past, this was not a problem, as we would adjust the TTL during the lookup and it was part of the flow (i.e. we would have one flow entry for each TTL value) and copy it. Now we do not adjust/match the TTL as part of the flow, so when we do the copy the wrong TTL value is copied.
>>
>> -
>> -
>>
>> 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.
>>
>> BK >> when dec_ttl is changed in datapath , assumption was once exception
> happens packets should be forwarded to controller.Controller will take
> right action on that packet and no further action will be taken by
> datapath. I understand it contradicts the documentation that you are
> referring to. But do you think this needs to be changed in documentation
> rather than in implementation.Since dec_ttl action is existing in
> user-space for a long time , I'm not sure about the reasoning behind the
> same.
I think it’s too late to change an existing behavior by just changing the documentation :) I have not checked the OpenFlow specification, but it might be part of it.
>>
>> -
>>
>> With all the above, I'm wondering if dec_ttl will solve the actual problem
>> of HW offloading?
>>
> BK >> Do you mean by this, to support dec_ttl in some use-cases , clone
> action needs to be taken care of ,and since clone action is not off
> loadable, hw offloading problem is not solved ?
ACK
>> Bindiya do you have an example of flows to show the use case (both
>> OpenFlow and the generated datapath flows)?
>>
> BK > In our system we do not have the use-case that you mentioned above ,
> so I do not have openflow action and corresponding datapath flows .
I’m referring to your use cases specifically. Can you share your OpenFlow rule set and some example traffic patterns?
However in general to make dec_ttl work as is (even without the HW offload) we still need to solve both above problems.
More information about the dev
mailing list