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

bindiya Kurle bindiyakurle at gmail.com
Tue Jul 20 07:55:59 UTC 2021


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

>
>    -
>    -
>
>    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_ttl is a dynamic action, the value
from the upcall packet is copied as the mpls_pop is static"

>
>    -
>    -
>
>    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.

>
>    -
>
> 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 ?

> 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 .

> Cheers,
>
> Eelco
>


More information about the dev mailing list