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

Eelco Chaudron echaudro at redhat.com
Wed May 26 12:34:29 UTC 2021



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!

//Eelco



More information about the dev mailing list