[ovs-dev] [PATCH v4] datapath: Add a new action dec_ttl

Eelco Chaudron echaudro at redhat.com
Wed Jun 23 10:20:37 UTC 2021



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


More information about the dev mailing list